Bug 163657

Summary: [DOMJIT] Add a way for DOMJIT::Patchpoint to express effects
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: JavaScriptCoreAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, fpizlo, ryanhaddad, sbarati
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 162544, 162980    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
sbarati: review+
Patch
none
Patch none

Description Yusuke Suzuki 2016-10-19 01:05:57 PDT
Let's start with a very simple one.
Comment 1 Yusuke Suzuki 2016-10-19 16:37:57 PDT
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.
Comment 2 Yusuke Suzuki 2016-10-21 01:30:09 PDT
Created attachment 292333 [details]
Patch

WIP
Comment 3 Yusuke Suzuki 2016-10-21 02:02:00 PDT
DOMJIT CallDOM ops get CSE-ed & LICM-ed...
Comment 4 Yusuke Suzuki 2016-10-21 14:25:58 PDT
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.
Comment 5 Yusuke Suzuki 2016-10-21 21:02:29 PDT
Created attachment 292457 [details]
Patch

WIP
Comment 6 Yusuke Suzuki 2016-10-22 17:02:28 PDT
Created attachment 292521 [details]
Patch

WIP
Comment 7 Yusuke Suzuki 2016-10-22 17:46:21 PDT
Created attachment 292522 [details]
Patch

WIP
Comment 8 WebKit Commit Bot 2016-10-22 17:47:42 PDT
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.
Comment 9 Yusuke Suzuki 2016-10-22 17:55:23 PDT
Created attachment 292523 [details]
Patch

WIP
Comment 10 Yusuke Suzuki 2016-10-22 19:30:15 PDT
Created attachment 292526 [details]
Patch

WIP
Comment 11 Yusuke Suzuki 2016-10-22 20:10:30 PDT
Created attachment 292529 [details]
Patch

I think it is almost completed. I need to add tests.
Comment 12 Yusuke Suzuki 2016-10-22 20:29:41 PDT
Created attachment 292530 [details]
Patch

Add dumps
Comment 13 Yusuke Suzuki 2016-10-22 21:53:37 PDT
Created attachment 292535 [details]
Patch

Fix bugs
Comment 14 Yusuke Suzuki 2016-10-22 21:56:07 PDT
Created attachment 292536 [details]
Patch

Fix bugs
Comment 15 Yusuke Suzuki 2016-10-22 23:03:40 PDT
Created attachment 292539 [details]
Patch

More fix
Comment 16 Yusuke Suzuki 2016-10-23 00:26:21 PDT
Created attachment 292541 [details]
Patch
Comment 17 Yusuke Suzuki 2016-10-23 00:30:01 PDT
OK, ready for reviews.
Comment 18 Yusuke Suzuki 2016-10-23 00:43:35 PDT
Created attachment 292542 [details]
Patch
Comment 19 Yusuke Suzuki 2016-10-23 00:52:05 PDT
Created attachment 292543 [details]
Patch

Add more tests
Comment 20 Saam Barati 2016-10-24 13:14:11 PDT
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 21 Yusuke Suzuki 2016-10-24 15:44:24 PDT
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 22 Filip Pizlo 2016-10-24 15:49:34 PDT
Comment on attachment 292543 [details]
Patch

LGTM too.
Comment 23 Saam Barati 2016-10-24 16:00:27 PDT
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 24 Yusuke Suzuki 2016-10-24 16:12:55 PDT
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()).
Comment 25 Yusuke Suzuki 2016-10-24 16:37:23 PDT
Committed r207787: <http://trac.webkit.org/changeset/207787>
Comment 26 Yusuke Suzuki 2016-10-24 17:15:52 PDT
VC++ build errors appear. Looking...
Comment 27 Yusuke Suzuki 2016-10-24 17:33:56 PDT
Committed r207793: <http://trac.webkit.org/changeset/207793>
Comment 28 Ryan Haddad 2016-10-24 17:48:18 PDT
** 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
Comment 29 Yusuke Suzuki 2016-10-24 17:49:02 PDT
(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.
Comment 30 Yusuke Suzuki 2016-10-24 18:21:35 PDT
(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.
Comment 31 Yusuke Suzuki 2016-10-24 18:28:42 PDT
(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 :)
Comment 32 Yusuke Suzuki 2016-10-24 18:49:38 PDT
Reopening to attach new patch.
Comment 33 Yusuke Suzuki 2016-10-24 18:49:39 PDT
Created attachment 292699 [details]
Patch

EWS checking for Windows bot
Comment 34 Yusuke Suzuki 2016-10-24 19:03:59 PDT
Created attachment 292701 [details]
Patch

EWS checking for Windows bot
Comment 35 Yusuke Suzuki 2016-10-24 19:58:16 PDT
Committed r207799: <http://trac.webkit.org/changeset/207799>
Comment 36 Yusuke Suzuki 2016-10-24 19:58:42 PDT
*** Bug 163931 has been marked as a duplicate of this bug. ***