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.
Created attachment 380706 [details] Patch
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.
(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.
Comment on attachment 380706 [details] Patch Clearing flags on attachment: 380706 Committed r251090: <https://trac.webkit.org/changeset/251090>
All reviewed patches have been landed. Closing bug.
This patch assert crashes on debug builds.
Re-opened since this is blocked by bug 202959
<rdar://problem/56266847>
<rdar://problem/56266848>
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.
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
Created attachment 381024 [details] Patch Trying again to check if the failing tests were related or not.
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.
Created attachment 381463 [details] Patch Adding "orh" and using it instead of "ori" since UnaryArithProfile and BinaryArithProfile are now 16-bits.
Created attachment 381626 [details] Patch rebased patch (it had some minor conflicts with Yusuke's recent patch).
Created attachment 381763 [details] Patch rebasing, and removing a bit of dead code.
Created attachment 382248 [details] Patch rebased
Created attachment 382253 [details] Patch Fix iOS build
Created attachment 382337 [details] Patch Fix another problem with orh on ARM.
Created attachment 382346 [details] Patch Fix yet another problem with the offlineasm on ARM.
Created attachment 382349 [details] Patch Copying my attempted fix from arm.rb to arm64.rb
Comment on attachment 382349 [details] Patch Still hasn't fixed the problem..
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.
Created attachment 382693 [details] Patch It should build on ARM64, now that the bug with immediates and logical instructions has been fixed.
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
Comment on attachment 382693 [details] Patch r=me.
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.
Comment on attachment 382693 [details] Patch Clearing flags on attachment: 382693 Committed r252015: <https://trac.webkit.org/changeset/252015>
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
EWS did catch this build failure
Reverted r252015 for reason: Broke the Windows build Committed r252021: <https://trac.webkit.org/changeset/252021>
Created attachment 383078 [details] Patch fixed the cloop problem, will land again once all EWS bots are green.
Created attachment 383100 [details] Patch Fixing another cloop issue
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).
Comment on attachment 383100 [details] Patch Clearing flags on attachment: 383100 Committed r252229: <https://trac.webkit.org/changeset/252229>
Reverted r252229 for reason: This caused internal failures. Committed r252273: <https://trac.webkit.org/changeset/252273>
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.
Created attachment 383379 [details] diff Diff between last version of the patch (reviewed by Keith) and this one.
Comment on attachment 383377 [details] Patch Still visibly buggy on ARM
Created attachment 383382 [details] Patch Added the right load16/store16 to the ARM backends.
Comment on attachment 383382 [details] Patch Still failing on ARM..
Created attachment 383383 [details] Patch Just another stupid typo in MacroAssemblerARM64.h
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.
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
Created attachment 383473 [details] Patch Just fixed the Changelog.
(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
Comment on attachment 383473 [details] Patch Clearing flags on attachment: 383473 Committed r252422: <https://trac.webkit.org/changeset/252422>
(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.
Committed r254395: <https://trac.webkit.org/changeset/254395>
Committed r254396: <https://trac.webkit.org/changeset/254396>