Bug 22352 – Annotate opcodes with their length

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

[Judit Jász]

Annotating each opcode with length can be convenient in many cases instead of the using of hard–coded constants. The file Opcode.h would be a suitable place to assign the lengths of the byte code instructions to the opcodes.

[Judit Jász]

Created attachment 25261 [details]
Proposed patch

This solution was requested in the Bug #20764.

[Cameron Zwarich (cpst)]

Comment on attachment 25261 [details]
Proposed patch

This is a really great idea. I only see two issues. First, you use an enum to store the lengths. It would make a lot more sense to use const ints instead. It would also make more sense to use OPCODE_SIZE instead of GET_OPCODE_SIZE, since there is no lookup and the value is resolved at compile time. I am r-'ing this, but I really like the change and it is something we definitely need.

If you don't want to update this patch to ToT WebKit due to all the conflicts, you can tell me that you want me to do it. Or you can just do the bytecode interpreter and I can do the JIT, since the JIT code is changing a lot every day now.

[Judit Jász]

Created attachment 25855 [details]
requested patch

Does this patch contain the requested things or have I misunderstood something? Because this was not so big task.

[Cameron Zwarich (cpst)]

Comment on attachment 25855 [details]
requested patch

+ 

You introduced some trailing whitespace at the top of Opcode.cpp.

+    #define OPCODE_ID_SIZES(id, size) const int opcode_size_##id = size;

I prefer the style ##id_length to opcode_length_##id.

Other than that, r=me. I will apply these changes and land it tonight. Thanks for doing this cleanup. There are still other uses of hardcoded opcode lengths in the code that you didn't catch, but I will make a separate bug for those.

[Cameron Zwarich (cpst)]

Landed in r39141, with the minor change of replacing SIZE with LENGTH after a suggestion by Geoff.