Let's start with a very simple one.
Ideally, we would like to express precise clobbering rules from DOMJIT code. But it seems too intrusive... So, let's start with a super simple one. DOMJIT offers preset of mutations. Like, Pure, NoSideEffect, SideEffect. Pure will reads World and def(). NoSideEffect reads World and write HeapObjectCount (Since it will create a DOM wrapper in many cases). And SideEffect reads World and write Heap! For example, nodeType => Pure firstChild => NoSideEffect appendChild() => SideEffect We note that checks are already separated from CallDOM. So we don't need to account the exceptions by Checks for CallDOM.
Created attachment 292333 [details] Patch WIP
DOMJIT CallDOM ops get CSE-ed & LICM-ed...
Discussed with Fil and Saam. We should have some hierarchy based abstract heap in DOMJIT side. This hierarchy will be encoded to the pair of integers. And these integers will be told to DFG and FTL. Then, DFG and FTL constructs DOMState heap with this integer. It can offer more detailed DOM abstract heap than just telling it as DOMState. And it offers more chance to perform LICM & CSE.
Created attachment 292457 [details] Patch WIP
Created attachment 292521 [details] Patch WIP
Created attachment 292522 [details] Patch WIP
Attachment 292522 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 32 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 292523 [details] Patch WIP
Created attachment 292526 [details] Patch WIP
Created attachment 292529 [details] Patch I think it is almost completed. I need to add tests.
Created attachment 292530 [details] Patch Add dumps
Created attachment 292535 [details] Patch Fix bugs
Created attachment 292536 [details] Patch Fix bugs
Created attachment 292539 [details] Patch More fix
Created attachment 292541 [details] Patch
OK, ready for reviews.
Created attachment 292542 [details] Patch
Created attachment 292543 [details] Patch Add more tests
Comment on attachment 292543 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=292543&action=review r=me with comments > Source/JavaScriptCore/ChangeLog:30 > + Told heap range is represented as DOMJIT::HeapRange. DFG will handle Told? > Source/JavaScriptCore/dfg/DFGAbstractHeap.h:271 > + if (kind() == DOMState && other.kind() == DOMState) { > + Payload currentPayload = payload(); > + Payload otherPayload = other.payload(); > + if (currentPayload.isTop() || otherPayload.isTop()) > + return true; > + return DOMJIT::HeapRange::fromRaw(currentPayload.value32()).overlaps(DOMJIT::HeapRange::fromRaw(otherPayload.value32())); > + } Why is this necessary given your above implementation? > Source/JavaScriptCore/dfg/DFGClobberSet.cpp:96 > + if (clobber.payload().isTop()) > + continue; Why not just return "true" here or skip this check altogether since the below check should handle it? > Source/JavaScriptCore/dfg/DFGClobberize.h:922 > + write(Heap); // We do not assume that DOM operations will write(Stack). I think this is what you mean by the comment: "We assume that DOM operations will not write(Stack)". Also, is this a valid assumption? What's guaranteeing this? It seems like if the dom operation calls arbitrary JS code, it's possible to write(Stack) via function.arguments or whatever. > Source/JavaScriptCore/domjit/DOMJITEffect.h:40 > + bool exitsSideways { false }; I'd defer having this field until later when it's used (or if it ever becomes used). > Source/JavaScriptCore/domjit/DOMJITHeapRange.cpp:37 > + Style: extra new line not needed > Source/JavaScriptCore/domjit/DOMJITHeapRange.h:42 > + { Please assert that end => being to protect against overflow. > Source/JavaScriptCore/domjit/DOMJITHeapRange.h:50 > + enum RawRepresentationTag { RawRepresentation }; > + explicit constexpr HeapRange(RawRepresentationTag, uint32_t value) > + : m_begin(static_cast<uint16_t>(value >> 16)) > + , m_end(static_cast<uint16_t>(value)) > + { > + } Nit: Seems maybe more complicated than needed. I would do this instead: *bitwise_cast<uint32_t*>(this) = value > Source/JavaScriptCore/domjit/DOMJITHeapRange.h:59 > + uint32_t rawRepresentation() { return (static_cast<uint32_t>(m_begin) << 16) | m_end; } And here just do: return bitwise_cast<uint32_t>(*this); And then have a static assert like: sizeof(HeapRange) == sizeof(uint32_t). Anyways, this isn't super important, but I think it's more straight forward.
Comment on attachment 292543 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=292543&action=review Thanks! >> Source/JavaScriptCore/ChangeLog:30 >> + Told heap range is represented as DOMJIT::HeapRange. DFG will handle > > Told? I should rephrase this as `The heap range told by the WebCore`. >> Source/JavaScriptCore/dfg/DFGAbstractHeap.h:271 >> + } > > Why is this necessary given your above implementation? Yup, it is not necessary. isStrictSubtypeOf should cover the case since we guarantee that DOMState heap range is a tree. Dropped. >> Source/JavaScriptCore/dfg/DFGClobberSet.cpp:96 >> + continue; > > Why not just return "true" here or skip this check altogether since the below check should handle it? Nice, yeah, we can return true here since we found the direct write of DOMState(TOP). Of course, we can handle this in the subsequent generic path (while ... path), but returning true here is a little bit efficient. Changed. >> Source/JavaScriptCore/dfg/DFGClobberize.h:922 >> + write(Heap); // We do not assume that DOM operations will write(Stack). > > I think this is what you mean by the comment: "We assume that DOM operations will not write(Stack)". > > Also, is this a valid assumption? What's guaranteeing this? It seems like if the dom operation calls arbitrary JS code, it's possible to write(Stack) via function.arguments or whatever. Yeah, at least, the current DOMJIT getter meets this requirements. But it is not safe. I'll change this to write(World) to make this safer for the default case. >> Source/JavaScriptCore/domjit/DOMJITEffect.h:40 >> + bool exitsSideways { false }; > > I'd defer having this field until later when it's used (or if it ever becomes used). Yeah, right. Dropped. >> Source/JavaScriptCore/domjit/DOMJITHeapRange.cpp:37 >> + > > Style: extra new line not needed Fixed. >> Source/JavaScriptCore/domjit/DOMJITHeapRange.h:42 >> + { > > Please assert that end => being to protect against overflow. Added. >> Source/JavaScriptCore/domjit/DOMJITHeapRange.h:50 >> + } > > Nit: Seems maybe more complicated than needed. I would do this instead: > *bitwise_cast<uint32_t*>(this) = value Interesting. I've changed. It should work in both little / big endianness environments since raw representation does not consider about the begin and end position. And if we do that, I think changing HeapRange like, union { struct { uint16_t m_begin; uint16_t m_end; }; uint32_t m_raw; }; is required to take care of alignment thingy. And defining the constructor like, m_raw = value; is simpler I think. Changed. >> Source/JavaScriptCore/domjit/DOMJITHeapRange.h:59 >> + uint32_t rawRepresentation() { return (static_cast<uint32_t>(m_begin) << 16) | m_end; } > > And here just do: > return bitwise_cast<uint32_t>(*this); > > And then have a static assert like: > sizeof(HeapRange) == sizeof(uint32_t). > > Anyways, this isn't super important, but I think it's more straight forward. Changed.
Comment on attachment 292543 [details] Patch LGTM too.
Comment on attachment 292543 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=292543&action=review >>> Source/JavaScriptCore/dfg/DFGClobberSet.cpp:96 >>> + continue; >> >> Why not just return "true" here or skip this check altogether since the below check should handle it? > > Nice, yeah, we can return true here since we found the direct write of DOMState(TOP). Of course, we can handle this in the subsequent generic path (while ... path), but returning true here is a little bit efficient. Changed. Wouldn't the `if (DOMJIT::HeapRange::fromRaw(clobber.payload().value32()).overlaps(range))` right below also be true? That's what I was thinking when I said drop the check.
Comment on attachment 292543 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=292543&action=review >>>> Source/JavaScriptCore/dfg/DFGClobberSet.cpp:96 >>>> + continue; >>> >>> Why not just return "true" here or skip this check altogether since the below check should handle it? >> >> Nice, yeah, we can return true here since we found the direct write of DOMState(TOP). Of course, we can handle this in the subsequent generic path (while ... path), but returning true here is a little bit efficient. Changed. > > Wouldn't the `if (DOMJIT::HeapRange::fromRaw(clobber.payload().value32()).overlaps(range))` right below also be true? That's what I was thinking when I said drop the check. No. If the payload is `Top`, this abstract heap does not hold any DOMJIT::HeapRange. This is consistent to the other AbstractHeap. For example, `supertype()` is defined as follows, if (payload().isTop()) { if (kind() == Stack) return World; return Heap; } return AbstractHeap(kind()); If the payload is not top, the supertype() is the same kind with the top(). The hierarchy is, Heap <- DOMState(TOP) <- DOMState(some heap range including DOMJIT::HeapRange::top()).
Committed r207787: <http://trac.webkit.org/changeset/207787>
VC++ build errors appear. Looking...
Committed r207793: <http://trac.webkit.org/changeset/207793>
** The following JSC stress test failures have been introduced: stress/domjit-exception-ic.js.ftl-eager-no-cjit stress/domjit-exception.js.ftl-eager stress/domjit-exception.js.ftl-eager-no-cjit stress/domjit-exception.js.ftl-no-cjit-no-inline-validate stress/domjit-exception.js.ftl-no-cjit-validate-sampling-profiler stress/domjit-getter-complex.js.ftl-eager-no-cjit stress/domjit-getter-complex.js.ftl-no-cjit-no-inline-validate stress/domjit-getter-complex.js.ftl-no-cjit-validate-sampling-profiler stress/domjit-getter-poly.js.ftl-eager-no-cjit stress/domjit-getter-poly.js.ftl-no-cjit-no-inline-validate stress/domjit-getter-poly.js.ftl-no-cjit-validate-sampling-profiler stress/domjit-getter-proto.js.ftl-eager stress/domjit-getter-proto.js.ftl-eager-no-cjit stress/domjit-getter-proto.js.ftl-no-cjit-no-inline-validate stress/domjit-getter-proto.js.ftl-no-cjit-validate-sampling-profiler stress/domjit-getter-super-poly.js.ftl-eager-no-cjit stress/domjit-getter.js.default stress/domjit-getter.js.ftl-eager-no-cjit stress/domjit-getter.js.ftl-no-cjit-no-inline-validate stress/domjit-getter.js.ftl-no-cjit-validate-sampling-profiler https://build.webkit.org/builders/Apple%20El%20Capitan%20Release%20JSC%20%28Tests%29/builds/9987
(In reply to comment #28) > ** The following JSC stress test failures have been introduced: > stress/domjit-exception-ic.js.ftl-eager-no-cjit > stress/domjit-exception.js.ftl-eager > stress/domjit-exception.js.ftl-eager-no-cjit > stress/domjit-exception.js.ftl-no-cjit-no-inline-validate > stress/domjit-exception.js.ftl-no-cjit-validate-sampling-profiler > stress/domjit-getter-complex.js.ftl-eager-no-cjit > stress/domjit-getter-complex.js.ftl-no-cjit-no-inline-validate > stress/domjit-getter-complex.js.ftl-no-cjit-validate-sampling-profiler > stress/domjit-getter-poly.js.ftl-eager-no-cjit > stress/domjit-getter-poly.js.ftl-no-cjit-no-inline-validate > stress/domjit-getter-poly.js.ftl-no-cjit-validate-sampling-profiler > stress/domjit-getter-proto.js.ftl-eager > stress/domjit-getter-proto.js.ftl-eager-no-cjit > stress/domjit-getter-proto.js.ftl-no-cjit-no-inline-validate > stress/domjit-getter-proto.js.ftl-no-cjit-validate-sampling-profiler > stress/domjit-getter-super-poly.js.ftl-eager-no-cjit > stress/domjit-getter.js.default > stress/domjit-getter.js.ftl-eager-no-cjit > stress/domjit-getter.js.ftl-no-cjit-no-inline-validate > stress/domjit-getter.js.ftl-no-cjit-validate-sampling-profiler > > https://build.webkit.org/builders/ > Apple%20El%20Capitan%20Release%20JSC%20%28Tests%29/builds/9987 Thanks. I'm now investigating and it should be fixed soon.
(In reply to comment #29) > (In reply to comment #28) > > ** The following JSC stress test failures have been introduced: > > stress/domjit-exception-ic.js.ftl-eager-no-cjit > > stress/domjit-exception.js.ftl-eager > > stress/domjit-exception.js.ftl-eager-no-cjit > > stress/domjit-exception.js.ftl-no-cjit-no-inline-validate > > stress/domjit-exception.js.ftl-no-cjit-validate-sampling-profiler > > stress/domjit-getter-complex.js.ftl-eager-no-cjit > > stress/domjit-getter-complex.js.ftl-no-cjit-no-inline-validate > > stress/domjit-getter-complex.js.ftl-no-cjit-validate-sampling-profiler > > stress/domjit-getter-poly.js.ftl-eager-no-cjit > > stress/domjit-getter-poly.js.ftl-no-cjit-no-inline-validate > > stress/domjit-getter-poly.js.ftl-no-cjit-validate-sampling-profiler > > stress/domjit-getter-proto.js.ftl-eager > > stress/domjit-getter-proto.js.ftl-eager-no-cjit > > stress/domjit-getter-proto.js.ftl-no-cjit-no-inline-validate > > stress/domjit-getter-proto.js.ftl-no-cjit-validate-sampling-profiler > > stress/domjit-getter-super-poly.js.ftl-eager-no-cjit > > stress/domjit-getter.js.default > > stress/domjit-getter.js.ftl-eager-no-cjit > > stress/domjit-getter.js.ftl-no-cjit-no-inline-validate > > stress/domjit-getter.js.ftl-no-cjit-validate-sampling-profiler > > > > https://build.webkit.org/builders/ > > Apple%20El%20Capitan%20Release%20JSC%20%28Tests%29/builds/9987 > > Thanks. I'm now investigating and it should be fixed soon. This issue is fixed by https://trac.webkit.org/changeset/207789. When defining write(World) / write(Stack), we need to correctly figure out the written stack positions. But after discussing with Saam and Filip, we found that this CallDOM never writes Stack. So, write(World) is changed to write(Heap). And the corresponding setup is not necessary any more.
(In reply to comment #30) > (In reply to comment #29) > > (In reply to comment #28) > > > ** The following JSC stress test failures have been introduced: > > > stress/domjit-exception-ic.js.ftl-eager-no-cjit > > > stress/domjit-exception.js.ftl-eager > > > stress/domjit-exception.js.ftl-eager-no-cjit > > > stress/domjit-exception.js.ftl-no-cjit-no-inline-validate > > > stress/domjit-exception.js.ftl-no-cjit-validate-sampling-profiler > > > stress/domjit-getter-complex.js.ftl-eager-no-cjit > > > stress/domjit-getter-complex.js.ftl-no-cjit-no-inline-validate > > > stress/domjit-getter-complex.js.ftl-no-cjit-validate-sampling-profiler > > > stress/domjit-getter-poly.js.ftl-eager-no-cjit > > > stress/domjit-getter-poly.js.ftl-no-cjit-no-inline-validate > > > stress/domjit-getter-poly.js.ftl-no-cjit-validate-sampling-profiler > > > stress/domjit-getter-proto.js.ftl-eager > > > stress/domjit-getter-proto.js.ftl-eager-no-cjit > > > stress/domjit-getter-proto.js.ftl-no-cjit-no-inline-validate > > > stress/domjit-getter-proto.js.ftl-no-cjit-validate-sampling-profiler > > > stress/domjit-getter-super-poly.js.ftl-eager-no-cjit > > > stress/domjit-getter.js.default > > > stress/domjit-getter.js.ftl-eager-no-cjit > > > stress/domjit-getter.js.ftl-no-cjit-no-inline-validate > > > stress/domjit-getter.js.ftl-no-cjit-validate-sampling-profiler > > > > > > https://build.webkit.org/builders/ > > > Apple%20El%20Capitan%20Release%20JSC%20%28Tests%29/builds/9987 > > > > Thanks. I'm now investigating and it should be fixed soon. > > This issue is fixed by https://trac.webkit.org/changeset/207789. > When defining write(World) / write(Stack), we need to correctly figure out > the written stack positions. > But after discussing with Saam and Filip, we found that this CallDOM never > writes Stack. > So, write(World) is changed to write(Heap). And the corresponding setup is > not necessary any more. OK, validated. Thanks :)
Reopening to attach new patch.
Created attachment 292699 [details] Patch EWS checking for Windows bot
Created attachment 292701 [details] Patch EWS checking for Windows bot
Committed r207799: <http://trac.webkit.org/changeset/207799>
*** Bug 163931 has been marked as a duplicate of this bug. ***