Bug 202832 – Split ArithProfile into a Unary and a Binary version

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

[Robin Morisset]

ArithProfile was for a long time only used for add/sub/mul/div, but recently it started being used for negate. And it will soon also have to be used for inc and dec due to BigInt.
So I'd like to make a separate version that only has the data for a single argument, and thus takes half as much memory.

[Robin Morisset]

Created attachment 380706 [details]
Patch

[Keith Miller]

Comment on attachment 380706 [details]
Patch

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

r=me with question

> Source/JavaScriptCore/ChangeLog:9
> +        ArithProfile was for a long time only used for add/sub/mul/div, but recently it started being used for negate. And it will soon also have to be used for inc and dec due to BigInt.
> +        So in this patch I make a separate version that only has the data for a single argument, and thus takes half as much memory.

Did you see an improvement in RAMification?

> Source/JavaScriptCore/bytecode/ArithProfile.cpp:158
> +void printInternal(PrintStream& out, const UnaryArithProfile& profile)
> +{
> +    const char* separator = "";
> +
> +    out.print("Result:<");
> +    if (!profile.didObserveNonInt32()) {
> +        out.print("Int32");
> +        separator = "|";
> +    } else {
> +        if (profile.didObserveNegZeroDouble()) {
> +            out.print(separator, "NegZeroDouble");
> +            separator = "|";
> +        }
> +        if (profile.didObserveNonNegZeroDouble()) {
> +            out.print(separator, "NonNegZeroDouble");
> +            separator = "|";
> +        }
> +        if (profile.didObserveNonNumeric()) {
> +            out.print(separator, "NonNumeric");
> +            separator = "|";
> +        }
> +        if (profile.didObserveInt32Overflow()) {
> +            out.print(separator, "Int32Overflow");
> +            separator = "|";
> +        }
> +        if (profile.didObserveInt52Overflow()) {
> +            out.print(separator, "Int52Overflow");
> +            separator = "|";
> +        }
> +        if (profile.didObserveBigInt()) {
> +            out.print(separator, "BigInt");
> +            separator = "|";
> +        }
> +    }
> +    out.print(">");
> +
> +    out.print(" Arg ObservedType:<");
> +    out.print(profile.argObservedType());
> +    out.print(">");
> +
> +    out.print(" Arg ResultType:<");
> +    out.print(RawPointer(bitwise_cast<void*>(static_cast<uintptr_t>(profile.argResultType().bits()))));
> +    out.print(">");
> +}
> +

Seems like you could template this? e.g.

template<typename BitField>
inline void printInternal(PrintStream& out, const ArithProfile<BitField>&) ...

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:950
> +            if (!arithProfile)
> +                break;

Is this ever true? It kinda seems like it from reading the code below but that's somewhat surprising.

[Robin Morisset]

