Bug 205553

Summary: [JSC] Compact Bytecodes more by emitting 1-byte Opcode
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: JavaScriptCoreAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, keith_miller, mark.lam, msaboff, rmorisset, saam, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch keith_miller: review+

Yusuke Suzuki
Reported 2019-12-22 23:55:10 PST
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.
Attachments
Patch (12.28 KB, patch)
2019-12-23 00:07 PST, Yusuke Suzuki
no flags
Patch (14.22 KB, patch)
2019-12-23 00:53 PST, Yusuke Suzuki
keith_miller: review+
Yusuke Suzuki
Comment 1 2019-12-23 00:07:00 PST
Yusuke Suzuki
Comment 2 2019-12-23 00:53:27 PST
Robin Morisset
Comment 3 2019-12-23 06:11:15 PST
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 4 2019-12-23 13:11:13 PST
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 5 2019-12-23 14:55:22 PST
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
Comment 6 2019-12-23 15:03:21 PST
(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
Comment 7 2019-12-23 15:07:34 PST
(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
Comment 8 2019-12-23 15:09:26 PST
(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
Comment 9 2019-12-23 19:40:49 PST
https://bugs.webkit.org/show_bug.cgi?id=197979#c12 This is saying that we can get free 500KB saving.
Keith Miller
Comment 10 2019-12-24 09:59:18 PST
(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 11 2019-12-24 10:18:03 PST
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 12 2019-12-25 01:01:47 PST
Comment on attachment 386330 [details] Patch Fixed, and I added more descriptive name to represent the meaning without comments.
Yusuke Suzuki
Comment 13 2019-12-25 01:07:24 PST
Radar WebKit Bug Importer
Comment 14 2019-12-25 01:08:27 PST
Yusuke Suzuki
Comment 15 2019-12-30 21:41:40 PST
Note You need to log in before you can comment on or make changes to this bug.