Bug 226556 - Drop the FTL(DFG) graph after lowering to B3
Summary: Drop the FTL(DFG) graph after lowering to B3
Status: REOPENED
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-06-02 15:40 PDT by Robin Morisset
Modified: 2021-06-08 17:59 PDT (History)
9 users (show)

See Also:


Attachments
Patch (68.17 KB, patch)
2021-06-02 15:56 PDT, Robin Morisset
fpizlo: review+
mark.lam: commit-queue-
Details | Formatted Diff | Diff
Patch (68.16 KB, patch)
2021-06-02 18:05 PDT, Robin Morisset
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (73.52 KB, patch)
2021-06-03 16:20 PDT, Robin Morisset
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (74.79 KB, patch)
2021-06-04 00:43 PDT, Robin Morisset
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (74.80 KB, patch)
2021-06-04 08:35 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff
Patch for re-landing (75.03 KB, patch)
2021-06-08 15:03 PDT, Robin Morisset
ews-feeder: commit-queue-
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-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