Bug 226556

Summary: Drop the FTL(DFG) graph after lowering to B3
Product: WebKit Reporter: Robin Morisset <rmorisset>
Component: JavaScriptCoreAssignee: Robin Morisset <rmorisset>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ews-watchlist, fpizlo, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 226749    
Bug Blocks: 227095    
Attachments:
Description Flags
Patch
fpizlo: review+, mark.lam: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch for landing
ews-feeder: commit-queue-
Patch for landing
none
Patch for re-landing
ews-feeder: commit-queue-
Patch for re-landing none

Description Robin Morisset 2021-06-02 15:40:29 PDT
It is a fairly simple memory saving, that may well be significant since it will last through all of B3 and Air.
Comment 1 Robin Morisset 2021-06-02 15:56:08 PDT
Created attachment 430414 [details]
Patch
Comment 2 Robin Morisset 2021-06-02 16:06:56 PDT
I do not have performance numbers for this yet, I am currently launching the relevant A/B tasks and will add a comment with the results here once they complete.
Comment 3 Mark Lam 2021-06-02 16:23:51 PDT
Comment on attachment 430414 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=430414&action=review

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:10140
> +                Checked<int32_t> offsetCount { 1 };

This appears to be unused.
Comment 4 Robin Morisset 2021-06-02 16:50:23 PDT
Comment on attachment 430414 [details]
Patch

The Mac-wk2 bot has found a bug, I'm investigating it.
Comment 5 Robin Morisset 2021-06-02 18:05:07 PDT
Created attachment 430428 [details]
Patch

Fixed a bug: I forgot disabling the freeing in the case where --useSamplingProfiler=1.
As it relies on PCToCodeOriginMapBuilder which contains code such as
```
DFG::Node* node = bitwise_cast<DFG::Node*>(originRange.origin.data());
        if (node)
            appendItem(originRange.label, node->origin.semantic);
```
, having freed all DFG::Nodes obviously lead to trouble.
Comment 6 Robin Morisset 2021-06-03 16:20:15 PDT
Created attachment 430512 [details]
Patch

fix two bugs related to the DOMJIT: I had missed the fact that it uses the DOMJITSnippets in a late-running generator (so freeing them obviously crashes), and that FTL::SnippetParams was capturing a Node* (which was easy to fix as it was only using it for its CodeOrigin).
Comment 7 Robin Morisset 2021-06-03 16:28:45 PDT
I got some RAMification results on an M1 MacBookAir for this patch. Listing all of the subtests with p<0.05 changes:
- Box2D: 1.66% better (p<0.02)
- ML: 0.84% better (p<0.05)
- OfflineAssembler: 4.78% better (p<0.02)
- coffeescript-wtb: regression of 0.71% ! (p<0.05)
- crypto-aes-sp: 2.42% better (p<0.02)
- crypto-md5-sp: 3.10% better (p<0.02)
- crypto-sha1-sp: 0.89% better (p<0.02)
- date-format-tofte-sp: regression of 0.27% ! (p<0.02)
- delta-blue: 2.56% better (p<0.02)
- early-boyer: 0.86% better (p<0.05)
- gbemu: 1.74% better (p<0.02)
- navier-stokes: 1.52% better (p<0.02)
- octane-code-load: 0.39% better (p<0.05)
- raytrace: 2.00% better (p<0.02)
- regex-dna-sp: 0.65% better (p<0.02)
- regexp: 2.85% better (p<0.02)
- stanford-crypto-pbkdf2: 0.91% better (p<0.02)
- stanford-crypto-sha256: 0.46% better (p<0.02)

So despite some noise (as I only got 5 iterations instead of 8 as planned as the performance bots have been flaky in the past few days), it seems to be a pretty clear win.

I'll try to land it once EWS is happy with it.
Comment 8 EWS 2021-06-03 22:34:23 PDT
Tools/Scripts/svn-apply failed to apply attachment 430512 [details] to trunk.
Please resolve the conflicts and upload a new patch.
Comment 9 Robin Morisset 2021-06-04 00:43:40 PDT
Created attachment 430555 [details]
Patch for landing

rebased
Comment 10 EWS 2021-06-04 01:26:31 PDT
Patch 430555 does not build
Comment 11 Robin Morisset 2021-06-04 08:35:12 PDT
Created attachment 430580 [details]
Patch for landing
Comment 12 EWS 2021-06-04 09:04:41 PDT
Committed r278463 (238483@main): <https://commits.webkit.org/238483@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 430580 [details].
Comment 13 Radar WebKit Bug Importer 2021-06-04 09:05:21 PDT
<rdar://problem/78871457>
Comment 14 WebKit Commit Bot 2021-06-07 18:25:28 PDT
Re-opened since this is blocked by bug 226749
Comment 15 Robin Morisset 2021-06-08 15:03:27 PDT
Created attachment 430898 [details]
Patch for re-landing
Comment 16 EWS 2021-06-08 17:59:29 PDT
Patch 430898 does not build
Comment 17 Robin Morisset 2021-06-15 19:32:39 PDT
Created attachment 431510 [details]
Patch for re-landing

rebased
Comment 18 Robin Morisset 2021-06-16 11:10:38 PDT
Comment on attachment 431510 [details]
Patch for re-landing

Re-landing.
Comment 19 EWS 2021-06-16 11:34:32 PDT
Committed r278945 (238875@main): <https://commits.webkit.org/238875@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 431510 [details].