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.
Created attachment 388381 [details] WIP
Created attachment 388387 [details] Patch The compile time saving is disappointing, but there is a nice binary size saving.
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`
Created attachment 388389 [details] Patch
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.
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 on attachment 388623 [details] Patch Marking as obsolete in the hope of unblocking the jsc-armv7 test queue which appears stuck on this patch.
Created attachment 389903 [details] Patch Re-trying to get this patch through EWS.
Created attachment 400015 [details] Patch rebased, let's see if the armv7 EWS bot still dislikes this patch.