RESOLVED FIXED 226187
We should drop B3 values while running Air
https://bugs.webkit.org/show_bug.cgi?id=226187
Summary We should drop B3 values while running Air
Robin Morisset
Reported 2021-05-24 13:37:09 PDT
B3 Values are not very big, but they can be quite numerous, and running Air is often the peak memory use for JSC. So instead of keeping all of B3::Procedure including all of its values throughout Air, we should only keep what is needed, and free everything else at the end of B3::lowerToAir.
Attachments
Patch (9.49 KB, patch)
2021-05-24 13:46 PDT, Robin Morisset
no flags
Patch (9.50 KB, patch)
2021-05-24 13:51 PDT, Robin Morisset
no flags
Patch (11.75 KB, patch)
2021-05-24 16:57 PDT, Robin Morisset
no flags
Patch (12.04 KB, patch)
2021-05-24 17:01 PDT, Robin Morisset
saam: review+
Patch (12.00 KB, patch)
2021-05-24 17:37 PDT, Robin Morisset
no flags
Patch (12.54 KB, patch)
2021-05-26 16:15 PDT, Robin Morisset
no flags
Patch (14.67 KB, patch)
2021-06-01 15:30 PDT, Robin Morisset
no flags
Patch for landing (14.38 KB, patch)
2021-06-02 11:44 PDT, Robin Morisset
no flags
Patch for re-landing (14.91 KB, patch)
2021-06-08 14:51 PDT, Robin Morisset
no flags
Robin Morisset
Comment 1 2021-05-24 13:46:36 PDT
Robin Morisset
Comment 2 2021-05-24 13:51:07 PDT
Created attachment 429559 [details] Patch fix style nits
Saam Barati
Comment 3 2021-05-24 16:13:24 PDT
Comment on attachment 429559 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=429559&action=review > Source/JavaScriptCore/b3/air/AirCode.cpp:60 > + , m_preserveB3Origins(Options::dumpAirGraphAtEachPhase() || Options::dumpFTLDisassembly() || Options::useSamplingProfiler()) this isn't how you test for the sampling profiler. It needs to be piped through from VM::shouldBuilderPCToCodeOriginMapping
Saam Barati
Comment 4 2021-05-24 16:14:39 PDT
Comment on attachment 429559 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=429559&action=review > Source/JavaScriptCore/b3/B3SparseCollection.h:121 > + void transferOrFreeAll(Vector<std::unique_ptr<T>>& dst, const Functor& functor) why not just call this transferTo? Not sure what "OrFreeAll" is adding here in the name.
Saam Barati
Comment 5 2021-05-24 16:15:37 PDT
Comment on attachment 429559 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=429559&action=review > Source/JavaScriptCore/b3/B3Procedure.cpp:442 > +void Procedure::freeMostB3Values() name nit: not sure I love the name here. Maybe something more along the lines of "freeUnneeededB3ValuesAfterLowing"?
Saam Barati
Comment 6 2021-05-24 16:15:47 PDT
(In reply to Saam Barati from comment #5) > Comment on attachment 429559 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=429559&action=review > > > Source/JavaScriptCore/b3/B3Procedure.cpp:442 > > +void Procedure::freeMostB3Values() > > name nit: not sure I love the name here. Maybe something more along the > lines of "freeUnneeededB3ValuesAfterLowing"? Lowering*
Robin Morisset
Comment 7 2021-05-24 16:57:35 PDT
Created attachment 429591 [details] Patch Thanks Saam for the review! I updated the patch accordingly.
Robin Morisset
Comment 8 2021-05-24 17:01:14 PDT
Created attachment 429594 [details] Patch Fix changelog and a typo.
Robin Morisset
Comment 9 2021-05-24 17:37:37 PDT
Created attachment 429601 [details] Patch Remove two extraneous "JS_EXPORT_PRIVATE"
Robin Morisset
Comment 10 2021-05-26 16:15:22 PDT
Created attachment 429810 [details] Patch Dropping m_tuples was causing issues. Also we should do the freeing after the end of lowerToAir, since the validator may run on the B3 procedure at the end of lowerToAir.
Robin Morisset
Comment 11 2021-05-30 13:14:32 PDT
I've finally got RAMification numbers for this patch: Effect on MacBookPro15,2: 3d-cube-SP: 0.37% better (p<0.02) prepack-wtb: 1.58% better (p<0.02) stanford-crypto-sha1: 0.39% better (p<0.02) Nothing else highly significant, but lots of p<0.2 progressions and no visible regression Effect on iPhone7: Air: regression of 2.31% (p<0.02) UniPoker: regression of 0.53% (p<0.02) crypto-aes-SP: 0.27% better (p<0.02) date-format-tofte: regression of 0.15% (p<0.02) quicksort-wasm: 1.05% better (p<0.02) richards: regression of 0.66% (p<0.02) typescript: regression of 26.29% !! (p<0.02) The effect on typescript is definitely spurious (typescript memory consumption is heavily bimodal, being sometimes 270MB and sometimes 400MB, I just got unlucky and had no 400MB runs among the control runs on this machine), but the other numbers are just puzzling. JetStream2 on the iPhone7 also shows a weird mix of progressions and regressions, with more regressions overall.
Radar WebKit Bug Importer
Comment 12 2021-05-31 13:38:16 PDT
Robin Morisset
Comment 13 2021-06-01 14:56:04 PDT
Since this is a progression on Mac, I've decided (after discussing it with Phil) to try to land this. I'll monitor the performance bots in case the iPhone regression was somehow real.
Robin Morisset
Comment 14 2021-06-01 15:30:25 PDT
Created attachment 430299 [details] Patch Simplification: we now shrink B3::Procedure::m_values in-place, without needing to add a Vector to Air::Code. I also updated the Changelog.
Robin Morisset
Comment 15 2021-06-02 11:44:18 PDT
Created attachment 430378 [details] Patch for landing rebased
EWS
Comment 16 2021-06-02 13:46:02 PDT
Committed r278371 (238398@main): <https://commits.webkit.org/238398@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 430378 [details].
WebKit Commit Bot
Comment 17 2021-06-07 18:25:26 PDT
Re-opened since this is blocked by bug 226749
Robin Morisset
Comment 18 2021-06-08 14:51:02 PDT
Created attachment 430894 [details] Patch for re-landing Re-landing with a fix.
EWS
Comment 19 2021-06-08 17:17:44 PDT
Found 1 new test failure: imported/w3c/web-platform-tests/navigation-timing/nav2_test_attributes_values.html
Robin Morisset
Comment 20 2021-06-12 09:48:18 PDT
Comment on attachment 430894 [details] Patch for re-landing Trying to get it through the commit queue once more, since the tests were known flakey at the time it failed.
Robin Morisset
Comment 21 2021-06-12 09:48:53 PDT
Comment on attachment 430894 [details] Patch for re-landing Does not work, I'll re-upload the patch.
Robin Morisset
Comment 22 2021-06-12 09:49:27 PDT
Comment on attachment 430894 [details] Patch for re-landing Actually was working, just took a while to update the EWS bubble.
EWS
Comment 23 2021-06-12 10:17:34 PDT
Committed r278810 (238765@main): <https://commits.webkit.org/238765@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 430894 [details].
Note You need to log in before you can comment on or make changes to this bug.