Bug 205553 – [JSC] Compact Bytecodes more by emitting 1-byte Opcode

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

[Yusuke Suzuki]

When using op_wide16 / op_wide32, we are emitting Opcode in 16bit / 32bit. But Opcode always fit in 8bit.
We should emit 8bit opcode instead, I think this makes Bytecode further compact, and this is nice since bytecode Opcode access becomes alignment-free.

[Yusuke Suzuki]

Created attachment 386329 [details]
Patch

[Yusuke Suzuki]

Created attachment 386330 [details]
Patch

[Robin Morisset]

Comment on attachment 386330 [details]
Patch

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

I generally like the idea of this patch, but don't feel comfortable r+-ing it for now as I don't fully understand some of it.
One question about the general idea: it seems to limit us to 256 possible opcodes. How many do we currently have, and do we have a plan if we ever exceed 256?

> Source/JavaScriptCore/bytecode/Instruction.h:123
> +        return 1 + (Traits::opcodeLengths[opcodeID<Traits>()] - 1) * size + padding;

Maybe add a comment here, this computation is clear in the context of this patch, but I doubt it would be as clear if I stumble upon it in 6 months.

> Source/JavaScriptCore/generator/Argument.rb:87
> +        auto* stream = bitwise_cast<typename TypeBySize<size>::unsignedType*>(reinterpret_cast<uint8_t*>(this) + #{@index} * size + PaddingBySize<size>::value + 1);

I'm a bit confused by this '+1', can you explain the reasoning behind it?

> Source/JavaScriptCore/generator/Opcode.rb:53
> +        @args = args.map.with_index { |(arg_name, type), index| Argument.new arg_name, type, index } unless args.nil?

Does this mean that the opcode is now just an arg like any other? Or am I misunderstanding it?

> Source/JavaScriptCore/generator/Opcode.rb:-214
> -#{map_fields_with_size("            ", "__size", &:fits_write).join "\n"}

Should the other use of map_fields_with_size (line 195) also be updated?

> Source/JavaScriptCore/generator/Opcode.rb:256
> +        ASSERT_UNUSED(stream, bitwise_cast<const uint8_t*>(stream)[-1] == opcodeID);

From this, it seems like stream now points one past the opcodeID. But the +1 in the decode function below makes it seem like stream now points 1 before opcodeID instead. I'm getting confused, I struggle with this kind of code vulnerable to off-by-ones.

[Yusuke Suzuki]

Comment on attachment 386330 [details]
Patch

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

> One question about the general idea: it seems to limit us to 256 possible opcodes. How many do we currently have, and do we have a plan if we ever exceed 256?

Even without this patch, we are having 8bit bytecode format, so that Opcode ID is already restricted in 256 values. We can add large Opcode, but it becomes always 16-bit / 32-bit, which is super inefficient, so I think we will never take 256 < Opcodes. (And if we really want to take this path, we can do by adding op_wide16_opcode16, and read 16bit opcode, but I think we won't do that).
Currently we are using 179.

>> Source/JavaScriptCore/bytecode/Instruction.h:123
>> +        return 1 + (Traits::opcodeLengths[opcodeID<Traits>()] - 1) * size + padding;
> 
> Maybe add a comment here, this computation is clear in the context of this patch, but I doubt it would be as clear if I stumble upon it in 6 months.

Added.

>> Source/JavaScriptCore/generator/Argument.rb:87
>> +        auto* stream = bitwise_cast<typename TypeBySize<size>::unsignedType*>(reinterpret_cast<uint8_t*>(this) + #{@index} * size + PaddingBySize<size>::value + 1);
> 
> I'm a bit confused by this '+1', can you explain the reasoning behind it?

+ 1 is for Opcode (1 byte). @index no longer include Opcode.

>> Source/JavaScriptCore/generator/Opcode.rb:53
>> +        @args = args.map.with_index { |(arg_name, type), index| Argument.new arg_name, type, index } unless args.nil?
> 
> Does this mean that the opcode is now just an arg like any other? Or am I misunderstanding it?

The opcode is no longer included in @args. @args only contain operands, and which starts with 0-index now.

>> Source/JavaScriptCore/generator/Opcode.rb:-214
>> -#{map_fields_with_size("            ", "__size", &:fits_write).join "\n"}
> 
> Should the other use of map_fields_with_size (line 195) also be updated?

This is intentional not to generate wrong expression if there is no @args.

>> Source/JavaScriptCore/generator/Opcode.rb:256
>> +        ASSERT_UNUSED(stream, bitwise_cast<const uint8_t*>(stream)[-1] == opcodeID);
> 
> From this, it seems like stream now points one past the opcodeID. But the +1 in the decode function below makes it seem like stream now points 1 before opcodeID instead. I'm getting confused, I struggle with this kind of code vulnerable to off-by-ones.

Now, `stream` always points the start of operands. So -1 is pointing opcode. And `decode` function is calculating the starting point of operands.

[Tadeu Zagallo]

Comment on attachment 386330 [details]
Patch

The patch looks good, my only concern is that I believe we'll have more than 256 opcodes in WebAssembly in a not so distant future. Have you measured how much do we gain with this patch?

[Yusuke Suzuki]

(In reply to Tadeu Zagallo from comment #5)
> Comment on attachment 386330 [details]
> Patch
> 
> The patch looks good, my only concern is that I believe we'll have more than
> 256 opcodes in WebAssembly in a not so distant future. Have you measured how
> much do we gain with this patch?

Not measuring things, just removing some wasting memory.
I think if we want to have more than 256 opcodes, we can just add op_wide16_16 etc., which interprets the next 16bits as Opcode, so I don't think this patch does not introduce restriction using < 256 Opcodes.

[Tadeu Zagallo]

(In reply to Yusuke Suzuki from comment #6)
> Not measuring things, just removing some wasting memory.
> I think if we want to have more than 256 opcodes, we can just add
> op_wide16_16 etc., which interprets the next 16bits as Opcode, so I don't
> think this patch does not introduce restriction using < 256 Opcodes.

Wouldn't that require adding different implementations for the opcodes, since we'd need to read the arguments at a different offset?

[Yusuke Suzuki]

(In reply to Tadeu Zagallo from comment #7)
> (In reply to Yusuke Suzuki from comment #6)
> > Not measuring things, just removing some wasting memory.
> > I think if we want to have more than 256 opcodes, we can just add
> > op_wide16_16 etc., which interprets the next 16bits as Opcode, so I don't
> > think this patch does not introduce restriction using < 256 Opcodes.
> 
> Wouldn't that require adding different implementations for the opcodes,
> since we'd need to read the arguments at a different offset?

I think we need adding different implementations anyway, otherwise, those opcodes always gets 16-bits / 32-bits operands even if operands fit in 8-bit.

[Yusuke Suzuki]

https://bugs.webkit.org/show_bug.cgi?id=197979#c12
This is saying that we can get free 500KB saving.

[Keith Miller]

(In reply to Yusuke Suzuki from comment #8)
> (In reply to Tadeu Zagallo from comment #7)
> > (In reply to Yusuke Suzuki from comment #6)
> > > Not measuring things, just removing some wasting memory.
> > > I think if we want to have more than 256 opcodes, we can just add
> > > op_wide16_16 etc., which interprets the next 16bits as Opcode, so I don't
> > > think this patch does not introduce restriction using < 256 Opcodes.
> > 
> > Wouldn't that require adding different implementations for the opcodes,
> > since we'd need to read the arguments at a different offset?
> 
> I think we need adding different implementations anyway, otherwise, those
> opcodes always gets 16-bits / 32-bits operands even if operands fit in 8-bit.

My guess is that we will either end up with a ARM Thumb style mode switch. Alternatively, we might also end up with prefixes for groups of opcodes.

[Keith Miller]

Comment on attachment 386330 [details]
Patch

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

r=me with some comments.

> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:322
> +        dispatch((constexpr %opcodeName%_length - 1) * 2 + 2)

Can you use names for these constants? I always find it like to read the code later and have names for the various numbers so I don't have to figure it out. In this case, it would be great if there were:

const OpcodeIDWide16Size = 2 # Wide16 Prefix + OpcodeID.

Ditto for the other sizes.

Also, can you put the comment Robin suggested here too.

[Yusuke Suzuki]

Comment on attachment 386330 [details]
Patch

Fixed, and I added more descriptive name to represent the meaning without comments.

[Yusuke Suzuki]

Committed r253904: <https://trac.webkit.org/changeset/253904>

[Radar WebKit Bug Importer]

<rdar://problem/58190791>

[Yusuke Suzuki]

Committed r253956: <https://trac.webkit.org/changeset/253956>