Closed Bug 551021 Opened 11 years ago Closed 10 years ago

remove JSTreeContext* parameter from JSCompiler parse methods

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dherman, Assigned: dherman)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 4 obsolete files)

I split this task off from bug 518055. Make the JSTreeContext* parameter in all the JSCompiler parse methods a member variable; since it's updated infrequently, this would hopefully win by eliminating a bunch of passing around an unchanged tc.
***NB: This patch depends on the patch attached to bug 518055, which in turn depends on the patch attached to bug 550350.***

I'm submitting this for reference. Passes all tests, but I'm seeing no performance gains on my machine, and I consider it a *lose* for readability-- too easy to get the internal tc pointer in the wrong state.

Somebody else should probably double-check in case I'm doing perf tests (or the implementation) wrong. Otherwise, my vote on this patch is -1.

Dave
Blocks: 548461
Attachment #431222 - Flags: review?(brendan)
***NB: This patch still depends on the patch attached to bug 518055, which in turn
depends on the patch attached to bug 550350.***

Updated to reflect changes in bug 518055 (parseFoo --> foo).

Dave
Attachment #431222 - Attachment is obsolete: true
Attachment #431222 - Flags: review?(brendan)
I will update this based on suggestions from jimb and brendan: eliminate pushTreeContext and popTreeContext and move that logic into the TreeContext ctor/dtor. Updated patch forthcoming.

Dave
***NB: This patch still depends on the patch attached to bug 518055, which in
turn depends on the patch attached to bug 550350.***

Now uses RAII to push/pop the pointer to the JSTreeContext stack top.

Dave
Attachment #431741 - Attachment is obsolete: true
Attachment #431760 - Flags: review?(jim)
Comment on attachment 431760 [details] [diff] [review]
removes JSTreeContext* parameter from parse methods

This looks good to me, if 1) we can sort out the nits below, 2) you've run the regression tests, and 3) it benchmarks neutral or better.

>-    JSTreeContext tc(this);
>-    tc.scopeChain = chain;
>-    if (!GenerateBlockId(&tc, tc.bodyid))
>+    JSTreeContext currtc(this);
>+    currtc.scopeChain = chain;
>+    if (!GenerateBlockId(&currtc, currtc.bodyid))

"current" suggests to me that it's connected with something extant.  Could we call this "globaltc", since it represents the script's global code?

>+    JSTreeContext *prevtc = tc;
>+

Would "enclosingtc" be a better name?  If you change it here, change it in generatorExpr, too.

>     /* Initialize early for possible flags mutation via DestructuringExpr. */
>     JSTreeContext funtc(tc->compiler);
> 
>-    JSFunctionBox *funbox = EnterFunction(pn, tc, &funtc, funAtom, lambda);
>+    JSFunctionBox *funbox = EnterFunction(pn, prevtc, &funtc, funAtom, lambda);

Since JSTreeContexts are guaranteed by their constructors to have valid parent pointers, could EnterFunction and LeaveFunction just drop the prevtc argument altogether and use funtc's parent?

>-     * Each parser takes tree context struct.
>+     * Each parser requires this->tc to be properly initialized.

I think the right way to say this is, "Each parser must only be called in the dynamic scope of a JSTreeContext object, as indicated by this->tc."  Or something like that...
Attachment #431760 - Flags: review?(jim) → review+
> This looks good to me, if 1) we can sort out the nits below, 2) you've run the
> regression tests, and 3) it benchmarks neutral or better.

I'll ask cdleary to do one last benchmarking just to be sure. Passes all tests.

> Would "enclosingtc" be a better name?  If you change it here, change it in
> generatorExpr, too.

I think "parenttc" would be best since it's consistent with JSTreeContext::parent.

I'll attach one last patch in a moment.

Dave
Updated to latest tracemonkey tree, and to address points from jimb's review.

Dave
Attachment #431760 - Attachment is obsolete: true
Good patch and review comments.

Uber-nit-picky naming comment: globaltc is good, parenttc less so.

Sometimes you see oldtc or oldxx for canonical shortname xx (oldflags for flags, older for JSErrorReporter er, oldfp for JSStackFrame *fp, etc.). For short-lived locals the shorter name wins, but back-to-back consonants are always awkward, so parenttc seems losing at least on this basis compared to oldtc, oldertc, outertc, enclosingtc, etc.

Yet shouting at the end, e.g., parentTC, is too much. Could use _ a bit: parent_tc, but then you'd want global_tc to match. Not bad, but personally I'd go with oldtc just to match other such local naming conventions. It's totally up to you guys; I just wanted to write some of this down once.

/be
As jimb mentioned, "previous" or "old" suggests "earlier in history" rather than "part of the outer computation surrounding this inner computation." Your point is well-taken about the visuals. To my eye "outertc" looks best; reasonably short, no double-consonant, but with the more appropriate connotation of nested computation.

I'll wait for any other comments and probably update the patch tomorrow morning.

Dave
s/parenttc/outertc/g

I want to get one last sanity-check on perf and then it should be ready to land.

Dave
Attachment #432057 - Attachment is obsolete: true
Looks good to me perf-wise.
Huzzah!

/be
http://hg.mozilla.org/mozilla-central/rev/48e57a961b5e
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.