Bug 211059 – [JSC] CallData/ConstructData should include CallType/ConstructType

• Component: New Bugs
• Status: RESOLVED FIXED

[Ross Kirsling]

[JSC] CallData/ConstructData struct should include CallType/CallData

[Ross Kirsling]

Created attachment 397642 [details]
Patch

[Darin Adler]

Comment on attachment 397642 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:12
> +        getCallData/getConstructData return a CallType/ConstructType and have a CallData/ConstructData out param,
> +        and then *both* of these are passed side-by-side to `call`/`construct`, which all seems a bit silly.
> +
> +        This patch adds a CallType/ConstructType field to CallData/ConstructData such that getCallData/getConstructData
> +        can do exactly what their names suggest and such that `call`/`construct` require one less overt parameter.

Thanks. Glad you considered my suggestion.

I will point out that "get" in WebKit coding style is supposed to be reserved for functions with out arguments, so a name change is called for here. But I don’t think "callData" and "constructData" are necessarily good names, because they sound like verb phrases. So it might take a little work to think of better names.

> Source/JavaScriptCore/runtime/CallData.h:43
>      Host,

Irritating inconsistency that we call it "Host" here and "native" in the union.

Might also want to move CallType inside the struct and name it CallData::Type.

> Source/JavaScriptCore/runtime/ConstructData.h:46
> -enum class ConstructType : unsigned {
> +enum class ConstructType : uint8_t {
>      None,
>      Host,
>      JS
>  };
>  
>  struct ConstructData {

Seems that CallData and ConstructData are not different. Maybe they were at some point. At this point, I don’t understand why we have a separate type for ConstructType and ConstructData.

> Source/JavaScriptCore/runtime/JSCJSValue.h:89
> -enum class CallType : unsigned;
> +enum class CallType : uint8_t;
>  struct CallData;
> -enum class ConstructType : unsigned;
> +enum class ConstructType : uint8_t;
>  struct ConstructData;

Seems unlikely that we need the forward declarations of CallType and ConstructType here any more. Seems unlikely they are used by code that doesn’t include the header that has the struct definitions in it.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:517
>      VM& vm, JSGlobalObject* globalObject, CallFrame* callFrame, JSString* string, JSValue searchValue, CallData& callData,

Doesn’t it seem like this should be const CallData&?

[Ross Kirsling]

(In reply to Darin Adler from comment #2)
> I will point out that "get" in WebKit coding style is supposed to be
> reserved for functions with out arguments, so a name change is called for
> here. But I don’t think "callData" and "constructData" are necessarily good
> names, because they sound like verb phrases. So it might take a little work
> to think of better names.

Oh whoops. Perhaps `makeCallData`? Or even `dataForCall`?

> > Source/JavaScriptCore/runtime/CallData.h:43
> >      Host,
> 
> Irritating inconsistency that we call it "Host" here and "native" in the
> union.

Hmm, true. Changing the union would be less visible, but then I think "native" is a clearer word than "host".

> Might also want to move CallType inside the struct and name it
> CallData::Type.

I thought this wasn't possible because there's a different usage of CallType in JSC but apparently it's a totally different enum:
https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/bytecode/CallLinkInfo.h#L46

So that ought to work after all!

> > Source/JavaScriptCore/runtime/ConstructData.h:46
> > -enum class ConstructType : unsigned {
> > +enum class ConstructType : uint8_t {
> >      None,
> >      Host,
> >      JS
> >  };
> >  
> >  struct ConstructData {
> 
> Seems that CallData and ConstructData are not different. Maybe they were at
> some point. At this point, I don’t understand why we have a separate type
> for ConstructType and ConstructData.

That's a really good point. Different functions are needed for these but not different structs.

[Ross Kirsling]

(In reply to Ross Kirsling from comment #3)
> (In reply to Darin Adler from comment #2)
> > I will point out that "get" in WebKit coding style is supposed to be
> > reserved for functions with out arguments, so a name change is called for
> > here. But I don’t think "callData" and "constructData" are necessarily good
> > names, because they sound like verb phrases. So it might take a little work
> > to think of better names.
> 
> Oh whoops. Perhaps `makeCallData`? Or even `dataForCall`?

Hmm, even given the convention though, I'm a bit apprehensive about suddenly changing this particular name given its pervasiveness -- in particular, OverridesGetCallData is a flag used for JSC Structures at large.

[Ross Kirsling]

Created attachment 397654 [details]
Patch for landing

[Ross Kirsling]

Created attachment 397659 [details]
Patch for landing

[EWS]

Committed r260744: <https://trac.webkit.org/changeset/260744>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 397659 [details].

[Radar WebKit Bug Importer]

<rdar://problem/62437437>

[Darin Adler]

(In reply to Ross Kirsling from comment #4)
> OverridesGetCallData is a flag used for JSC Structures

Yes, we'd rename that flag too.

[Saam Barati]

Comment on attachment 397659 [details]
Patch for landing

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

This is a nice refactoring

> Source/JavaScriptCore/ChangeLog:16
> +        - renames enum value Host to Native in alignment with CallData's union

Nit: Maybe should’ve done the renaming in the other direction. We use “JSC_HOST_CALL” everywhere for what this “Native” is referring to

[Mark Lam]

(In reply to Saam Barati from comment #10)
> Comment on attachment 397659 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=397659&action=review
> 
> This is a nice refactoring
> 
> > Source/JavaScriptCore/ChangeLog:16
> > +        - renames enum value Host to Native in alignment with CallData's union
> 
> Nit: Maybe should’ve done the renaming in the other direction. We use
> “JSC_HOST_CALL” everywhere for what this “Native” is referring to

Bike shedding here, but I came from a world where "Native" is the term of art for such a concept i.e. C/C++ code that is native to the machine.  Also, in Source/JavaScriptCore, ...

$ grep -rn Native * | grep -v ChangeLog | grep -v CMakeLists.txt | grep -v xcode | grep -v 'JavaScriptCore.order' | grep -v Sources.txt | grep -v NativeTraits | grep -v InjectedScriptHost | wc
     599    3718   73246

$ grep -rn Host * | grep -v ChangeLog | grep -v CMakeLists.txt | grep -v xcode | grep -v 'JavaScriptCore.order' | grep -v Sources.txt | grep -v NativeTraits | grep -v InjectedScriptHost | wc
     249    1267   25459

If you consider InjectedScriptHost a valid vote for using "Host" as well, then ...

$ grep -rn Host * | grep -v ChangeLog | grep -v CMakeLists.txt | grep -v xcode | grep -v 'JavaScriptCore.order' | grep -v Sources.txt | grep -v NativeTraits | wc
     426    2192   50769

No matter how you slice it, "Native" is more commonly used than "Host".  

I don't strongly object to making "Host" the name of choice.  I used to think of "Host" as the machine hosting the development code, but I understand that in some worlds (e.g. gcc), "host" means the machine hosting the emulated code.

Regardless, I think we all agree on one thing: let's be consistent everywhere in the code on using only one term.

[Ross Kirsling]

(In reply to Mark Lam from comment #11)
> Regardless, I think we all agree on one thing: let's be consistent
> everywhere in the code on using only one term.

Absolutely. My thinking is merely that global consistency is tricky at present but "when callData.type is Native, we have a TaggedNativeFunction callData.native.function" is nice local consistency; the notion of "host call" is a good counterpoint though, so I'm happy to switch it if anybody feels strongly enough.

[Darin Adler]

I understand one reason we might prefer "host". In the past I’ve found the "native" term ambiguous. To me, in a JavaScript engine, it seems the "native tongue" could be JavaScript itself, rather than the underlying computing system the engine is built on top of. It’s things like DOM bindings that cause us to leave the JavaScript type system, garbage collection, etc. that seem "less native".

[Ross Kirsling]

(In reply to Darin Adler from comment #13)
> I understand one reason we might prefer "host". In the past I’ve found the
> "native" term ambiguous. To me, in a JavaScript engine, it seems the "native
> tongue" could be JavaScript itself, rather than the underlying computing
> system the engine is built on top of. It’s things like DOM bindings that
> cause us to leave the JavaScript type system, garbage collection, etc. that
> seem "less native".

Oh, hmm, that's a bit surprising to me. I think I'd understand if somebody was concerned that C++ "isn't native enough"(?) but "native" is the term I'm used to hearing in describing something implemented below the level of userland code.

For instance, ECMA-262 requires that Function.prototype.toString include "[native code]" for native functions (https://tc39.es/ecma262/#prod-NativeFunction) and the non-userland Error classes are all NativeErrors (https://tc39.es/ecma262/#sec-native-error-types-used-in-this-standard).

Other references:
- https://reactnative.dev/
- http://craftinginterpreters.com/functions.html#native-functions

On the other hand, from the view of ECMA-262, the "host (environment)" for JSC is WebCore (or any application that embeds JSC).

[Saam Barati]

I have a slight personal preference for "Host" over "Native", but I don't care that much. My stronger preference is for consistency. (That said, we've had both these terms for a while, and I don't think it really confuses anybody.)