[webkit-reviews] review granted: [Bug 172669] Implement a faster Interpreter::getOpcodeID(). : [Attachment 311388] proposed patch.

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 172669: Implement a faster Interpreter::getOpcodeID().
https://bugs.webkit.org/show_bug.cgi?id=172669

Attachment 311388: proposed patch.

https://bugs.webkit.org/attachment.cgi?id=311388&action=review




--- Comment #3 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 311388
  --> https://bugs.webkit.org/attachment.cgi?id=311388
proposed patch.

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

> Source/JavaScriptCore/interpreter/Interpreter.cpp:351
> +HashMap<Opcode, OpcodeID>& Interpreter::opcodeIDTable()
> +{
> +    static NeverDestroyed<HashMap<Opcode, OpcodeID>> opcodeIDTable;
>  
> -#if !ASSERT_DISABLED
> -    m_initialized = true;
> -#endif
> +    static std::once_flag initializeKey;
> +    std::call_once(initializeKey, [&] {
> +	   const Opcode* opcodeTable = LLInt::opcodeMap();
> +	   for (int i = 0; i < numOpcodeIDs; ++i)
> +	       opcodeIDTable.get().add(opcodeTable[i],
static_cast<OpcodeID>(i));
> +    });
> +
> +    return opcodeIDTable;
>  }

It looks like this hashtable is only used on debug builds. Perhaps we can only
build it on debug, or not build it all. Up to you.

Also, why does this loop use numOpcodeIDs but the other loop use
NUMBER_OF_BYTECODE_IDS?

One more nit: both use loops "int". I'd use unsigned or size_t

> Source/JavaScriptCore/interpreter/Interpreter.h:124
> +#if CPU(ARM_THUMB2)
> +	       int8_t* opcodeAddress = reinterpret_cast<int8_t*>(opcode) - 1;
// Decode thumb address.
> +	       int32_t* opcodeIDAddress =
reinterpret_cast<int32_t*>(opcodeAddress) - 1;
> +#else
> +	       int32_t* opcodeIDAddress = reinterpret_cast<int32_t*>(opcode) -
1;
> +#endif

perhaps there exists a function that already does this, if so, it could be
worth abstracting it into a new function.

> Source/WTF/wtf/Platform.h:924
> +/* This feature works by embedding the OpcodeID in the 16 bit just before
the generated LLint code
> +   that executes each opcode. It cannot be supported by the CLoop since
there's no way to embed the
> +   OpcodeID word in the CLoop's switch statement cases. It is also currently
not implemented for MSVC.
> +*/

should say 32-bit, not 16