Bug 68328 – The generator and intrinsic fields in HashTableValue/HashEntry and associated structures and methods are redundant

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Filip Pizlo]

The old JIT optimizes intrinsics with ThunkGenerators, which are functions that when called generate JIT code that implements the intrinsic.  The DFG JIT optimizes intrinsics by weaving them through the optimizing compiler, and only exposes an enum (DFG::Intrinsic) for declaring whether or not an ExecutableBase is intrinsic, and if so, which intrinsic it is.

These two mechanism require some redundancy in runtime/Lookup.h, and various scripts for creating JS bindings.

A good solution would be to have one enum (say, in jit/JITIntrinsic.h).  The DFG would simply switch to using this intrinsic, while the old JIT would abandon passing function pointers and simply select the thunk generator by switching on the intrinsic enum.

[Sam Weinig]

If you are going to make this change, it might also be a good opportunity to remove the special casing from the generation script and put it in the lookup definitions.

[Gavin Barraclough]

It may even be that we don't want the enum to be in either JIT - we may with to make use of the intrinsic from the interpreter, in environments where we don't have the JIT enabled.  Maybe an enum in JSFunction?

[Filip Pizlo]

(In reply to comment #2)
> It may even be that we don't want the enum to be in either JIT - we may with to make use of the intrinsic from the interpreter, in environments where we don't have the JIT enabled.  Maybe an enum in JSFunction?

How about runtime/Intrinsic.h?

The benefit of a separate file is that:

1) JSFunction and pretty much everything else in runtime/ is cluttered.

2) If we do our jobs correctly then this enum will get big. ;-)

[Gavin Barraclough]

Agreed, that sounds even better.

[Gavin Barraclough]

Created attachment 117999 [details]
Fix

[Geoffrey Garen]

Comment on attachment 117999 [details]
Fix

r=me

[Early Warning System Bot]

Comment on attachment 117999 [details]
Fix

Attachment 117999 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10690698

[Gyuyoung Kim]

Comment on attachment 117999 [details]
Fix

Attachment 117999 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10728902

[Gustavo Noronha (kov)]

Comment on attachment 117999 [details]
Fix

Attachment 117999 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10729969

[Sam Weinig]

Comment on attachment 117999 [details]
Fix

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

> Source/JavaScriptCore/create_hash_table:276
>          if ($key eq "charCodeAt") {
>              $thunkGenerator = "charCodeAtThunkGenerator";
> -            $intrinsic = "DFG::CharCodeAtIntrinsic";
> +            $intrinsic = "CharCodeAtIntrinsic";

Do we really need this specialized check? Can't we just write CharCodeAtIntrinsic at the definition site?

> Source/JavaScriptCore/create_hash_table:323
>                  $thunkGenerator = "logThunkGenerator";
> +                $intrinsic = "LogIntrinsic";

Is $thunkGenerator still being used?

[Gavin Barraclough]

(In reply to comment #10)
> (From update of attachment 117999 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117999&action=review
> 
> > Source/JavaScriptCore/create_hash_table:276
> >          if ($key eq "charCodeAt") {
> >              $thunkGenerator = "charCodeAtThunkGenerator";
> > -            $intrinsic = "DFG::CharCodeAtIntrinsic";
> > +            $intrinsic = "CharCodeAtIntrinsic";
> 
> Do we really need this specialized check? Can't we just write CharCodeAtIntrinsic at the definition site?

I definitely agree with you that we should do this, but I don't think it's a part of this change.  This patch changes the data in the table output by the create_hash_table script, you're proposing changing the data in the tables parsed in by create_hash_table, these two changes are completely independent of each other.
 
> > Source/JavaScriptCore/create_hash_table:323
> >                  $thunkGenerator = "logThunkGenerator";
> > +                $intrinsic = "LogIntrinsic";
> 
> Is $thunkGenerator still being used?

Ooops - meant to remove them - thanks! - will do.

[Gavin Barraclough]

Fixed in r102167