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.
Created attachment 429558 [details] Patch
Created attachment 429559 [details] Patch fix style nits
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
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.
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"?
(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*
Created attachment 429591 [details] Patch Thanks Saam for the review! I updated the patch accordingly.
Created attachment 429594 [details] Patch Fix changelog and a typo.
Created attachment 429601 [details] Patch Remove two extraneous "JS_EXPORT_PRIVATE"
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.
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.
<rdar://problem/78693011>
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.
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.
Created attachment 430378 [details] Patch for landing rebased
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].
Re-opened since this is blocked by bug 226749
Created attachment 430894 [details] Patch for re-landing Re-landing with a fix.
Found 1 new test failure: imported/w3c/web-platform-tests/navigation-timing/nav2_test_attributes_values.html
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.
Comment on attachment 430894 [details] Patch for re-landing Does not work, I'll re-upload the patch.
Comment on attachment 430894 [details] Patch for re-landing Actually was working, just took a while to update the EWS bubble.
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].