Bug 226563 - [JSC] Implement JIT ICs for InByVal
Summary: [JSC] Implement JIT ICs for InByVal
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ross Kirsling
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-06-02 18:29 PDT by Ross Kirsling
Modified: 2021-06-03 20:11 PDT (History)
7 users (show)

See Also:


Attachments
Patch (123.89 KB, patch)
2021-06-02 18:57 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (2.42 KB, patch)
2021-06-02 19:29 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (99.41 KB, patch)
2021-06-02 19:32 PDT, Ross Kirsling
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (99.39 KB, patch)
2021-06-02 20:18 PDT, Ross Kirsling
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (99.37 KB, patch)
2021-06-02 20:37 PDT, Ross Kirsling
sbarati: review+
Details | Formatted Diff | Diff
Patch for landing (103.93 KB, patch)
2021-06-03 19:23 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ross Kirsling 2021-06-02 18:29:44 PDT
[JSC] Implement JIT ICs for InByVal
Comment 1 Ross Kirsling 2021-06-02 18:57:49 PDT
Created attachment 430432 [details]
Patch
Comment 2 Ross Kirsling 2021-06-02 19:29:11 PDT Comment hidden (obsolete)
Comment 3 Ross Kirsling 2021-06-02 19:32:39 PDT
Created attachment 430436 [details]
Patch
Comment 4 Ross Kirsling 2021-06-02 20:18:23 PDT
Created attachment 430439 [details]
Patch
Comment 5 Ross Kirsling 2021-06-02 20:37:32 PDT
Created attachment 430440 [details]
Patch
Comment 6 Saam Barati 2021-06-03 16:51:25 PDT
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"
Comment 7 Ross Kirsling 2021-06-03 19:18:58 PDT
(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.
Comment 8 Ross Kirsling 2021-06-03 19:23:00 PDT
Created attachment 430533 [details]
Patch for landing
Comment 9 EWS 2021-06-03 20:11:01 PDT
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].
Comment 10 Radar WebKit Bug Importer 2021-06-03 20:11:19 PDT
<rdar://problem/78850863>