Bug 213545 – [JSC] add IC support for op_get_private_name

• Component: New Bugs
• Status: RESOLVED FIXED

[Caitlin Potter (:caitp)]

[JSC] add IC support for op_get_private_name

[Caitlin Potter (:caitp)]

Created attachment 402618 [details]
Patch

[Caitlin Potter (:caitp)]

Created attachment 402899 [details]
Patch

[Caitlin Potter (:caitp)]

Created attachment 403323 [details]
Patch

Fixes the JSVALUE32_64 builds, but I haven't been able to actually test them yet

[Saam Barati]

Comment on attachment 403323 [details]
Patch

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

Mostly LGTM, I agree with the direction here. But I have a suggestion and I also found a bug that we should have a test for.

> Source/JavaScriptCore/bytecode/AccessCase.cpp:268
> +    case LoadPrivate:

name nit: I'd call this "PrivateFieldLoad"

> Source/JavaScriptCore/bytecode/AccessCase.cpp:1437
> +        if (viaProxy() && m_type != LoadPrivate) {

why not have this be an assert inside viaProxy()?

We could just bail on caching if via window proxy.

> Source/JavaScriptCore/bytecode/AccessCase.h:127
> +        LoadPrivate,

Do we really need this to be a different type? It's going down the same path as Load. Why can't all the logic for private fields and cacheablility be done inside Repatch.cpp when deciding if it's ok to cache?

> Source/JavaScriptCore/jit/JITOperations.cpp:2229
> +    if (baseValue.isCell() && baseValue.asCell()->isObject()) {

nit: baseValue.isObject() instead

> Source/JavaScriptCore/jit/JITOperations.cpp:2231
> +        RETURN_IF_EXCEPTION(scope, encodedJSValue());

can this throw in practice? Maybe this should be an EXCEPTION_ASSERT that no exception happened? IIRC, throwing is related to OOM for resolving rope strings.

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:112
> +        auto notCell = branchIfNotCell(regT0);
> +        notCell.link(this);

not needed

> Source/JavaScriptCore/jit/JITPropertyAccess32_64.cpp:302
> +        auto notCell = branchIfNotCell(regT1);
> +        notCell.link(this);

ditto

> Source/JavaScriptCore/jit/Repatch.cpp:181
> +        return operationGetPrivateName;

this is wrong because the two functions here don't actually have the same signature. This will lead to it being called with the wrong arguments when you repatch. (For comparison, you can look at operationGetById and operationGetByIdOptimize).

Please add a test for this, as it should lead to a crash.

[Caitlin Potter (:caitp)]

Comment on attachment 403323 [details]
Patch

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

>> Source/JavaScriptCore/jit/Repatch.cpp:181
>> +        return operationGetPrivateName;
> 
> this is wrong because the two functions here don't actually have the same signature. This will lead to it being called with the wrong arguments when you repatch. (For comparison, you can look at operationGetById and operationGetByIdOptimize).
> 
> Please add a test for this, as it should lead to a crash.

Is there a way I can make this work without having the signature match? It seems like a shame that it needs to match

[Saam Barati]

Comment on attachment 403323 [details]
Patch

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

>>> Source/JavaScriptCore/jit/Repatch.cpp:181
>>> +        return operationGetPrivateName;
>> 
>> this is wrong because the two functions here don't actually have the same signature. This will lead to it being called with the wrong arguments when you repatch. (For comparison, you can look at operationGetById and operationGetByIdOptimize).
>> 
>> Please add a test for this, as it should lead to a crash.
> 
> Is there a way I can make this work without having the signature match? It seems like a shame that it needs to match

I mean, there is definitely a way. But it'd be crazy, and require patching way more code than just the call. The only reasonable way is to make them match IMO.

If you think about the machine code, we already have a call laid out. So as part of the ABI, we've prepped the argument registers with the required values. When we switch the call, we rewire just the call instruction (or materialization of constant) to be A or B. To rewire it to a different signature function would require a lot more machinery, that isn't really profitable in any way to do.

[Caio Lima]

Comment on attachment 403323 [details]
Patch

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

>> Source/JavaScriptCore/bytecode/AccessCase.h:127
>> +        LoadPrivate,
> 
> Do we really need this to be a different type? It's going down the same path as Load. Why can't all the logic for private fields and cacheablility be done inside Repatch.cpp when deciding if it's ok to cache?

I share the same question. The only difference from `LoadPrivate` and `Load` I can see is the way we print things on `PolymorphicAccess::printInternal`, but I might be missing something.

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:119
> +            JSValueRegs(regT0), JSValueRegs(regT1), JSValueRegs(regT0));

I think it is very good to have `GPRReg baseGRP = regT0;` and `GPRReg propertyGRP = regT1;` above, so the call here is easier to read as `JITGetByValGenerator gen(..., JSValueRegs(baseGPR), JSValueRegs(propertyGPR), JSValueRegs(regT0));`

> Source/JavaScriptCore/jit/JITPropertyAccess32_64.cpp:309
> +            JSValueRegs::payloadOnly(regT0), JSValueRegs(regT3, regT2), JSValueRegs(regT1, regT0));

