Bug 43464 - Remove the global object from the bytecode
Summary: Remove the global object from the bytecode
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nathan Lawrence
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-08-03 21:26 PDT by Nathan Lawrence
Modified: 2010-08-05 15:23 PDT (History)
3 users (show)

See Also:


Attachments
patch (26.50 KB, patch)
2010-08-03 21:26 PDT, Nathan Lawrence
no flags Details | Formatted Diff | Diff
patch (29.18 KB, patch)
2010-08-04 09:34 PDT, Nathan Lawrence
no flags Details | Formatted Diff | Diff
patch (29.19 KB, patch)
2010-08-04 14:31 PDT, Nathan Lawrence
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nathan Lawrence 2010-08-03 21:26:31 PDT
Created attachment 63411 [details]
patch

Currently, the global object is being embedded in the JavaScriptCore bytecode, however since the global object is the same for all opcodes in a code block, we can have the global object just be a member of the associated code block.
Comment 1 WebKit Review Bot 2010-08-03 21:28:47 PDT
Attachment 63411 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
JavaScriptCore/bytecode/CodeBlock.cpp:1461:  Missing space before ( in if(  [whitespace/parens] [5]
JavaScriptCore/bytecode/Opcode.h:191:  One space before end of line comments  [whitespace/comments] [5]
JavaScriptCore/bytecode/Opcode.h:259:  Missing space before ( in switch(  [whitespace/parens] [5]
JavaScriptCore/bytecode/Opcode.h:260:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 4 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 2 Darin Adler 2010-08-03 23:22:40 PDT
Comment on attachment 63411 [details]
patch

> -        if(vPC[4].u.structure)
> -            vPC[4].u.structure->deref();
> +        if(vPC[3].u.structure)
> +            vPC[3].u.structure->deref();

There's a missing space after the word if in this code.

> -        macro(op_end, 2) // end must be the last opcode in the list
> +        macro(op_end, 2)  // end must be the last opcode in the list

Why did you add this space?

> +        switch(opcode)
> +        {

There should be a space after the word "switch" and the brace should be on the same line as the switch.

> +    #define OPCODE_ID_LENGTHS(id, length) case id: return OPCODE_LENGTH(id);
> +         FOR_EACH_OPCODE_ID(OPCODE_ID_LENGTHS);
> +    #undef OPCODE_ID_LENGTHS

I'd indent the FOR_EACH_OPCODE one level further so it's in the place where the code would have gone.

That semicolon after the macro invocation is not needed or helpful.

> +            default:
> +                ASSERT_NOT_REACHED();
> +                return 0;

Normally in switches like this where each case has a return, we put the default and the ASSERT_NOT_REACHED outside the switch statement so the compiler can warn us if there are any enum values that have no case.

In this case it's probably not an issue because we use the macro instead, but it would be nice to follow suit unless there is a reason not to.

> +        };

There's no need for a semicolon after the switch statement's closing brace.

> +    }
> +

How did you test all those different code paths?

All these concerns are minor, so I'm going to say r=me
Comment 3 Nathan Lawrence 2010-08-04 09:34:01 PDT
Created attachment 63458 [details]
patch

(In reply to comment #2)

I think I've addressed all of your concerns.

> How did you test all those different code paths?

I compiled and ran javascriptcore tests on the 32 bit JIT, the 64 bit JIT and the 64 bit interpreter.
Comment 4 WebKit Review Bot 2010-08-04 09:35:26 PDT
Attachment 63458 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
JavaScriptCore/bytecode/Opcode.h:259:  Missing space before ( in switch(  [whitespace/parens] [5]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Eric Seidel (no email) 2010-08-04 10:22:55 PDT
Comment on attachment 63411 [details]
patch

Cleared Darin Adler's review+ from obsolete attachment 63411 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 6 Nathan Lawrence 2010-08-04 14:31:45 PDT
Created attachment 63492 [details]
patch

forgot the space between swtich and '('
Comment 7 WebKit Commit Bot 2010-08-05 15:23:14 PDT
Comment on attachment 63492 [details]
patch

Clearing flags on attachment: 63492

Committed r64790: <http://trac.webkit.org/changeset/64790>
Comment 8 WebKit Commit Bot 2010-08-05 15:23:20 PDT
All reviewed patches have been landed.  Closing bug.