This is an archive of the discontinued LLVM Phabricator instance.

[AST] Split out Attrs.h for table generated *Attr classes
AbandonedPublic

Authored by rnk on Nov 22 2019, 5:00 PM.

Details

Summary

Many AST headers require Attr to be complete, but do not need the
complete table generated list of attributes. Separate that into Attrs.h
from Attr.h, and include it where necessary.

-ftime-trace shows that including Attrs.h by itself takes 370ms to
parse, so separating it out is valuable.

After this change, 1706 files depend on Attr.h, but only 600 object
files depend on Attrs.h. This represents a potential savings of 409s CPU
time per build, but on my workstation, a local build only got faster by
2-3s overall (4m45s -> 4m42s).

Event Timeline

rnk created this revision.Nov 22 2019, 5:00 PM

Basically LGTM but I am hoping we can get a better name than Attr.h and Attrs.h.

clang/include/clang/AST/Attrs.h
1

This should read Attrs.h instead, but I find the naming distinction to be a bit too subtle. How about AttrSubclasses.h or ConcreteAttrs.h (other suggestions welcome too).

13

Once the file name is updated, be sure to update the include guards.

rnk marked an inline comment as done.Nov 25 2019, 1:05 PM

Thanks!

clang/include/clang/AST/Attrs.h
1

WDYT about following Decl.h / DeclBase.h? So, rename Attr.h to AttrBase.h, update all current includers of Attr.h to AttrBase.h, then add in new includes of Attr.h. Perhaps staged as two commits, one to do the rename, one to introduce the new header.

Otherwise, I guess I like AttrSubclasses.h from your list, or AttrClasses.h as an alternative.

aaron.ballman added inline comments.Nov 25 2019, 1:09 PM
clang/include/clang/AST/Attrs.h
1

I think AttrBase.h for the Attr base class makes a lot of sense -- it makes it more clear that you only get the base class. WDYT about Attrs.h for the subclasses instead of Attr.h -- might make it a bit more clear that you're getting all of the attributes, not just the base class?

rnk marked an inline comment as done.Nov 25 2019, 2:09 PM
rnk added inline comments.
clang/include/clang/AST/Attrs.h
1

That makes sense. Do you mind if I land this first, and then do the Attr.h -> AttrBase.h rename? That way I don't have to make a new patch, put it up for review, and rebase this patch over it. I'll send a review assuming the answer is yes and then reorder them if you have concerns.

aaron.ballman added inline comments.Nov 25 2019, 2:13 PM
clang/include/clang/AST/Attrs.h
1

Totally fine with that approach, thanks for checking!

rnk marked an inline comment as done.Nov 25 2019, 2:19 PM
rnk added inline comments.
clang/include/clang/AST/Attrs.h
1

Hm, I just realized that this will create a lot of churn for out of tree includers of Attr.h. Chrome's plugins have three hits:
https://cs.chromium.org/search/?q=include.*AST/Attr%5C.h&sq=package:chromium&type=cs
Other users will have more.

Clang doesn't promise API stability, so we can certainly do this, but it creates a fair volume of work.

Now I'm considering splitting out AttrBase.h on its own and updating as many includes of Attr.h to use that as possible. That'll end up being a much less disruptive and much smaller change.

aaron.ballman accepted this revision.Nov 25 2019, 2:20 PM

LGTM with the follow-up plan.

This revision is now accepted and ready to land.Nov 25 2019, 2:20 PM
rnk abandoned this revision.Dec 9 2019, 4:10 PM

I think this isn't necessary. I think I got most of the value of this in rG60573ae6fe509b618dc6a2c5c55d921bccd77608, and we don't need two AttrBase.h / Attr.h headers in the long run. This is how many files depend on Attr.h now:

$ ninja -t deps | grep 'AST.Attr\.h' | wc -l
683

I think most deps are through Sema.h and ASTMatchers.h:

$ ninja -t deps | grep 'Sema.Sema\.h' | wc -l
102
$ ninja -t deps | grep 'ASTMatchers.ASTMatchers\.h' | wc -l
391