Bug 226146 – [JSC] Add JIT ICs for `#x in obj` feature

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Ross Kirsling]

This is needed in order to ship.

[Radar WebKit Bug Importer]

<rdar://problem/78653813>

[Ross Kirsling]

Created attachment 431495 [details]
Patch

[Ross Kirsling]

Created attachment 431514 [details]
Test patch for MIPS EWS

[Ross Kirsling]

Created attachment 431520 [details]
Test patch for MIPS EWS

[Ross Kirsling]

Created attachment 431704 [details]
Baseline-only patch for MIPS EWS debugging

[Ross Kirsling]

Created attachment 431707 [details]
Baseline-only patch for MIPS EWS debugging

[Ross Kirsling]

Created attachment 431708 [details]
Baseline-only patch for MIPS EWS debugging

[Ross Kirsling]

Created attachment 431734 [details]
Patch

Rebasing.

[Ross Kirsling]

Well, I dunno what was up with MIPS before, but we're green now!

[Saam Barati]

Comment on attachment 431734 [details]
Patch

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

Nice. r=me

> Source/JavaScriptCore/jit/Repatch.cpp:1083
> +        InlineCacheAction action = actionForCell(vm, base);

Do we care about Structure::propertyAccessesAreCacheable for has private brand semantics?

I also wonder if we care about uncacheable dictionary. It looks like setPrivateBrand will always transition.

Not sure if we really care about those cases or not.

My hunch is: we don't care about Structure::propertyAccessesAreCacheable, but we probably shouldn't rely on UncacheableDictionary not transitioning. That said, I don't see in our current implementation how we could do it without always transitioning given that we rely on the m_parentBrand pointer.

[Saam Barati]

(In reply to Saam Barati from comment #10)
> Comment on attachment 431734 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=431734&action=review
> 
> Nice. r=me
> 
> > Source/JavaScriptCore/jit/Repatch.cpp:1083
> > +        InlineCacheAction action = actionForCell(vm, base);
> 
> Do we care about Structure::propertyAccessesAreCacheable for has private
> brand semantics?
> 
> I also wonder if we care about uncacheable dictionary. It looks like
> setPrivateBrand will always transition.
> 
> Not sure if we really care about those cases or not.
> 
> My hunch is: we don't care about Structure::propertyAccessesAreCacheable,
> but we probably shouldn't rely on UncacheableDictionary not transitioning.
> That said, I don't see in our current implementation how we could do it
> without always transitioning given that we rely on the m_parentBrand pointer.

When I say "we don't care", I mean in terms of correctness:

    bool propertyAccessesAreCacheable()
    {
        return dictionaryKind() != UncachedDictionaryKind
            && prototypeQueriesAreCacheable()
            && !(typeInfo().getOwnPropertySlotIsImpure() && !typeInfo().newImpurePropertyFiresWatchpoints());
    }


I don't think "typeInfo().getOwnPropertySlotIsImpure" changes the semantics here.

I don't think prototypeQueriesAreCacheable() changes the semantics here either.

[Ross Kirsling]

Comment on attachment 431734 [details]
Patch

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

>>> Source/JavaScriptCore/jit/Repatch.cpp:1083
>>> +        InlineCacheAction action = actionForCell(vm, base);
>> 
>> Do we care about Structure::propertyAccessesAreCacheable for has private brand semantics?
>> 
>> I also wonder if we care about uncacheable dictionary. It looks like setPrivateBrand will always transition.
>> 
>> Not sure if we really care about those cases or not.
>> 
>> My hunch is: we don't care about Structure::propertyAccessesAreCacheable, but we probably shouldn't rely on UncacheableDictionary not transitioning. That said, I don't see in our current implementation how we could do it without always transitioning given that we rely on the m_parentBrand pointer.
> 
> When I say "we don't care", I mean in terms of correctness:
> 
>     bool propertyAccessesAreCacheable()
>     {
>         return dictionaryKind() != UncachedDictionaryKind
>             && prototypeQueriesAreCacheable()
>             && !(typeInfo().getOwnPropertySlotIsImpure() && !typeInfo().newImpurePropertyFiresWatchpoints());
>     }
> 
> 
> I don't think "typeInfo().getOwnPropertySlotIsImpure" changes the semantics here.
> 
> I don't think prototypeQueriesAreCacheable() changes the semantics here either.

Hmm, good question(s). tryCacheHasPrivateBrand is largely a mimicry of tryCacheCheckPrivateBrand except that it creates AccessCase::{InHit, InMiss}, so I'm kind of relying on the other having been written correctly.

Happy to do follow-up work here if needed.

[EWS]

Committed r279105 (239022@main): <https://commits.webkit.org/239022@main>

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