Bug 172260 – [DOMJIT] Move DOMJIT patchpoint infrastructure out of domjit

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Yusuke Suzuki]

Since it well generalize patchpoint over all the JIT tiers, I think it is useful for non DOMJIT use cases.
It is somewhat well defined compiler injection to JSC.

[Yusuke Suzuki]

Created attachment 310647 [details]
Patch

[Build Bot]

Comment on attachment 310647 [details]
Patch

Attachment 310647 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3776288

New failing tests:
imported/w3c/web-platform-tests/media-source/mediasource-buffered.html

[Build Bot]

Created attachment 310651 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 310647 [details]
Patch

Attachment 310647 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3777010

New failing tests:
media/W3C/video/currentSrc/currentSrc_nonempty_after_setting_src.html

[Build Bot]

Created attachment 310662 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Yusuke Suzuki]

Created attachment 310769 [details]
Patch

[Saam Barati]

Comment on attachment 310769 [details]
Patch

I think just choosing the name Patchpoint might be confusing w.r.t B3’s patchpoint. I don’t have a suggestion off the top of my head but I’ll think about it.

[Yusuke Suzuki]

(In reply to Saam Barati from comment #7)
> Comment on attachment 310769 [details]
> Patch
> 
> I think just choosing the name Patchpoint might be confusing w.r.t B3’s
> patchpoint. I don’t have a suggestion off the top of my head but I’ll think
> about it.

How about "InjectableCodeGenerator" / "InjectableCompiler" / "PatchpointCompiler" / "PatchpointCodeGenerator"?

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #8)
> (In reply to Saam Barati from comment #7)
> > Comment on attachment 310769 [details]
> > Patch
> > 
> > I think just choosing the name Patchpoint might be confusing w.r.t B3’s
> > patchpoint. I don’t have a suggestion off the top of my head but I’ll think
> > about it.
> 
> How about "InjectableCodeGenerator" / "InjectableCompiler" /
> "PatchpointCompiler" / "PatchpointCodeGenerator"?

Ping?

[Filip Pizlo]

Comment on attachment 310769 [details]
Patch

I like the idea of putting this functionality at the top-level and dissociating it from "DOM".

I'm just not sure I like spreading the use of the name "patchpoint".  It has a pretty specific meaning in B3, which is subtly different from this.

The term for this that I've seen used elsewhere is "snippet".  The patchpoint is providing a snippet, and the "patchpoint generator" is really a "snippet compiler".

The benefit of renaming DOMPatchpoint to Snippet and DOMPatchpointGenerator to SnippetCompiler is threefold:

- It completely avoids any name collisions with B3.

- It avoids people expecting B3's patchpoints and these things to behave exactly the same.  They are similar but not identical and they are allowed to diverge from each other.

- I think that "snippet" is the better term.  If I had a chance to do B3 over again, I would have used "snippet".

RS=me to move this to the top level.  I'm not completely opposed to continuing to use the patchpoint name for this functionality, but I think that calling it snippet would be nicer.

[Filip Pizlo]

(In reply to Yusuke Suzuki from comment #9)
> (In reply to Yusuke Suzuki from comment #8)
> > (In reply to Saam Barati from comment #7)
> > > Comment on attachment 310769 [details]
> > > Patch
> > > 
> > > I think just choosing the name Patchpoint might be confusing w.r.t B3’s
> > > patchpoint. I don’t have a suggestion off the top of my head but I’ll think
> > > about it.
> > 
> > How about "InjectableCodeGenerator" / "InjectableCompiler" /
> > "PatchpointCompiler" / "PatchpointCodeGenerator"?
> 
> Ping?

Ha!  I didn't see this thread before clicking "Review Patch".

What do you think of Snippet/SnippetCompiler?  It's so concise!

[Saam Barati]

+1 for Snippet/SnippetCompiler

[Yusuke Suzuki]

Sounds super nice!
I'll rename to Snippet and SnippetCompiler :D

[Yusuke Suzuki]

Created attachment 311421 [details]
Patch

Patch for landing

[Filip Pizlo]

Comment on attachment 311421 [details]
Patch

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

lgtm.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:10270
> +                FTLSnippetParams domJITParams(*state, params, node, nullptr, WTFMove(regs), WTFMove(gpScratch), WTFMove(fpScratch));

I wonder if FTLLowerDFGToB3 could just include FTLSnippetParams, which means you could just say SnippetParams here, and you'd get the FTL one.  We do this game with FTL::JITCode (which inherits from JSC::JITCode).  I think it's mostly not confusing, but that's just me.  I like being terse.  But maybe it's confusing to others.  I don't feel strongly about this.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:10367
> +                Vector<SnippetParams::Value> regs;

If I'm right, you'd still say SnippetParams here - you'd be referring to FTL::SnippetParams, but that publicly extends JSC::SnippetParams, so this just works.

> Source/JavaScriptCore/ftl/FTLSnippetParams.h:38
> +class FTLSnippetParams : public SnippetParams {

What about "class SnippetParams : public JSC::SnippetParams"?

This would mirror how JITCode works, for example.  I would only want us to do this if it doesn't cause us to have to say JSC::SnippetParams everywhere.  OTOH I think it's OK to say FTL::SnippetParams instead of FTLSnippetParams.

[Yusuke Suzuki]

Comment on attachment 311421 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:10270
>> +                FTLSnippetParams domJITParams(*state, params, node, nullptr, WTFMove(regs), WTFMove(gpScratch), WTFMove(fpScratch));
> 
> I wonder if FTLLowerDFGToB3 could just include FTLSnippetParams, which means you could just say SnippetParams here, and you'd get the FTL one.  We do this game with FTL::JITCode (which inherits from JSC::JITCode).  I think it's mostly not confusing, but that's just me.  I like being terse.  But maybe it's confusing to others.  I don't feel strongly about this.

Agreed. That's good call. This leads to unconfusing code, like, what is the difference between SnippetParams and FTLSnippetParams :)
Fixed.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:10367
>> +                Vector<SnippetParams::Value> regs;
> 
> If I'm right, you'd still say SnippetParams here - you'd be referring to FTL::SnippetParams, but that publicly extends JSC::SnippetParams, so this just works.

Yup right. I don't think this SnippetParams will be extended in different tiers because Snippet's code generator need to receive SnippetParams::Value vector for all the tiers, but renaming FTLSnippetParams to FTL::SnippetParams leads to more readable code. We don't need to confuse the difference between FTLSnippetParams and JSC::SnippetParams :).

>> Source/JavaScriptCore/ftl/FTLSnippetParams.h:38
>> +class FTLSnippetParams : public SnippetParams {
> 
> What about "class SnippetParams : public JSC::SnippetParams"?
> 
> This would mirror how JITCode works, for example.  I would only want us to do this if it doesn't cause us to have to say JSC::SnippetParams everywhere.  OTOH I think it's OK to say FTL::SnippetParams instead of FTLSnippetParams.

Good call. While we need to keep the name AccessCaseSnippetParams (Since it is used in all the tiers' IC and IC does not have specific namespace.), we can rename FTLSnippetParams to FTL::SnippetParams. And we can also rename DFGSnippetParams to DFG::SnippetParams. Fixed.

[Yusuke Suzuki]

Committed r217523: <http://trac.webkit.org/changeset/217523>

[Radar WebKit Bug Importer]

<rdar://problem/32479824>