Bug 163657 – [DOMJIT] Add a way for DOMJIT::Patchpoint to express effects

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Yusuke Suzuki]

Let's start with a very simple one.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 292333 [details]
Patch

WIP

[Yusuke Suzuki]

DOMJIT CallDOM ops get CSE-ed & LICM-ed...

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 292457 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 292521 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 292522 [details]
Patch

WIP

[WebKit Commit Bot]

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.

[Yusuke Suzuki]

Created attachment 292523 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 292526 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 292529 [details]
Patch

I think it is almost completed. I need to add tests.

[Yusuke Suzuki]

Created attachment 292530 [details]
Patch

Add dumps

[Yusuke Suzuki]

Created attachment 292535 [details]
Patch

Fix bugs

[Yusuke Suzuki]

Created attachment 292536 [details]
Patch

Fix bugs

[Yusuke Suzuki]

Created attachment 292539 [details]
Patch

More fix

[Yusuke Suzuki]

Created attachment 292541 [details]
Patch

[Yusuke Suzuki]

OK, ready for reviews.

[Yusuke Suzuki]

Created attachment 292542 [details]
Patch

[Yusuke Suzuki]

Created attachment 292543 [details]
Patch

Add more tests

[Saam Barati]

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.

[Yusuke Suzuki]

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.

[Filip Pizlo]

Comment on attachment 292543 [details]
Patch

LGTM too.

[Saam Barati]

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.

[Yusuke Suzuki]

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()).

[Yusuke Suzuki]

Committed r207787: <http://trac.webkit.org/changeset/207787>

[Yusuke Suzuki]

VC++ build errors appear. Looking...

[Yusuke Suzuki]

Committed r207793: <http://trac.webkit.org/changeset/207793>

[Ryan Haddad]

** 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

[Yusuke Suzuki]

(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.

[Yusuke Suzuki]

(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.

[Yusuke Suzuki]

(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 :)

[Yusuke Suzuki]

Reopening to attach new patch.

[Yusuke Suzuki]

Created attachment 292699 [details]
Patch

EWS checking for Windows bot

[Yusuke Suzuki]

Created attachment 292701 [details]
Patch

EWS checking for Windows bot

[Yusuke Suzuki]

Committed r207799: <http://trac.webkit.org/changeset/207799>

[Yusuke Suzuki]

*** Bug 163931 has been marked as a duplicate of this bug. ***