Bug 206563

Summary:

Shrink BytecodeStructs.h

Product:

WebKit

Reporter:

Robin Morisset <rmorisset>

Component:

JavaScriptCore

Assignee:

Robin Morisset <rmorisset>

Status:

ASSIGNED ---

Severity:

Normal

CC:

ews-watchlist, keith_miller, mark.lam, msaboff, saam, tzagallo

Priority:

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

See Also:

https://bugs.webkit.org/show_bug.cgi?id=206566

Attachments:

Description	Flags
WIP	none
Patch	tzagallo: review+
Patch	none
Patch	none
Patch	none
Patch	none

Description Robin Morisset 2020-01-21 17:21:50 PST

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.

Comment 1 Robin Morisset 2020-01-21 17:24:41 PST

Created attachment 388381 [details]
WIP

Comment 2 Robin Morisset 2020-01-21 18:32:33 PST

Created attachment 388387 [details]
Patch

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

Comment 3 Tadeu Zagallo 2020-01-21 18:53:33 PST

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`

Comment 4 Robin Morisset 2020-01-21 19:08:32 PST

Created attachment 388389 [details]
Patch

Comment 5 Robin Morisset 2020-01-21 19:09:08 PST

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.

Comment 6 Robin Morisset 2020-01-23 17:13:14 PST

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.

Comment 7 Robin Morisset 2020-01-28 04:27:24 PST

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.

Comment 8 Robin Morisset 2020-02-05 17:08:41 PST

Created attachment 389903 [details]
Patch

Re-trying to get this patch through EWS.

Comment 9 Robin Morisset 2020-05-21 20:04:53 PDT

Created attachment 400015 [details]
Patch

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