Bug 206563 – Shrink BytecodeStructs.h

• Status: ASSIGNED
• Component: JavaScriptCore

[Robin Morisset]

BytecodeStructs.h is autogenerated by a ruby script, and is more than 60k lines of complex C++ templates.
It is included in a bunch of places, including CommonSlowPaths.h which is included in JIT.h which is included approximately everywhere.
The commit that introduced it slowed the compilation of JSC by 20%.

I have several ideas for shrinking it (with the goal to regain some of the compilation time loss), in particular through having opcodes of a same op_group share some of their code.

[Robin Morisset]

Created attachment 388381 [details]
WIP

[Robin Morisset]

Created attachment 388387 [details]
Patch

The compile time saving is disappointing, but there is a nice binary size saving.

[Tadeu Zagallo]

Comment on attachment 388387 [details]
Patch

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

Looks good to me, just a couple very minor comments.

> Source/JavaScriptCore/ChangeLog:13
> +        - The checks that op_wide16, op_wide32, wasm_wide16, wasm_wide32 fit in OpcodeID/WasmOpcodeID are now in a single place as a static assert in Fits.h, instead of being repeated in the checkImpl of every single instruction

Nice!

> Source/JavaScriptCore/generator/Opcode.rb:159
> +    def baseClass

nit: in ruby we usually use snake_case

> Source/JavaScriptCore/generator/OpcodeGroup.rb:59
> +    def constructors

Would it make sense to share this code with Opcode.rb?

> Source/JavaScriptCore/generator/Section.rb:46
> +  def create_opcode(name, config, opcode_group)

You can also use a default argument, e.g. `opcode_group = nil`

[Robin Morisset]

Created attachment 388389 [details]
Patch

[Robin Morisset]

Thanks for the quick review!

> > Source/JavaScriptCore/generator/Opcode.rb:159
> > +    def baseClass
> 
> nit: in ruby we usually use snake_case

Fixed.
 
> > Source/JavaScriptCore/generator/OpcodeGroup.rb:59
> > +    def constructors
> 
> Would it make sense to share this code with Opcode.rb?

Yes, fixed.

> > Source/JavaScriptCore/generator/Section.rb:46
> > +  def create_opcode(name, config, opcode_group)
> 
> You can also use a default argument, e.g. `opcode_group = nil`

Done.

[Robin Morisset]

Created attachment 388623 [details]
Patch

I found the bug: I was just putting dst as the last argument instead of first, for autogenerated Wasm opcodes. And because all of the arguments are of type VirtualRegister, there was no compilation failure.
I've fixed it, and also grouped a few more manually written Wasm opcodes, saving another 30kB from the binary.

[Robin Morisset]

Comment on attachment 388623 [details]
Patch

Marking as obsolete in the hope of unblocking the jsc-armv7 test queue which appears stuck on this patch.

[Robin Morisset]

Created attachment 389903 [details]
Patch

Re-trying to get this patch through EWS.

[Robin Morisset]

Created attachment 400015 [details]
Patch

rebased, let's see if the armv7 EWS bot still dislikes this patch.