WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
saam
: 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
Show Obsolete
(8)
View All
Add attachment
proposed patch, testcase, etc.
Robin Morisset
Comment 1
2021-05-24 13:46:36 PDT
Created
attachment 429558
[details]
Patch
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
<
rdar://problem/78693011
>
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.
Top of Page
Format For Printing
XML
Clone This Bug