ditto

[Caio Lima]

Comment on attachment 403323 [details]
Patch

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

> Source/JavaScriptCore/bytecode/AccessCase.cpp:1504
> +        if (m_type == Load || m_type == GetGetter || m_type == LoadPrivate) {

We are missing the inclusion of `m_type == LoadPrivate` above into 32-bits path and this is making we only copy payload when the executing IC's fast path. This endup corrupting the result of `get_private_name`, which is triggering some segmentation faults into ARMv7 and MIPS (they are reported on last EWS run, BTW).

[Caio Lima]

Comment on attachment 403323 [details]
Patch

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

>> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:112
>> +        notCell.link(this);
> 
> not needed

I'm not sure if we would like to have this path here. IIRC, this was added as a mitigation into get_by_val IC due to some performance regression into JetStream 2 or Speedometer 2, but I'm not sure if this would be necessary for private fields. It will only be possible to take this path if we have multiple evaluations of class and this might be a unlikely case IMHO. We might decide to add it later if we identify that multiple class evaluation is a common pattern.

[Caitlin Potter (:caitp)]

Created attachment 404383 [details]
Patch

[Caitlin Potter (:caitp)]

Comment on attachment 403323 [details]
Patch

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

I've found some minor problems with the current patch so far, including some mentioned here. Nothing significant though.

>> Source/JavaScriptCore/bytecode/AccessCase.cpp:268
>> +    case LoadPrivate:
> 
> name nit: I'd call this "PrivateFieldLoad"

I've removed the LoadPrivate enum value instead, as requested below

>> Source/JavaScriptCore/bytecode/AccessCase.cpp:1437
>> +        if (viaProxy() && m_type != LoadPrivate) {
> 
> why not have this be an assert inside viaProxy()?
> 
> We could just bail on caching if via window proxy.

done

>>> Source/JavaScriptCore/bytecode/AccessCase.h:127
>>> +        LoadPrivate,
>> 
>> Do we really need this to be a different type? It's going down the same path as Load. Why can't all the logic for private fields and cacheablility be done inside Repatch.cpp when deciding if it's ok to cache?
> 
> I share the same question. The only difference from `LoadPrivate` and `Load` I can see is the way we print things on `PolymorphicAccess::printInternal`, but I might be missing something.

It doesn't really need to be a different type in practice, but it seemed nice to have a different descriptive string for it. I've removed it.

>> Source/JavaScriptCore/jit/JITOperations.cpp:2229
>> +    if (baseValue.isCell() && baseValue.asCell()->isObject()) {
> 
> nit: baseValue.isObject() instead

done

>> Source/JavaScriptCore/jit/JITOperations.cpp:2231
>> +        RETURN_IF_EXCEPTION(scope, encodedJSValue());
> 
> can this throw in practice? Maybe this should be an EXCEPTION_ASSERT that no exception happened? IIRC, throwing is related to OOM for resolving rope strings.

no, in practice it should be impossible

--- oops, I haven't changed this yet...

>>> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:112
>>> +        notCell.link(this);
>> 
>> not needed
> 
> I'm not sure if we would like to have this path here. IIRC, this was added as a mitigation into get_by_val IC due to some performance regression into JetStream 2 or Speedometer 2, but I'm not sure if this would be necessary for private fields. It will only be possible to take this path if we have multiple evaluations of class and this might be a unlikely case IMHO. We might decide to add it later if we identify that multiple class evaluation is a common pattern.

it's gone

>> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:119
>> +            JSValueRegs(regT0), JSValueRegs(regT1), JSValueRegs(regT0));
> 
> I think it is very good to have `GPRReg baseGRP = regT0;` and `GPRReg propertyGRP = regT1;` above, so the call here is easier to read as `JITGetByValGenerator gen(..., JSValueRegs(baseGPR), JSValueRegs(propertyGPR), JSValueRegs(regT0));`

Done... I'm undecided on if it's actually better or not though 👀

>>>> Source/JavaScriptCore/jit/Repatch.cpp:181
>>>> +        return operationGetPrivateName;
>>> 
>>> this is wrong because the two functions here don't actually have the same signature. This will lead to it being called with the wrong arguments when you repatch. (For comparison, you can look at operationGetById and operationGetByIdOptimize).
>>> 
>>> Please add a test for this, as it should lead to a crash.
>> 
>> Is there a way I can make this work without having the signature match? It seems like a shame that it needs to match
> 
> I mean, there is definitely a way. But it'd be crazy, and require patching way more code than just the call. The only reasonable way is to make them match IMO.
> 
> If you think about the machine code, we already have a call laid out. So as part of the ABI, we've prepped the argument registers with the required values. When we switch the call, we rewire just the call instruction (or materialization of constant) to be A or B. To rewire it to a different signature function would require a lot more machinery, that isn't really profitable in any way to do.

My mistake, I didn't notice I was using this in 2 cases --- one where it worked (megamorphic before baseline JIT) and switching to megamorphic in the JIT slow path.

I've added a StructureStub parameter to the generic case, and just have it pass null for it if no StubInfo will be created (as opposed to having 2 generic variants, like GetByVal has)

I'll add an explicit test to make sure both paths are hit.

----

I've added a test since writing this draft, and talked with Caio offline about this... I haven't been able to actually get the IC system to completely give up on caching (and repatch with `operationGetPrivateName` instead of `operationGetPrivateNameOptimize`), so the only way I've been able to reproduce it is with --forceICFailure. This seems like a problem, but I'm not sure if it's with my changes to `tryCacheGetBy` or something upstream

> Source/JavaScriptCore/jit/Repatch.cpp:256
> +            if (baseCell->type() == PureForwardingProxyType && !isPrivate) {

I had tried adding `if (isPrivate) return GiveUpOnCache;` but the branch was never taken, even when the base object is `globalThis`.

I really have no idea :(

[Caitlin Potter (:caitp)]

Created attachment 404389 [details]
Patch

fixes the build, I think/hope. Not sure why cmake builds aren't erroring on unused variables ��

[Caitlin Potter (:caitp)]

I've fixed the C-linkage-for-C++-function compiler error locally.

[Caitlin Potter (:caitp)]

Created attachment 404586 [details]
Patch

[Caitlin Potter (:caitp)]

Created attachment 405334 [details]
Patch

[Saam Barati]

Comment on attachment 405334 [details]
Patch

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

Mostly LGTM, just a few comments.

> Source/JavaScriptCore/jit/JITOperations.cpp:2231
> +    // Only consider caching if the base value doesn't need to be converted to a JSObject

nit: this comment is not needed given the code is doing what the comment says

Question: should we invalidate the opportunity to cache if we ever see non-object?

> Source/JavaScriptCore/jit/JITOperations.cpp:2237
> +        JSObject* base = baseValue.asCell()->toObject(globalObject);

this should be a jsCast<JSObject*>

> Source/JavaScriptCore/jit/JITOperations.cpp:2260
> +    auto scope = DECLARE_THROW_SCOPE(vm);

nit: You can remove this.

> Source/JavaScriptCore/jit/JITOperations.cpp:2267
> +    RELEASE_AND_RETURN(scope, JSValue::encode(getPrivateName(globalObject, callFrame, baseValue, fieldNameValue)));

and just turn this into a normal return.

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:115
> +    gen.stubInfo()->accessType = AccessType::GetPrivateName;

nit: This is a bit fragile. It's relying on SSI's constructor not looking at this access type. Why not have JITGetByValGenerator take an AccessType as a parameter?

> Source/JavaScriptCore/jit/JITPropertyAccess32_64.cpp:306
> +    gen.stubInfo()->accessType = AccessType::GetPrivateName;

ditto

> Source/JavaScriptCore/jit/Repatch.cpp:385
> +                    ASSERT(!slot.isUnset());

let's RELEASE_ASSERT

> Source/JavaScriptCore/jit/Repatch.cpp:386
> +                    constexpr bool cannotBeProxy = false;

This name has a negative that it should not. I'd do:
"constexpr bool isProxy = false;"

[Caitlin Potter (:caitp)]

Created attachment 405385 [details]
Patch

[Caitlin Potter (:caitp)]

Comment on attachment 405334 [details]
Patch

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

Thanks for the review, Saam

>> Source/JavaScriptCore/jit/JITOperations.cpp:2231
>> +    // Only consider caching if the base value doesn't need to be converted to a JSObject
> 
> nit: this comment is not needed given the code is doing what the comment says
> 
> Question: should we invalidate the opportunity to cache if we ever see non-object?

I'm not sure. I'm imagining an application which primarily falls on the fast path, but rarely takes the slow path due to a primitive. It would be a shame to eliminate the fast path if the primitive case is taken rarely. But maybe it does make sense to do that.

Anyways, comment removed.

>> Source/JavaScriptCore/jit/JITOperations.cpp:2237
>> +        JSObject* base = baseValue.asCell()->toObject(globalObject);
> 
> this should be a jsCast<JSObject*>

done

>> Source/JavaScriptCore/jit/JITOperations.cpp:2260
>> +    auto scope = DECLARE_THROW_SCOPE(vm);
> 
> nit: You can remove this.

done and done

>> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:115
>> +    gen.stubInfo()->accessType = AccessType::GetPrivateName;
> 
> nit: This is a bit fragile. It's relying on SSI's constructor not looking at this access type. Why not have JITGetByValGenerator take an AccessType as a parameter?

Done and done

>> Source/JavaScriptCore/jit/Repatch.cpp:385
>> +                    ASSERT(!slot.isUnset());
> 
> let's RELEASE_ASSERT

done

>> Source/JavaScriptCore/jit/Repatch.cpp:386
>> +                    constexpr bool cannotBeProxy = false;
> 
> This name has a negative that it should not. I'd do:
> "constexpr bool isProxy = false;"

done

[Saam Barati]

Comment on attachment 405385 [details]
Patch

Nice. r=me

[EWS]

Committed r265000: <https://trac.webkit.org/changeset/265000>

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

[Radar WebKit Bug Importer]

<rdar://problem/66231520>