[DOMJIT] Add initial CheckDOM and CallDOM implementations
Created attachment 290649 [details] Patch WIP
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.
Created attachment 290674 [details] Patch WIP
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.
Created attachment 290678 [details] Patch
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.
Comment on attachment 290678 [details] Patch Fix build.
Created attachment 290680 [details] Patch
Created attachment 290681 [details] Patch
Comment on attachment 290681 [details] Patch Oops. Still fixing.
Created attachment 290682 [details] Patch
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.
Created attachment 290683 [details] Patch
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.
Comment on attachment 290683 [details] Patch Still fixing.......
Created attachment 290691 [details] Patch
Rebasing
Created attachment 290692 [details] Patch
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.
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).
cool!
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.
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
Committed r206846: <http://trac.webkit.org/changeset/206846>
Committed r206847: <http://trac.webkit.org/changeset/206847>
Committed r206848: <http://trac.webkit.org/changeset/206848>