Bug 175738 – Make GetDynamicVar propagate heap predictions instead of saying HeapTop

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Robin Morisset]

In DFGPredictionPropagationPhase.cpp

[Robin Morisset]

Created attachment 318553 [details]
Patch, not yet benchmarked

[Saam Barati]

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

[Robin Morisset]

Created attachment 318557 [details]
Patch, not yet benchmarked, nits fixed

[Saam Barati]

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

[Robin Morisset]

Created attachment 318621 [details]
new Patch

[Robin Morisset]

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.

[Saam Barati]

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

[Robin Morisset]

Created attachment 318879 [details]
Patch with static

[WebKit Commit Bot]

Comment on attachment 318879 [details]
Patch with static

Clearing flags on attachment: 318879

Committed r221084: <http://trac.webkit.org/changeset/221084>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34038306>