Bug 172669 – Implement a faster Interpreter::getOpcodeID().

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

We can implement Interpreter::getOpcodeID() without a hash table lookup by always embedding the OpcodeID in the 32-bit word just before the start of the LLInt handler code that executes each opcode.  getOpcodeID() can therefore just read the 32-bits before the opcode address to get its OpcodeID.

This is currently only enabled for CPU(X86), CPU(X86_64), CPU(ARM64), CPU(ARM_THUMB2), and only for OS(DARWIN).  It'll probably just work for linux as well, but I'll let the Linux folks turn that on after they have verified that it works on linux too.

I'll also take this opportunity to clean up how we initialize the opcodeIDTable:
1. we only need to initialize it once per process, not once per VM / interpreter instance.
2. we can initialize it in the Interpreter constructor instead of requiring a separate call to an initialize() function.

[Mark Lam]

Created attachment 311388 [details]
proposed patch.

[Build Bot]

Attachment 311388 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/interpreter/Interpreter.cpp:322:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Comment on attachment 311388 [details]
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

[Yusuke Suzuki]

Comment on attachment 311388 [details]
proposed patch.

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

> Source/JavaScriptCore/ChangeLog:11
> +        the 32-bits before the opcode address to get its OpcodeID.

Super cool!

> Source/JavaScriptCore/ChangeLog:16
> +        works on linux too.

Once it is landed, let me know. I'll check it on my linux box.

[Yusuke Suzuki]

Comment on attachment 311388 [details]
proposed patch.

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

>> Source/JavaScriptCore/ChangeLog:16
>> +        works on linux too.
> 
> Once it is landed, let me know. I'll check it on my linux box.

It works on x64 Linux box.

> Source/JavaScriptCore/llint/LowLevelInterpreter.cpp:509
> +// For x86 and x86_64, the .word directive only embeds a 16-bit value. So, we'll pad with
> +// another 16-bit .word to make up the upper bits.
> +#define EMBED_OPCODE_ID_IF_NEEDED(__opcode) ".word " __opcode##_value_string "\n .word 0\n"
> +
> +#elif CPU(ARM64) || CPU(ARM_THUMB2)
> +#define EMBED_OPCODE_ID_IF_NEEDED(__opcode) ".word " __opcode##_value_string "\n"
> +#endif

How about using `.int` instead?

[Mark Lam]

Comment on attachment 311388 [details]
proposed patch.

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

>> Source/JavaScriptCore/ChangeLog:11
>> +        the 32-bits before the opcode address to get its OpcodeID.
> 
> Super cool!

Thanks.

>>> Source/JavaScriptCore/ChangeLog:16
>>> +        works on linux too.
>> 
>> Once it is landed, let me know. I'll check it on my linux box.
> 
> It works on x64 Linux box.

I'll still let you make this change to enable for Linux separately (to lower the risk) after I land this patch.  Also might be able to enable this for ARM_TRADITIONAL but I'll leave that to the Linux ARM folks.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:325
> +#if !defined(NDEBUG) && USE(LLINT_EMBEDDED_OPCODE_ID)

I'll simplify this to #if !ASSERT_DISABLED.

>> Source/JavaScriptCore/interpreter/Interpreter.cpp:351
>>  }
> 
> 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

I've made all uses of opcodeIDTable conditional on !USE(LLINT_EMBEDDED_OPCODE_ID) || !ASSERT_DISABLED.  Hence, we won't build it for release builds anymore.

Thinking about this some more, I think NUMBER_OF_BYTECODE_IDS is the right value to use.  opcodeIDTable is only ever used for the basic bytecodes (whose values are < NUMBER_OF_BYTECODE_IDS).  There are extended bytecodes for some helper functions (which we classify as BYTECODE_HELPER_IDS and cloop synthetic branch targets which we classify as CLOOP_BYTECODE_HELPER_IDS).  However, getOpcodeID() is only needed for the basic bytecodes.  I've confirmed this by running the JSC test suite with this change.

I've switched the iterator type to unsigned.

>> Source/JavaScriptCore/interpreter/Interpreter.h:124
>> +#endif
> 
> perhaps there exists a function that already does this, if so, it could be worth abstracting it into a new function.

I fixed this by using MacroAssemblerCodePtr to decode the dataLocation of the opcode code.  This also simplifies the code.

>> Source/JavaScriptCore/llint/LowLevelInterpreter.cpp:509
>> +#endif
> 
> How about using `.int` instead?

Thanks.  I've switch to using .int.  It does exactly what I want and simplifies the code.

>> Source/WTF/wtf/Platform.h:924
>> +*/
> 
> should say 32-bit, not 16

Fixed.

[Mark Lam]

Created attachment 311432 [details]
patch for landing.

[Build Bot]

Attachment 311432 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/interpreter/Interpreter.cpp:323:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 311432 [details]
patch for landing.

Clearing flags on attachment: 311432

Committed r217526: <http://trac.webkit.org/changeset/217526>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/32479686>