Bug 204904 – get_by_id ICs should have a structure history used to indicate when we should skip generating an IC

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

[Saam Barati]

...

[Saam Barati]

<rdar://problem/57631437>

[Saam Barati]

Created attachment 384976 [details]
patch

[Saam Barati]

Created attachment 384977 [details]
patch

[Saam Barati]

Comment on attachment 384977 [details]
patch

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

> Source/JavaScriptCore/runtime/OptionsList.h:497
> +    v(Unsigned, getByIdICMaxNumberOfStructures, 4, Normal, "Number of structures we see in the LLInt that could cause us to bail on generating an IC for various get_by_id ICs.") \

I'm tuning this now.

[Yusuke Suzuki]

Comment on attachment 384977 [details]
patch

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

r=me with nits.

> Source/JavaScriptCore/dfg/DFGGraph.h:1156
> +    HashSet<Node*> m_shouldSkipIC;

Why not adding this information in DFGNode itself?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:999
> +        jsValueResult(resultRegs, node, DataFormatJS);

`jsValueResult(resultRegs, node);` is OK.

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:509
> +        callOperationWithProfile(bytecode.metadata(m_codeBlock), operationGetByIdGeneric, resultVReg, TrustedImmPtr(m_codeBlock->globalObject()), regT0, TrustedImmPtr(ident->impl()));

Don't we need to do ArrayProfile in generic path?

[Tadeu Zagallo]

Comment on attachment 384977 [details]
patch

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

r=me

> Source/JavaScriptCore/bytecode/PointerHistory.h:45
> +        uintptr_t implBits = bitwise_cast<uintptr_t>(pointer);

nit: pointerBits?

> Source/JavaScriptCore/bytecode/PointerHistory.h:59
> +        update(count() + 1, filter());

Was this supposed to be `Options::getByValICMaxNumberOfIdentifiers() + 1`?

> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:647
> +    if (metadata.m_seenStructures.count() <= Options::getByIdICMaxNumberOfStructures())

maybe it's worth adding a helper to PointerHistory to perform this check? It's done in many places.

[Saam Barati]

Comment on attachment 384977 [details]
patch

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

>> Source/JavaScriptCore/bytecode/PointerHistory.h:59
>> +        update(count() + 1, filter());
> 
> Was this supposed to be `Options::getByValICMaxNumberOfIdentifiers() + 1`?

I changed the policy here. (I also noted it in the changelog). The idea is basically just to increment each time, which is pretty sensible for get_by_val and get_by_id.

>> Source/JavaScriptCore/dfg/DFGGraph.h:1156
>> +    HashSet<Node*> m_shouldSkipIC;
> 
> Why not adding this information in DFGNode itself?

I don't think there is room in the node for this. I could hijack a bit from the identifier number, or from the speculated type, but that feels ugly.

>> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:509
>> +        callOperationWithProfile(bytecode.metadata(m_codeBlock), operationGetByIdGeneric, resultVReg, TrustedImmPtr(m_codeBlock->globalObject()), regT0, TrustedImmPtr(ident->impl()));
> 
> Don't we need to do ArrayProfile in generic path?

My reasoning was just that if we're on the slow path, it is not needed. But I guess we can observe many structures all with the same array type. I'll add it back.

>> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:647
>> +    if (metadata.m_seenStructures.count() <= Options::getByIdICMaxNumberOfStructures())
> 
> maybe it's worth adding a helper to PointerHistory to perform this check? It's done in many places.

the different checks have different limits though. I guess I could pass in the limit, but I'm not sure it's much better than how it's done now.

>> Source/JavaScriptCore/runtime/OptionsList.h:497
>> +    v(Unsigned, getByIdICMaxNumberOfStructures, 4, Normal, "Number of structures we see in the LLInt that could cause us to bail on generating an IC for various get_by_id ICs.") \
> 
> I'm tuning this now.

I'm going to pick 5.

[Saam Barati]

Thanks for the reviews

[Saam Barati]

Created attachment 384989 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 384989 [details]
patch for landing

Clearing flags on attachment: 384989

Committed r253201: <https://trac.webkit.org/changeset/253201>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Saam Barati]

rolled out in:
https://trac.webkit.org/changeset/253263/webkit

It was not a speedup on anything, and slowed down JetStream 2's ML test by ~8%.