Page MenuHomePhabricator

Cache attribute checks
ClosedPublic

Authored by nickwasmer on Sun, Apr 18, 9:01 PM.

Details

Summary

As a followup to D99362 cache the checks that attributes belong to the same context. Looking for presence in a FoldingSet is slow.

Diff Detail

Event Timeline

nickwasmer created this revision.Sun, Apr 18, 9:01 PM
nickwasmer requested review of this revision.Sun, Apr 18, 9:02 PM

I triggered a run in the compile-time tracker for this patch:
http://llvm-compile-time-tracker.com/compare.php?from=a7712091ea7a2b4a7b5c4764af7545a7574b651b&to=f01acba0cdc4625738be504449a71704595cd12f&stat=instructions

Looks like an improvement, but I'm not how link time is exposed in that interface... or maybe it isn't? @nikic, you mentioned there was a 1% regression in link time: can you confirm if this fixes it?

Note I also triggered a run where attributes themselves are also cached:
http://llvm-compile-time-tracker.com/compare.php?from=f01acba0cdc4625738be504449a71704595cd12f&to=68b7232d1d68abb75486de8588094742c3146ba9&stat=instructions
Seems more noisy.

I triggered a run in the compile-time tracker for this patch:
http://llvm-compile-time-tracker.com/compare.php?from=a7712091ea7a2b4a7b5c4764af7545a7574b651b&to=f01acba0cdc4625738be504449a71704595cd12f&stat=instructions

Looks like an improvement, but I'm not how link time is exposed in that interface... or maybe it isn't? @nikic, you mentioned there was a 1% regression in link time: can you confirm if this fixes it?

Ah, I found it; the "per-file details" checkbox says it's .29% faster:

tramp3d-v4	213713M	213394M (-0.15%)
    CMakeFiles/tramp3d-v4.dir/tramp3d-v4.cpp.o	100598M	100613M (+0.01%)
    tramp3d-v4.link	113114M	112781M (-0.29%)

for LegacyPM-ReleaseLTO-g, with my follow-up that adds attribute caching totally in the noise:

tramp3d-v4	213394M	213318M (-0.04%)
    CMakeFiles/tramp3d-v4.dir/tramp3d-v4.cpp.o	100613M	100575M (-0.04%)
    tramp3d-v4.link	112781M	112744M (-0.03%)

This patch LGTM (certainly an improvement), but maybe this isn't quite enough. @nikic, thoughts?

nikic accepted this revision.Mon, Apr 19, 2:39 PM

I think this is good enough. Doesn't recover fully, but most of the way.

This revision is now accepted and ready to land.Mon, Apr 19, 2:39 PM
dexonsmith accepted this revision.Mon, Apr 19, 2:40 PM
This revision was automatically updated to reflect the committed changes.