Bug 175738 - Make GetDynamicVar propagate heap predictions instead of saying HeapTop
Summary: Make GetDynamicVar propagate heap predictions instead of saying HeapTop
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Enhancement
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-08-18 15:50 PDT by Robin Morisset
Modified: 2017-08-23 10:42 PDT (History)
7 users (show)

See Also:


Attachments
Patch, not yet benchmarked (4.92 KB, patch)
2017-08-18 16:02 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff
Patch, not yet benchmarked, nits fixed (5.02 KB, patch)
2017-08-18 16:43 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff
new Patch (6.05 KB, patch)
2017-08-21 02:44 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff
Patch with static (5.92 KB, patch)
2017-08-23 09:54 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 2017-08-18 15:50:33 PDT
In DFGPredictionPropagationPhase.cpp
Comment 1 Robin Morisset 2017-08-18 16:02:25 PDT
Created attachment 318553 [details]
Patch, not yet benchmarked
Comment 2 Saam Barati 2017-08-18 16:33:18 PDT
Comment on attachment 318553 [details]
Patch, not yet benchmarked

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

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4071
> +    ASSERT(sizeof(identifierNumber) == 4);

let's use static assert

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4072
> +    return identifierNumber | ((uint64_t) getPutInfo << 32);

style nit: we use static_cast in webkit instead of c-style casts
Comment 3 Robin Morisset 2017-08-18 16:43:47 PDT
Created attachment 318557 [details]
Patch, not yet benchmarked, nits fixed
Comment 4 Saam Barati 2017-08-21 00:34:09 PDT
Comment on attachment 318557 [details]
Patch, not yet benchmarked, nits fixed

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

The patch LGTM but I’ll hold off r+ing until it builds.

The one thing word adding is a micro benchmark just so you can verify it’s indeed faster.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4067
> +// Both GetDynamicVar and PutDynamicVar need to cram two 32-bit numbers into their

Style nit: This comment is unnecessary IMO. It’s not adding anything that’s not obvious from reading the code.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4071
> +    static_assert(sizeof(identifierNumber) == 4,

FWIW, this is probably a hard assumption when compiling WK. I’d bet many thing would break if this didn’t hold.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4073
> +    return identifierNumber | (static_cast<uint64_t>(getPutInfo) << 32);

That’s tricky! I guess Node::identifierNumber() just works?

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:5373
> +                uint64_t opInfo1 = makeDynamicVarOpInfo(identifierNumber, currentInstruction[4].u.operand);

Style nit: not sure either of these really need to be local variables
Comment 5 Robin Morisset 2017-08-21 02:44:58 PDT
Created attachment 318621 [details]
new Patch
Comment 6 Robin Morisset 2017-08-21 02:48:18 PDT
On the micro-benchmark I added, there is an improvement, but it is small: time to execute in release mode goes from 245ms to 235ms on my machine. Should I try to make a larger benchmark that makes the difference more visible, or was this level of improvement what was expected?

Node::identifierNumber() just works, since it reads the OpInfoWrapper union as an int32, so it should only load the low bits.

The bug on compilation on the build bots seemed to be caused by the function makeDynamicVarOpInfo not having its prototype declared separately. I don't see why that is a problem, so I just added the prototype next to the declaration.
Comment 7 Saam Barati 2017-08-21 09:44:50 PDT
Comment on attachment 318621 [details]
new Patch

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

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4068
> +uint64_t makeDynamicVarOpInfo(unsigned, unsigned);

I believe if you mark the function static this declaration isn’t needed. Or, you can always declare a lambda inside the function that switches on opcode
Comment 8 Robin Morisset 2017-08-23 09:54:44 PDT
Created attachment 318879 [details]
Patch with static
Comment 9 WebKit Commit Bot 2017-08-23 10:41:56 PDT
Comment on attachment 318879 [details]
Patch with static

Clearing flags on attachment: 318879

Committed r221084: <http://trac.webkit.org/changeset/221084>
Comment 10 WebKit Commit Bot 2017-08-23 10:41:58 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Radar WebKit Bug Importer 2017-08-23 10:42:29 PDT
<rdar://problem/34038306>