Bug 226187 - We should drop B3 values while running Air
Summary: We should drop B3 values while running Air
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Robin Morisset
URL:
Keywords: InRadar
Depends on: 226749
Blocks:
  Show dependency treegraph
 
Reported: 2021-05-24 13:37 PDT by Robin Morisset
Modified: 2021-06-12 10:17 PDT (History)
8 users (show)

See Also:


Attachments
Patch (9.49 KB, patch)
2021-05-24 13:46 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff
Patch (9.50 KB, patch)
2021-05-24 13:51 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff
Patch (11.75 KB, patch)
2021-05-24 16:57 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff
Patch (12.04 KB, patch)
2021-05-24 17:01 PDT, Robin Morisset
sbarati: review+
Details | Formatted Diff | Diff
Patch (12.00 KB, patch)
2021-05-24 17:37 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff
Patch (12.54 KB, patch)
2021-05-26 16:15 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff
Patch (14.67 KB, patch)
2021-06-01 15:30 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff
Patch for landing (14.38 KB, patch)
2021-06-02 11:44 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff
Patch for re-landing (14.91 KB, patch)
2021-06-08 14:51 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robin Morisset 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.
Comment 1 Robin Morisset 2021-05-24 13:46:36 PDT
Created attachment 429558 [details]
Patch
Comment 2 Robin Morisset 2021-05-24 13:51:07 PDT
Created attachment 429559 [details]
Patch

fix style nits
Comment 3 Saam Barati 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
Comment 4 Saam Barati 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.
Comment 5 Saam Barati 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"?
Comment 6 Saam Barati 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*
Comment 7 Robin Morisset 2021-05-24 16:57:35 PDT
Created attachment 429591 [details]
Patch

Thanks Saam for the review! I updated the patch accordingly.
Comment 8 Robin Morisset 2021-05-24 17:01:14 PDT
Created attachment 429594 [details]
Patch

Fix changelog and a typo.
Comment 9 Robin Morisset 2021-05-24 17:37:37 PDT
Created attachment 429601 [details]
Patch

Remove two extraneous "JS_EXPORT_PRIVATE"
Comment 10 Robin Morisset 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.
Comment 11 Robin Morisset 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.
Comment 12 Radar WebKit Bug Importer 2021-05-31 13:38:16 PDT
<rdar://problem/78693011>
Comment 13 Robin Morisset 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.
Comment 14 Robin Morisset 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.
Comment 15 Robin Morisset 2021-06-02 11:44:18 PDT
Created attachment 430378 [details]
Patch for landing

rebased
Comment 16 EWS 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].
Comment 17 WebKit Commit Bot 2021-06-07 18:25:26 PDT
Re-opened since this is blocked by bug 226749
Comment 18 Robin Morisset 2021-06-08 14:51:02 PDT
Created attachment 430894 [details]
Patch for re-landing

Re-landing with a fix.
Comment 19 EWS 2021-06-08 17:17:44 PDT
Found 1 new test failure: imported/w3c/web-platform-tests/navigation-timing/nav2_test_attributes_values.html
Comment 20 Robin Morisset 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.
Comment 21 Robin Morisset 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.
Comment 22 Robin Morisset 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.
Comment 23 EWS 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].