The default bug view has changed. See this FAQ.

Remove up-front flags from TreeContextFlags

RESOLVED FIXED in mozilla15

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla15
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
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.
Attachment #621892 - Flags: review?(luke)
(Assignee)

Comment 2

5 years ago
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.
Attachment #621894 - Flags: review?(luke)
(Assignee)

Updated

5 years ago
Blocks: 750279
Depends on: 752793
(Assignee)

Comment 3

5 years ago
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).
Attachment #621897 - Flags: review?(luke)
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
(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...

Updated

5 years ago
Attachment #621892 - Flags: review?(luke) → review+

Updated

5 years ago
Attachment #621894 - Flags: review?(luke) → review+

Comment 6

5 years ago
Comment on attachment 621897 [details] [diff] [review]
patch 3: remove TCF_COMPILE_N_GO

sanity++

One request: could you make these bitfields 'const' ?
Attachment #621897 - Flags: review?(luke) → review+
(Assignee)

Updated

5 years ago
Blocks: 753249
(Assignee)

Comment 7

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/88f2ecef3c57
https://hg.mozilla.org/integration/mozilla-inbound/rev/a255fc97aa05
https://hg.mozilla.org/integration/mozilla-inbound/rev/0507b653aa2f
https://hg.mozilla.org/mozilla-central/rev/88f2ecef3c57
https://hg.mozilla.org/mozilla-central/rev/a255fc97aa05
https://hg.mozilla.org/mozilla-central/rev/0507b653aa2f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
You need to log in before you can comment on or make changes to this bug.