Bug 226563 – [JSC] Implement JIT ICs for InByVal

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ross Kirsling]

[JSC] Implement JIT ICs for InByVal

[Ross Kirsling]

Created attachment 430432 [details]
Patch

[Ross Kirsling]

Created attachment 430435 [details]
Patch

(Rebased.)

[Ross Kirsling]

Created attachment 430436 [details]
Patch

[Ross Kirsling]

Created attachment 430439 [details]
Patch

[Ross Kirsling]

Created attachment 430440 [details]
Patch

[Saam Barati]

Comment on attachment 430440 [details]
Patch

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

Nice. r=me with a few comments

> Source/JavaScriptCore/bytecode/InByStatus.cppSource/JavaScriptCore/bytecode/InByIdStatus.cpp:299
> +CacheableIdentifier InByStatus::singleIdentifier() const
> +{
> +    if (m_variants.isEmpty())
> +        return nullptr;
> +
> +    CacheableIdentifier result = m_variants.first().identifier();
> +    if (!result)
> +        return nullptr;
> +    for (size_t i = 1; i < m_variants.size(); ++i) {
> +        CacheableIdentifier identifier = m_variants[i].identifier();
> +        if (!identifier)
> +            return nullptr;
> +        if (identifier != result)
> +            return nullptr;
> +    }
> +    return result;
> +}

We have so many implementations of this 😭

Maybe it's time for a templated version inside ICStatusUtils.h?

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:8250
> +            InByStatus status = InByStatus::computeFor(

nit: can we compute this inside the branch where it's used?

> Source/JavaScriptCore/jit/JITOperations.cpp:479
> +    if (key.getUInt32(i)) {
> +        if (arrayProfile)
> +            arrayProfile->observeIndexedRead(vm, baseObject, i);
> +        RELEASE_AND_RETURN(scope, JSValue::encode(jsBoolean(baseObject->hasProperty(globalObject, i))));
> +    }

Can we file a bug on making StructureStubInfo do in by val for indices? I think it should be straight-forward-ish, and could be a nice win. Similar to what we did for GetBy and indexed accesses.

> Source/JavaScriptCore/jit/Repatch.cpp:-922
> -        
> +
>          if (!base->structure(vm)->propertyAccessesAreCacheable() || (!wasFound && !base->structure(vm)->propertyAccessesAreCacheableForAbsence()))
>              return GiveUpOnCache;
> -        
> +
>          if (wasFound) {
>              if (!slot.isCacheable())
>                  return GiveUpOnCache;
>          }
> -        
> +
>          Structure* structure = base->structure(vm);
> -        

nit: lots of whitespace changes that aren't needed

> Source/JavaScriptCore/jit/Repatch.h:54
> +enum class InByKind {
> +    Normal,
> +    NormalByVal
> +};

I'm sorry for imparting this naming convention onto us. After this patch, I'd propose a renaming like:

"ById" and "ByVal"

[Ross Kirsling]

(In reply to Saam Barati from comment #6)
> Nice. r=me with a few comments

Thanks!

> > Source/JavaScriptCore/bytecode/InByStatus.cppSource/JavaScriptCore/bytecode/InByIdStatus.cpp:299
> > +CacheableIdentifier InByStatus::singleIdentifier() const
> > +{
> > +    if (m_variants.isEmpty())
> > +        return nullptr;
> > +
> > +    CacheableIdentifier result = m_variants.first().identifier();
> > +    if (!result)
> > +        return nullptr;
> > +    for (size_t i = 1; i < m_variants.size(); ++i) {
> > +        CacheableIdentifier identifier = m_variants[i].identifier();
> > +        if (!identifier)
> > +            return nullptr;
> > +        if (identifier != result)
> > +            return nullptr;
> > +    }
> > +    return result;
> > +}
> 
> We have so many implementations of this 😭
> 
> Maybe it's time for a templated version inside ICStatusUtils.h?

Done!

> > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:8250
> > +            InByStatus status = InByStatus::computeFor(
> 
> nit: can we compute this inside the branch where it's used?

Good call; not sure why existing examples don't.

> > Source/JavaScriptCore/jit/JITOperations.cpp:479
> > +    if (key.getUInt32(i)) {
> > +        if (arrayProfile)
> > +            arrayProfile->observeIndexedRead(vm, baseObject, i);
> > +        RELEASE_AND_RETURN(scope, JSValue::encode(jsBoolean(baseObject->hasProperty(globalObject, i))));
> > +    }
> 
> Can we file a bug on making StructureStubInfo do in by val for indices? I
> think it should be straight-forward-ish, and could be a nice win. Similar to
> what we did for GetBy and indexed accesses.

Filed bug 226619.

> > Source/JavaScriptCore/jit/Repatch.h:54
> > +enum class InByKind {
> > +    Normal,
> > +    NormalByVal
> > +};
> 
> I'm sorry for imparting this naming convention onto us. After this patch,
> I'd propose a renaming like:
> 
> "ById" and "ByVal"

Will do this as well as removing "Id" from *ByIdVariant as we discussed.

[Ross Kirsling]

Created attachment 430533 [details]
Patch for landing

[EWS]

Committed r278445 (238465@main): <https://commits.webkit.org/238465@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/78850863>

[Saam Barati]

*** Bug 203865 has been marked as a duplicate of this bug. ***