Bug 162941 – [DOMJIT] Add initial CheckDOM and CallDOM implementations

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[DOMJIT] Add initial CheckDOM and CallDOM implementations

[Yusuke Suzuki]

Created attachment 290649 [details]
Patch

WIP

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:46:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGNode.h:2458:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 35 files


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

[Yusuke Suzuki]

Created attachment 290674 [details]
Patch

WIP

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:46:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGNode.h:2458:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 37 files


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

[Yusuke Suzuki]

Created attachment 290678 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:46:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGNode.h:2458:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 38 files


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

[Yusuke Suzuki]

Comment on attachment 290678 [details]
Patch

Fix build.

[Yusuke Suzuki]

Created attachment 290680 [details]
Patch

[Yusuke Suzuki]

Created attachment 290681 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 290681 [details]
Patch

Oops. Still fixing.

[Yusuke Suzuki]

Created attachment 290682 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:46:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGNode.h:2458:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 37 files


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

[Yusuke Suzuki]

Created attachment 290683 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:46:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGNode.h:2458:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 37 files


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

[Yusuke Suzuki]

Comment on attachment 290683 [details]
Patch

Still fixing.......

[Yusuke Suzuki]

Created attachment 290691 [details]
Patch

[Yusuke Suzuki]

Rebasing

[Yusuke Suzuki]

Created attachment 290692 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:47:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:46:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGNode.h:2458:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 3 in 40 files


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

[Yusuke Suzuki]

Comment on attachment 290692 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:14
> +        JSNode::info().

Fan fact: This CheckDOM is almost always eliminated.
We typically write some DOM operation like,

node.firstChild

OR

document.getElementById("...");

At that time, for node.firstChild GetById access, we typically emit CheckStructure for node.
AI in this patch completely recognizes it and drop CheckDOM if filtered Structures are subclass of ClassInfo.

But still CheckDOM is useful if we would like to handle super polymorphic case (Dromaeo's firstChild use).

[Alex Christensen]

cool!

[Filip Pizlo]

Comment on attachment 290692 [details]
Patch

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

Nice!!

> Source/JavaScriptCore/b3/B3Effects.h:81
> +    static Effects forCheck()

This is great!

> Source/JavaScriptCore/b3/B3Value.cpp:617
> +        result = Effects::forCheck();

Nice.

> Source/JavaScriptCore/dfg/DFGAbstractValue.cpp:315
> +    m_structure.filterClassInfo(classInfo);

I like this. Can you file a bug about putting a ClassInfo field into AbstractValue. Then AI could do type-based tracking of value sets even better than now. When merging we would pick the lowest common ancestor. 

No need to do it now. Worth a fixme and bug.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2688
> +    addToGraph(CheckStructure, OpInfo(m_graph.addStructureSet(variant.structureSet())), thisNode);

This is a bit unfortunate. Is there any way to only have the CheckDOM and not the structure check?

> Source/JavaScriptCore/domjit/DOMJITReg.h:41
> +class Reg {

Crazy!  I think it's a good idea to have this separate from JSC::Reg/JSValueRegs for now but it sure seems like the rest of JSC will want this eventually. Can you file a bug and put a fixme for eventually moving this to JSC so others can use it?  For example B3::ValueRep does some of the same stuff, maybe it could delegate to your Reg class eventually.

[Yusuke Suzuki]

Comment on attachment 290692 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/dfg/DFGAbstractValue.cpp:315
>> +    m_structure.filterClassInfo(classInfo);
> 
> I like this. Can you file a bug about putting a ClassInfo field into AbstractValue. Then AI could do type-based tracking of value sets even better than now. When merging we would pick the lowest common ancestor. 
> 
> No need to do it now. Worth a fixme and bug.

Sure! I've just opened the bug for this. https://bugs.webkit.org/show_bug.cgi?id=162989
And noted FIXME here with this URL.

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2688
>> +    addToGraph(CheckStructure, OpInfo(m_graph.addStructureSet(variant.structureSet())), thisNode);
> 
> This is a bit unfortunate. Is there any way to only have the CheckDOM and not the structure check?

Yes! This mechanism should be fixed definitely.
This CheckStrucutre guards against Object.defineProperty(object, 'customGetter', { ... }) case.
But as you know, due to this check, we give up optimization in Dromaeo dom-traverse. This is because we encounter bunch of Structures here (HTMLDivElement, HTMLAnchorElement, etc.).
After initial DOMJIT accessor is implemented in WebCore, I'm planning to implement some mechanism to allow optimization here. It should involve AccessCases etc.

I discussed with rniwa and keith_miller before. At that time, we come up with one idea: putting bloom filter in each Structure that tells us this Structure *must not* have properties related to the given name.
I'll mail to you and Saam about the idea.

>> Source/JavaScriptCore/domjit/DOMJITReg.h:41
>> +class Reg {
> 
> Crazy!  I think it's a good idea to have this separate from JSC::Reg/JSValueRegs for now but it sure seems like the rest of JSC will want this eventually. Can you file a bug and put a fixme for eventually moving this to JSC so others can use it?  For example B3::ValueRep does some of the same stuff, maybe it could delegate to your Reg class eventually.

Yup! Filed. https://bugs.webkit.org/show_bug.cgi?id=162990

[Yusuke Suzuki]

Committed r206846: <http://trac.webkit.org/changeset/206846>

[Yusuke Suzuki]

Committed r206847: <http://trac.webkit.org/changeset/206847>

[Yusuke Suzuki]

Committed r206848: <http://trac.webkit.org/changeset/206848>