Bug 163005 – [DOMJIT] Implement Node accessors in DOMJIT

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Yusuke Suzuki]

Let's implement DOMJIT Patchpoints for

firstChild, lastChild, nextSibling, previousSibling, parentNode.

[Yusuke Suzuki]

Created attachment 290894 [details]
Patch

WIP: initial working prototype

[WebKit Commit Bot]

Attachment 290894 [details] did not pass style-queue:


ERROR: Source/WebCore/ForwardingHeaders/domjit/DOMJITPatchpoint.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/WebCore/ForwardingHeaders/domjit/DOMJITPatchpointParams.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
Total errors found: 2 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 290894 [details]
Patch

Attachment 290894 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2235633

New failing tests:
imported/w3c/web-platform-tests/html/the-xhtml-syntax/parsing-xhtml-documents/xhtml-mathml-dtd-entity-support.htm

[Build Bot]

Created attachment 290902 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 290894 [details]
Patch

Attachment 290894 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2235646

New failing tests:
imported/w3c/web-platform-tests/html/the-xhtml-syntax/parsing-xhtml-documents/xhtml-mathml-dtd-entity-support.htm

[Build Bot]

Created attachment 290903 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Yusuke Suzuki]

Created attachment 290907 [details]
Patch

[WebKit Commit Bot]

Attachment 290907 [details] did not pass style-queue:


ERROR: Source/WebCore/ForwardingHeaders/domjit/DOMJITPatchpoint.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/WebCore/ForwardingHeaders/domjit/DOMJITPatchpointParams.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
Total errors found: 2 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 290908 [details]
Patch

[Build Bot]

Comment on attachment 290908 [details]
Patch

Attachment 290908 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2236277

New failing tests:
imported/w3c/web-platform-tests/html/the-xhtml-syntax/parsing-xhtml-documents/xhtml-mathml-dtd-entity-support.htm

[Build Bot]

Created attachment 290914 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 290908 [details]
Patch

Attachment 290908 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2236285

New failing tests:
imported/w3c/web-platform-tests/html/the-xhtml-syntax/parsing-xhtml-documents/xhtml-mathml-dtd-entity-support.htm

[Build Bot]

Created attachment 290915 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Yusuke Suzuki]

Created attachment 290942 [details]
Patch

[Yusuke Suzuki]

Created attachment 290945 [details]
Patch

[Build Bot]

Comment on attachment 290945 [details]
Patch

Attachment 290945 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2239016

New failing tests:
imported/w3c/web-platform-tests/html/the-xhtml-syntax/parsing-xhtml-documents/xhtml-mathml-dtd-entity-support.htm

[Build Bot]

Created attachment 290954 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 290945 [details]
Patch

Attachment 290945 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2239060

New failing tests:
imported/w3c/web-platform-tests/html/the-xhtml-syntax/parsing-xhtml-documents/xhtml-mathml-dtd-entity-support.htm

[Build Bot]

Created attachment 290956 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Yusuke Suzuki]

Created attachment 290960 [details]
Patch

[Yusuke Suzuki]

Created attachment 290985 [details]
Patch

[Sam Weinig]

Cool!

[Filip Pizlo]

Comment on attachment 290985 [details]
Patch

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

> Source/JavaScriptCore/domjit/DOMJITPatchpoint.h:65
> +    bool useTagMaskRegister { true };
> +    bool useTagTypeNumberRegister { true };

Can we just make these always true and remove them?

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:8737
> +        if (domJIT->useTagMaskRegister)
> +            patchpoint->append(m_tagMask, ValueRep::lateReg(GPRInfo::tagMaskRegister));
> +        if (domJIT->useTagTypeNumberRegister)
> +            patchpoint->append(m_tagTypeNumber, ValueRep::lateReg(GPRInfo::tagTypeNumberRegister));

It's relatively cheap to do this, so we could just do it all of the time.  I think that's what the get_by_id/put_by_id patchpoints do, for example.  Heck I think we also do it for ValueAdd and friends when emitting them in snippet (i.e. patchpoint) mode.

Also, why are you making them late registers?  I think it's correct, but stronger than what we need.  If the result is Void then you don't have to worry about them interfering with result.  If the result is not Void and you are already using SomeEarlyRegister for the result, then the result is already going to interfere with early uses.  Also, early uses already interfere with scratches.  It's OK to keep the code this way, since it's telling a sound truth, but I'm curious if you needed it to solve some bug!

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:8784
> +        if (domJIT->useTagMaskRegister)
> +            patchpoint->append(m_tagMask, ValueRep::lateReg(GPRInfo::tagMaskRegister));
> +        if (domJIT->useTagTypeNumberRegister)
> +            patchpoint->append(m_tagTypeNumber, ValueRep::lateReg(GPRInfo::tagTypeNumberRegister));

Ditto.

> Source/WTF/wtf/StdLibExtras.h:71
> +#define CAST_OFFSET(from, to) (reinterpret_cast<uintptr_t>(static_cast<to>((reinterpret_cast<from>(0x4000)))) - 0x4000)

Nice.  That's so evil.

> Source/WebCore/domjit/DOMJITHelpers.h:42
> +inline CCallHelpers::Jump branchIfNotWorldIsNormal(CCallHelpers& jit, GPRReg globalObject)

I love this style.  I wish that we had written CCallHelpers and AssemblyHelpers this way.

[Yusuke Suzuki]

Comment on attachment 290985 [details]
Patch

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

>> Source/JavaScriptCore/domjit/DOMJITPatchpoint.h:65
>> +    bool useTagTypeNumberRegister { true };
> 
> Can we just make these always true and remove them?

OK. Dropped.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:8737
>> +            patchpoint->append(m_tagTypeNumber, ValueRep::lateReg(GPRInfo::tagTypeNumberRegister));
> 
> It's relatively cheap to do this, so we could just do it all of the time.  I think that's what the get_by_id/put_by_id patchpoints do, for example.  Heck I think we also do it for ValueAdd and friends when emitting them in snippet (i.e. patchpoint) mode.
> 
> Also, why are you making them late registers?  I think it's correct, but stronger than what we need.  If the result is Void then you don't have to worry about them interfering with result.  If the result is not Void and you are already using SomeEarlyRegister for the result, then the result is already going to interfere with early uses.  Also, early uses already interfere with scratches.  It's OK to keep the code this way, since it's telling a sound truth, but I'm curious if you needed it to solve some bug!

Right. resultConstraint change is used to make the patchpoint environment in FTL closer to one in DFG: the result reg interferes with early uses.
On the other hand, this change does not fix any bugs. The previous one just works.
And you're right. This patchpoint already said that the result is SomeEarlyRegister. So we can just use ValueRep::reg() and they interfere with the other early uses and the result.

Changed it to ValueRep::reg and always added this constraint.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:8784
>> +            patchpoint->append(m_tagTypeNumber, ValueRep::lateReg(GPRInfo::tagTypeNumberRegister));
> 
> Ditto.

Fixed.

>> Source/WebCore/domjit/DOMJITHelpers.h:42
>> +inline CCallHelpers::Jump branchIfNotWorldIsNormal(CCallHelpers& jit, GPRReg globalObject)
> 
> I love this style.  I wish that we had written CCallHelpers and AssemblyHelpers this way.

Yeah, this allows us to extend helpers easily in everywhere (including outside of JSC like WebCore)

[Yusuke Suzuki]

Created attachment 291136 [details]
Patch for landing

[Yusuke Suzuki]

Committed r207013: <http://trac.webkit.org/changeset/207013>