Last Comment Bug 752816 - Remove up-front flags from TreeContextFlags
: Remove up-front flags from TreeContextFlags
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: Nicholas Nethercote [:njn]
:
Mentors:
Depends on: 752793
Blocks: UntangleFrontEnd 753249
  Show dependency treegraph
 
Reported: 2012-05-07 23:01 PDT by Nicholas Nethercote [:njn]
Modified: 2012-05-15 06:29 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch 1: remove TCF_COMPILE_FOR_EVAL (6.17 KB, patch)
2012-05-07 23:02 PDT, Nicholas Nethercote [:njn]
luke: review+
Details | Diff | Review
patch 2: remove TCF_{NO_SCRIPT_RVAL,NEED_SCRIPT_GLOBAL} (21.28 KB, patch)
2012-05-07 23:03 PDT, Nicholas Nethercote [:njn]
luke: review+
Details | Diff | Review
patch 3: remove TCF_COMPILE_N_GO (19.31 KB, patch)
2012-05-07 23:33 PDT, Nicholas Nethercote [:njn]
luke: review+
Details | Diff | Review

Description Nicholas Nethercote [:njn] 2012-05-07 23:01:30 PDT
In this bug I will remove the flags in TreeContextFlags that are set up-front and not modified after that, and find nicer ways to deal with them.
Comment 1 Nicholas Nethercote [:njn] 2012-05-07 23:02:30 PDT
Created attachment 621892 [details] [diff] [review]
patch 1: remove TCF_COMPILE_FOR_EVAL

TCF_COMPILE_FOR_EVAL is dead;  its only use is removed in bug 751818's patch.
Comment 2 Nicholas Nethercote [:njn] 2012-05-07 23:03:56 PDT
Created attachment 621894 [details] [diff] [review]
patch 2: remove TCF_{NO_SCRIPT_RVAL,NEED_SCRIPT_GLOBAL}

This patch removes TCF_NO_SCRIPT_RVAL and TCF_NEED_SCRIPT_GLOBAL from
TreeContextFlags.  They're both only used during bytecode generation (not
during parsing) and they're both only set at the start of bytecode 
generation.  So I added bit-fields to BytecodeEmitter for them.

This required changing the signature of CompileScript().  It turns out that
these two flags plus TCF_COMPILE_N_GO were the only ones that could be
passed into CompileScript(), so I replaced the |tcflags| with three boolean
arguments.  TCF_COMPILE_N_GO still exists within CompileScript();  removing
it is next on my list.
Comment 3 Nicholas Nethercote [:njn] 2012-05-07 23:33:26 PDT
Created attachment 621897 [details] [diff] [review]
patch 3: remove TCF_COMPILE_N_GO

TCF_COMPILE_N_GO is a great example of why stuffing 20+ motley flags in a
single word is a bad idea.  It's a global setting that needs to be seen by
both Parser and BytecodeEmitter, and it's set up-front (it's an argument to
CompileScript) and never changed.

And yet, the current code carefully propagates it everywhere -- for each
level of context/function nesting, Parser copies it into
FunctionBox::tcflags and then BytecodeEmitter extracts it in EmitFunc.  Its
global, read-only nature is almost perfectly obfuscated.

This patch removes TCF_COMPILE_N_GO and adds a |compileAndGo| bit-field to
Parser, which is a good spot for such global state because it can be seen by
BytecodeEmitter as well (because BytecodeEmitter has a pointer to Parser).
Comment 4 :Ms2ger 2012-05-08 00:03:35 PDT
Comment on attachment 621894 [details] [diff] [review]
patch 2: remove TCF_{NO_SCRIPT_RVAL,NEED_SCRIPT_GLOBAL}

Review of attachment 621894 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/BytecodeEmitter.cpp
@@ +5194,5 @@
>       * API users may also set the JSOPTION_NO_SCRIPT_RVAL option when
>       * calling JS_Compile* to suppress JSOP_POPV.
>       */
> +    bool wantval = false;
> +    JSBool useful = JS_FALSE;

'false', at least
Comment 5 :Ms2ger 2012-05-08 00:05:49 PDT
(In reply to Nicholas Nethercote [:njn] from comment #3)
> Created attachment 621897 [details] [diff] [review]
> patch 3: remove TCF_COMPILE_N_GO
> 
> TCF_COMPILE_N_GO is a great example of why stuffing 20+ motley flags in a
> single word is a bad idea.  It's a global setting that needs to be seen by
> both Parser and BytecodeEmitter, and it's set up-front (it's an argument to
> CompileScript) and never changed.
> 
> And yet, the current code carefully propagates it everywhere -- for each
> level of context/function nesting, Parser copies it into
> FunctionBox::tcflags and then BytecodeEmitter extracts it in EmitFunc.  Its
> global, read-only nature is almost perfectly obfuscated.
> 
> This patch removes TCF_COMPILE_N_GO and adds a |compileAndGo| bit-field to
> Parser, which is a good spot for such global state because it can be seen by
> BytecodeEmitter as well (because BytecodeEmitter has a pointer to Parser).

I'd say the read-only nature is still rather obfuscated by making it a public field...
Comment 6 Luke Wagner [:luke] 2012-05-08 11:31:56 PDT
Comment on attachment 621897 [details] [diff] [review]
patch 3: remove TCF_COMPILE_N_GO

sanity++

One request: could you make these bitfields 'const' ?

Note You need to log in before you can comment on or make changes to this bug.