(In reply to Keith Miller from comment #2)
> Comment on attachment 380706 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=380706&action=review
> 
> r=me with question
> 
> > Source/JavaScriptCore/ChangeLog:9
> > +        ArithProfile was for a long time only used for add/sub/mul/div, but recently it started being used for negate. And it will soon also have to be used for inc and dec due to BigInt.
> > +        So in this patch I make a separate version that only has the data for a single argument, and thus takes half as much memory.
> 
> Did you see an improvement in RAMification?

I've not looked, but it would be super surprising considering that UnaryArithProfile is only used for OpNegate currently. Saving 2 bytes per OpNegate seems fairly negligible. It should matter more once I add it to OpInc and OpDec.

> 
> > Source/JavaScriptCore/bytecode/ArithProfile.cpp:158
> > +void printInternal(PrintStream& out, const UnaryArithProfile& profile)
> > +{
> > +    const char* separator = "";
> > +
> > +    out.print("Result:<");
> > +    if (!profile.didObserveNonInt32()) {
> > +        out.print("Int32");
> > +        separator = "|";
> > +    } else {
> > +        if (profile.didObserveNegZeroDouble()) {
> > +            out.print(separator, "NegZeroDouble");
> > +            separator = "|";
> > +        }
> > +        if (profile.didObserveNonNegZeroDouble()) {
> > +            out.print(separator, "NonNegZeroDouble");
> > +            separator = "|";
> > +        }
> > +        if (profile.didObserveNonNumeric()) {
> > +            out.print(separator, "NonNumeric");
> > +            separator = "|";
> > +        }
> > +        if (profile.didObserveInt32Overflow()) {
> > +            out.print(separator, "Int32Overflow");
> > +            separator = "|";
> > +        }
> > +        if (profile.didObserveInt52Overflow()) {
> > +            out.print(separator, "Int52Overflow");
> > +            separator = "|";
> > +        }
> > +        if (profile.didObserveBigInt()) {
> > +            out.print(separator, "BigInt");
> > +            separator = "|";
> > +        }
> > +    }
> > +    out.print(">");
> > +
> > +    out.print(" Arg ObservedType:<");
> > +    out.print(profile.argObservedType());
> > +    out.print(">");
> > +
> > +    out.print(" Arg ResultType:<");
> > +    out.print(RawPointer(bitwise_cast<void*>(static_cast<uintptr_t>(profile.argResultType().bits()))));
> > +    out.print(">");
> > +}
> > +
> 
> Seems like you could template this? e.g.
> 
> template<typename BitField>
> inline void printInternal(PrintStream& out, const ArithProfile<BitField>&)
> ...

I could probably factor some of the code, but I like having the different names LHS+RHS vs Arg. So the ObservedResult part is pretty much the only part which can be easily shared.

> 
> > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:950
> > +            if (!arithProfile)
> > +                break;
> 
> Is this ever true? It kinda seems like it from reading the code below but
> that's somewhat surprising.

I suspect one case where it can happen is if the ArithAdd comes from an OpInc. There may be some other cases.

[WebKit Commit Bot]

Comment on attachment 380706 [details]
Patch

Clearing flags on attachment: 380706

Committed r251090: <https://trac.webkit.org/changeset/251090>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[John Wilander]

This patch assert crashes on debug builds.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 202959

[Radar WebKit Bug Importer]

<rdar://problem/56266847>

[Radar WebKit Bug Importer]

<rdar://problem/56266848>

[Robin Morisset]

Created attachment 381001 [details]
Patch

I'll be careful to wait for a green light from EWS before trying to land the patch this time.

[EWS Watchlist]

Comment on attachment 381001 [details]
Patch

Attachment 381001 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/13135424

New failing tests:
mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-llint

[Robin Morisset]

Created attachment 381024 [details]
Patch

Trying again to check if the failing tests were related or not.

[Robin Morisset]

I checked and this patch has no significant effect on RAMification.

Also I found a bug in it: I shrunk ArithProfile, but did not change the accesses to it in LowLevelAssembly{64/32_64}.asm.

[Robin Morisset]

Created attachment 381463 [details]
Patch

Adding "orh" and using it instead of "ori" since UnaryArithProfile and BinaryArithProfile are now 16-bits.

[Robin Morisset]

Created attachment 381626 [details]
Patch

rebased patch (it had some minor conflicts with Yusuke's recent patch).

[Robin Morisset]

Created attachment 381763 [details]
Patch

rebasing, and removing a bit of dead code.

[Robin Morisset]

Created attachment 382248 [details]
Patch

rebased

[Robin Morisset]

Created attachment 382253 [details]
Patch

Fix iOS build

[Robin Morisset]

Created attachment 382337 [details]
Patch

Fix another problem with orh on ARM.

[Robin Morisset]

Created attachment 382346 [details]
Patch

Fix yet another problem with the offlineasm on ARM.

[Robin Morisset]

Created attachment 382349 [details]
Patch

Copying my attempted fix from arm.rb to arm64.rb

[Robin Morisset]

Comment on attachment 382349 [details]
Patch

Still hasn't fixed the problem..

[Robin Morisset]

The problem came from https://bugs.webkit.org/show_bug.cgi?id=203752, I will rebase the patch and send it here for review once that bug is fixed.

[Robin Morisset]

Created attachment 382693 [details]
Patch

It should build on ARM64, now that the bug with immediates and logical instructions has been fixed.

[EWS Watchlist]

Comment on attachment 382693 [details]
Patch

Attachment 382693 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/13206739

New failing tests:
mozilla-tests.yaml/ecma/Date/15.9.5.14.js.mozilla-dfg-eager-no-cjit-validate-phases
mozilla-tests.yaml/ecma/Date/15.9.5.34-1.js.mozilla-dfg-eager-no-cjit-validate-phases
mozilla-tests.yaml/ecma/Date/15.9.5.14.js.mozilla-baseline
mozilla-tests.yaml/ecma/Date/15.9.5.34-1.js.mozilla-no-ftl
mozilla-tests.yaml/ecma/Date/15.9.5.14.js.mozilla-no-ftl
mozilla-tests.yaml/ecma/Date/15.9.5.34-1.js.mozilla-ftl-eager-no-cjit-validate-phases
mozilla-tests.yaml/ecma/Date/15.9.5.34-1.js.mozilla-baseline
mozilla-tests.yaml/ecma/Date/15.9.5.34-1.js.mozilla
mozilla-tests.yaml/ecma/Date/15.9.5.14.js.mozilla-llint
mozilla-tests.yaml/ecma/Date/15.9.5.14.js.mozilla-ftl-eager-no-cjit-validate-phases
mozilla-tests.yaml/ecma/Date/15.9.5.34-1.js.mozilla-llint
mozilla-tests.yaml/ecma/Date/15.9.5.14.js.mozilla

[Keith Miller]

Comment on attachment 382693 [details]
Patch

r=me.

[Robin Morisset]

Comment on attachment 382693 [details]
Patch

The bots failure related to Date are not caused by my patch, the bots just disliked the DST change somehow. So I am trying to land this.

[WebKit Commit Bot]

Comment on attachment 382693 [details]
Patch

Clearing flags on attachment: 382693

Committed r252015: <https://trac.webkit.org/changeset/252015>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Truitt Savell]

The changes in https://trac.webkit.org/changeset/252015/webkit

has broken the windows build:
https://build.webkit.org/builders/Apple%20Win%2010%20Release%20%28Build%29/builds/8096

[Truitt Savell]

EWS did catch this build failure

[Truitt Savell]

Reverted r252015 for reason:

Broke the Windows build

Committed r252021: <https://trac.webkit.org/changeset/252021>

[Robin Morisset]

Created attachment 383078 [details]
Patch

fixed the cloop problem, will land again once all EWS bots are green.

[Robin Morisset]

Created attachment 383100 [details]
Patch

Fixing another cloop issue

[Robin Morisset]

Comment on attachment 383100 [details]
Patch

I checked the cloop build locally and it appears finally truly fixed. So trying to land this once again (not waiting for the windows bot because it appears hopelessly slow with 74 patches in wait).

[WebKit Commit Bot]

Comment on attachment 383100 [details]
Patch

Clearing flags on attachment: 383100

Committed r252229: <https://trac.webkit.org/changeset/252229>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Matt Lewis]

Reverted r252229 for reason:

This caused internal failures.

Committed r252273: <https://trac.webkit.org/changeset/252273>

[Robin Morisset]

Created attachment 383377 [details]
Patch

The problem that caused the roll-out was diagnosed by Mark, I still had some or32 left around in the ICs code, and when they were used on the now 16-bits ArithProfiles it could lead to metadata corruption.

[Robin Morisset]

Created attachment 383379 [details]
diff

Diff between last version of the patch (reviewed by Keith) and this one.

[Robin Morisset]

Comment on attachment 383377 [details]
Patch

Still visibly buggy on ARM

[Robin Morisset]

Created attachment 383382 [details]
Patch

Added the right load16/store16 to the ARM backends.

[Robin Morisset]

Comment on attachment 383382 [details]
Patch

Still failing on ARM..

[Robin Morisset]

Created attachment 383383 [details]
Patch

Just another stupid typo in MacroAssemblerARM64.h

[Robin Morisset]

Comment on attachment 383383 [details]
Patch

This patch breaks the MIPS port, I filed https://bugs.webkit.org/show_bug.cgi?id=204126 about it.
The windows bot is currently orange, as neither my patch nor ToT pass tests. Since jsc and Mac-debug-wk1 were in the same situation and have since turned green I suspect this was just a temporary breakage on ToT.

[WebKit Commit Bot]

Comment on attachment 383383 [details]
Patch

Rejecting attachment 383383 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 383383, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in JSTests/ChangeLog contains OOPS!.

Full output: https://webkit-queues.webkit.org/results/13247583

[Robin Morisset]

Created attachment 383473 [details]
Patch

Just fixed the Changelog.

[Caio Lima]

(In reply to Robin Morisset from comment #47)
> Created attachment 383473 [details]
> Patch
> 
> Just fixed the Changelog.

Hi Robin. I have a diff to fix the issue into MIPS. Don't know if it is too late to include this. I uploaded it into https://bugs.webkit.org/attachment.cgi?id=383476&action=review

[WebKit Commit Bot]

Comment on attachment 383473 [details]
Patch

Clearing flags on attachment: 383473

Committed r252422: <https://trac.webkit.org/changeset/252422>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Robin Morisset]

(In reply to Caio Lima from comment #48)
> (In reply to Robin Morisset from comment #47)
> > Created attachment 383473 [details]
> > Patch
> > 
> > Just fixed the Changelog.
> 
> Hi Robin. I have a diff to fix the issue into MIPS. Don't know if it is too
> late to include this. I uploaded it into
> https://bugs.webkit.org/attachment.cgi?id=383476&action=review

Hi Caio,

Your patch does not apply cleanly on ToT. Feel free to rebase it, and I'll review it quickly so you can land it.

[Yusuke Suzuki]

Committed r254395: <https://trac.webkit.org/changeset/254395>

[Yusuke Suzuki]

Committed r254396: <https://trac.webkit.org/changeset/254396>