Bug 226187 – We should drop B3 values while running Air

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Robin Morisset]

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.

[Robin Morisset]

Created attachment 429558 [details]
Patch

[Robin Morisset]

Created attachment 429559 [details]
Patch

fix style nits

[Saam Barati]

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 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 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]

(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]

Created attachment 429591 [details]
Patch

Thanks Saam for the review! I updated the patch accordingly.

[Robin Morisset]

Created attachment 429594 [details]
Patch

Fix changelog and a typo.

[Robin Morisset]

Created attachment 429601 [details]
Patch

Remove two extraneous "JS_EXPORT_PRIVATE"

[Robin Morisset]

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]

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]

<rdar://problem/78693011>

[Robin Morisset]

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]

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]

Created attachment 430378 [details]
Patch for landing

rebased

[EWS]

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]

Re-opened since this is blocked by bug 226749

[Robin Morisset]

Created attachment 430894 [details]
Patch for re-landing

Re-landing with a fix.

[EWS]

Found 1 new test failure: imported/w3c/web-platform-tests/navigation-timing/nav2_test_attributes_values.html

[Robin Morisset]

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 on attachment 430894 [details]
Patch for re-landing

Does not work, I'll re-upload the patch.

[Robin Morisset]

Comment on attachment 430894 [details]
Patch for re-landing

Actually was working, just took a while to update the EWS bubble.

[EWS]

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].