RESOLVED FIXED 206975
Most of B3 and Air does not need to include CCallHelpers.h 
https://bugs.webkit.org/show_bug.cgi?id=206975
Summary Most of B3 and Air does not need to include CCallHelpers.h
Robin Morisset
Reported 2020-01-29 17:19:27 PST
They only do to use CCallHelpers::Jump or CCallHelpers::Label. But CCallHelpers inherit those from MacroAssembler. And MacroAssembler.h is dramatically cheaper to include (since CCallHelpers includes AssemblyHelpers which includes CodeBlock.h which includes roughly the entire runtime).
Attachments
Patch (25.75 KB, patch)
2020-01-29 17:21 PST, Robin Morisset 		rmorisset: review-
rmorisset: commit-queue-
DetailsFormatted DiffDiff
Patch (27.55 KB, patch)
2020-01-29 17:59 PST, Robin Morisset 		no flags
DetailsFormatted DiffDiff
Patch (28.04 KB, patch)
2020-01-29 18:19 PST, Robin Morisset 		no flags
DetailsFormatted DiffDiff
Patch (27.62 KB, patch)
2020-01-29 18:23 PST, Robin Morisset 		no flags
DetailsFormatted DiffDiff
Patch (27.64 KB, patch)
2020-02-03 19:07 PST, Robin Morisset 		no flags
DetailsFormatted DiffDiff
Patch (26.52 KB, patch)
2020-02-06 16:19 PST, Robin Morisset 		no flags
DetailsFormatted DiffDiff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.
Robin Morisset
Comment 1 2020-01-29 17:21:32 PST
Created attachment 389204 [details] Patch
Robin Morisset
Comment 2 2020-01-29 17:47:54 PST
Comment on attachment 389204 [details] Patch My patch visibly does not apply cleanly on ToT.
Robin Morisset
Comment 3 2020-01-29 17:59:36 PST
Created attachment 389210 [details] Patch
Robin Morisset
Comment 4 2020-01-29 18:19:45 PST
Created attachment 389213 [details] Patch
Robin Morisset
Comment 5 2020-01-29 18:23:14 PST
Created attachment 389214 [details] Patch
Robin Morisset
Comment 6 2020-02-03 19:07:12 PST
Created attachment 389616 [details] Patch Fix style nits.
Mark Lam
Comment 7 2020-02-06 15:35:05 PST
Comment on attachment 389616 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=389616&action=review > Source/JavaScriptCore/b3/B3Common.cpp:42 > - return FTL::verboseCompilationEnabled() || FTL::shouldDumpDisassembly() || shouldDumpIRAtEachPhase(mode); > + return DFG::verboseCompilationEnabled(DFG::FTLMode) || DFG::shouldDumpDisassembly(DFG::FTLMode) || shouldDumpIRAtEachPhase(mode); I don't think this is a good thing to do. There's no guarantee that FTL::verboseCompilationEnabled() will always be a forwarding wrapper to DFG::verboseCompilationEnabled(). Ditto for FTL::shouldDumpDisassembly(). I suggest either: 1. Get rid of FTL::verboseCompilationEnabled() and FTL::shouldDumpDisassembly(), and use DFG versions everywhere. Or ... 2. Create a separate header file that contains FTL::verboseCompilationEnabled() and FTL::shouldDumpDisassembly() only, and continue to use them here. Or ... 3. Continue to #include "FTLState.h" here. I recommend either (2) or (3). > Source/WTF/wtf/SingleRootGraph.h:32 > +#include <wtf/StringPrintStream.h> Isn't this already in the code base? I see it in my local checkout. If so, make sure to remove the ChangeLog as well.
Robin Morisset
Comment 8 2020-02-06 16:19:06 PST
Created attachment 390023 [details] Patch - I verified that even without unified sources this patch does not break the build - I reverted the change that removed FTLState.h from B3Common, following Mark's comment - I removed the wtf/Changelog since the corresponding change has already landed in the tree
Mark Lam
Comment 9 2020-02-06 16:23:11 PST
Comment on attachment 390023 [details] Patch r=me. If you have a record of the build time difference on a unified build of JSC, it would be nice to include that as well in the ChangeLog for posterity.
Robin Morisset
Comment 10 2020-02-06 16:29:26 PST
(In reply to Mark Lam from comment #9) > Comment on attachment 390023 [details] > Patch > > r=me. If you have a record of the build time difference on a unified build > of JSC, it would be nice to include that as well in the ChangeLog for > posterity. Thanks for the review. The compile time of JSC as a whole is a bit too variable to give good results here. But from what I remember, the difference when looking only at B3 files was quite significant (something like 20% in Debug builds).
WebKit Commit Bot
Comment 11 2020-02-06 19:12:38 PST
Comment on attachment 390023 [details] Patch Clearing flags on attachment: 390023 Committed r256003: <https://trac.webkit.org/changeset/256003>
WebKit Commit Bot
Comment 12 2020-02-06 19:12:40 PST
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 13 2020-02-06 19:13:19 PST
<rdar://problem/59248505>
Note You need to log in before you can comment on or make changes to this bug.