Bug 22352 - Annotate opcodes with their length
Summary: Annotate opcodes with their length
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-11-19 00:22 PST by Judit Jász
Modified: 2008-12-09 11:19 PST (History)
2 users (show)

See Also:


Attachments
Proposed patch (53.08 KB, patch)
2008-11-19 00:42 PST, Judit Jász
zwarich: review-
Details | Formatted Diff | Diff
requested patch (50.92 KB, patch)
2008-12-08 15:14 PST, Judit Jász
zwarich: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Judit Jász 2008-11-19 00:22:15 PST
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.
Comment 1 Judit Jász 2008-11-19 00:42:57 PST
Created attachment 25261 [details]
Proposed patch

This solution was requested in the Bug #20764.
Comment 2 Cameron Zwarich (cpst) 2008-12-08 12:47:17 PST
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.
Comment 3 Judit Jász 2008-12-08 15:14:47 PST
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.
Comment 4 Cameron Zwarich (cpst) 2008-12-08 16:43:42 PST
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.
Comment 5 Cameron Zwarich (cpst) 2008-12-09 11:19:49 PST
Landed in r39141, with the minor change of replacing SIZE with LENGTH after a suggestion by Geoff.