For libraries, it is good programming practice to avoid polluting the global object and keep a lot of state in local variables of large closure functions. e.g. instead of: var state0 = ... var state1 = ... function foo() { ... stateN ... } function main() { foo(); } do: function main() { var state0 = ... var state1 = ... function foo() { ... stateN ... } foo(); } Our performance on code written using these closures is much worse than code written using global names. Some microbenchmarks: var g = 0; function foo() { for (var i = 0; i < 100000000; i++) g++; } foo(); // js -m -n: 227 // d8: 302 // js -m: 520 // js -j: 217 function main() { var g = 0; function foo() { for (var i = 0; i < 100000000; i++) g++; } foo(); } main(); // js -m -n: 849 // d8: 575 // js -m: 829 // js -j: 2500 Inference is especially hurt by this pattern because we use NAME opcodes within closures, which inference doesn't understand at all. We still use an IC, and also need type barriers so things will be slower than with stock JM. We should be able to make accesses to variables within closures as fast as global accesses, provided we can determine the outer closure is non-reentrant: there is at most one activation of the closure with inner functions live at any point. The closure is not called while frames from an existing activation are on the stack, and after the closure is called no functions parented to a previous activation are called again. Then we can use a call object at a fixed address to represent the live activation, and bake addresses of its properties into the jitcode (as we do for global properties). I think we would want to detect reentrance dynamically, for robustness, but I'm not sure yet how much additional state this would entail (inference might help in reducing the resulting overhead, but can't really distinguish functions created by different activations of the same closure). This pattern doesn't show up in the main benchmark suites (ss-date-format-tofte excepted), but is important for some real world tests like JSLint (bug 652377) and gzip (bug 663087). Moreover, as stated above writing code this way is good practice and we shouldn't tacitly discourage that by making developers pay a large performance penalty for using closures.

Hmm. I thought we'd created fast paths for this at least in the tracer (see bug 530255 and bug 532477). Is that not working for some reason?

Hrm. Profile shows that with -j we're hitting js::GetClosureVar and js_SetCallVar after all. If I try this testcase: function main() { var g = 0; return function () { for (var i = 0; i < 100000000; i++) g++; } } main()(); then -j is fast (much faster than -m of even v8); I guess that's the case we optimized before....

Very raw WIP, just has structures associated with type objects to track closure state at script entry/exit and detect reentrant closures. On gzip (bug 663087) there are no reentrant closures, on JSLint (bug 662383) there are four small ones --- factory methods (names are prefix/infix/relation/assignop) which construct closures, several of which may be live at once (and which also reach up and do accesses on non-reentrant closure parents of the factory). Plan going forward with this is to not keep a fixed address for the individual variables, but have a fixed address which points to the base args and vars of the most recent activation. This needs two dereferences to get to a call var rather than one, but should still be really fast and doing the optimal thing would be really invasive (have to put the call object up front, can't use GETLOCAL/GETARG/etc. to access the vars) and should wait on a more complete refactoring of how closed args/vars are handled (dvander has a bug on this I think, can't find it).

Brian, do we need JM/TI versions of bug 530255 and bug 532477?

(In reply to comment #4) > Brian, do we need JM/TI versions of bug 530255 and bug 532477? Hopefully this bug will subsume both, will test Dromaeo before/after (would also be good to get some/all of the Dromaeo tests onto awfy-assorted).

Optimize NAME accesses which are definitely getting variables from a non-reentrant closure. Overhead at script entry/exit still not in place in JM (shouldn't be huge). For the second comment 0 testcase and the comment 2 testcase I get 250ms, 217ms by removing undefined checks (not safe to do, but these could be hoisted to loop or script headers). The comment 2 testcase gives me 393ms for TM, 576ms for d8.

> would also be good to get some/all of the Dromaeo tests onto awfy-assorted Does awfy-assorted run in a browser? If not, then doing this would involve a good bit of rewriting (though it would at least be possible for the string/array tests, unlike the DOM ones).

WIP state from before my vacation. Nearly feature complete but barely tested, makes us marginally faster than d8 on the gzip test (bug 663087), fixing fromCharCode will help a little more.

Patch. Most of this is the dynamic analysis to detect reentrant closures. The state here tracks pretty well with activation object construction/putting (though an exception for invoke sessions). Luke, do you mind looking at this?

Comment on attachment 543702 [details] [diff] [review] patch This is a big patch and seems to add a lot of code I don't understand. I could take a lot of time to learn it, but shouldn't current efforts be focused on stabilizing TI, finding regressions, measuring its memory usage, etc?

The main purpose of this patch is addressing a perf regression where TI is slower than plain JM, and nowhere near as fast as it could be (right now, the only regressions which need addressing are perf regressions). While it's making things much faster than JM, this is also a pretty glaring hole in the algorithm which I think is worth fixing now (it's also the only big feature patch I'm planning on doing while TI stabilizes).

This is also one of the JS patches that would actually help web-facing stuff, for what it's worth...

Rebase on the refactorings that have gone in since this patch was written. This moves closure information from TypeObject into TypeScript, and also moves a word for the function/global from JSScript into TypeScript (the number of TypeScripts at any point in time is much less than the number of JSScripts).

Comment on attachment 555123 [details] [diff] [review] rebase Review of attachment 555123 [details] [diff] [review]: ----------------------------------------------------------------- r=me with nits and on Luke's review of vm/Stack* and jsinterp* changes since he knows those areas better than me :) ::: js/src/jsfun.cpp @@ +781,5 @@ > + * the call object all end up in a contiguous range of slots. We need this > + * to be able to embed the args/vars arrays in the TypeFunctionClosure for > + * the function, after the call object's frame has finished. > + */ > + if (cx->typeInferenceEnabled() && gc::GetGCKindSlots(kind) < slots) { If it doesn't regress anything to do so, I advocate just always making these slots contiguous, it seems like what we'll want to do for bug 659577. ::: js/src/jsinterpinlines.h @@ +158,5 @@ > + * calls below. :XXX: this is pretty gross, and slows us down. Can the > + * debugger be prevented from observing this frame? > + */ > + fp->functionEpilogue(true); > + fp->markFunctionEpilogueDone(true); Comment is in vm/Stack.h but it's really unclear what these boolean inputs are, with just this snippet as context. ::: js/src/jsobj.cpp @@ +3014,5 @@ > { > JSScript *calleeScript = callee->getFunctionPrivate()->script(); > JSObject *res; > > + if (!calleeScript->ensureRanBytecode(cx, callee->getFunctionPrivate(), callee->getParent())) The name of this function is a little misleading, especially with these new parameters. Would something like "ensureAnalyzed" work better? ::: js/src/jsobj.h @@ +761,5 @@ > > void rollbackProperties(JSContext *cx, uint32 slotSpan); > > + js::Value *getSlotAddress(uintN slot) { > + JS_ASSERT(slot <= capacity); Should that be < capacity? (Was there something wrong with using &getSlotRef?) ::: js/src/jsobjinlines.h @@ +416,5 @@ > return slots && slots != fixedSlots(); > } > > +inline bool > +JSObject::contiguousSlots(size_t start, size_t count) const hasContiguousSlots ? ::: js/src/jsscript.h @@ +505,5 @@ > undefined properties in this > script */ > bool hasSingletons:1; /* script has singleton objects */ > + bool hasFunction:1; /* script has an associated function */ > + bool hasHeavyweightFunction:1; /* function is heavyweight */ This bit can't be derived from function()->isHeavyweight() ? ::: js/src/vm/Stack.h @@ +874,5 @@ > + * Mark any work needed in the function's epilogue as done. Only the args > + * and call objects are reset if activationOnly is set. If activationOnly > + * is *NOT* set, this call must be followed by a later functionEpilogue. > + */ > + inline void markFunctionEpilogueDone(bool activationOnly = false); Please replace these booleans with an enum or something named.

Comment on attachment 555123 [details] [diff] [review] rebase Review of attachment 555123 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsinfer.cpp @@ +5020,5 @@ > + if (fun) > + script->types->function = fun; > + > + if (!script->compileAndGo) { > + script->types->global = NULL; Shouldn't it already be null? @@ +5048,5 @@ > + * Walk the scope chain to the next call object, which will be the function > + * the script is nested inside. > + */ > + while (!scope->isCall()) > + scope = scope->getParent(); Watch out, strict eval sticks call objects on the scope chain. Does this need to skip over them? @@ +5129,5 @@ > + JSScript *child = children; > + while (child) { > + if (child->closure()->activeFrames) > + return false; > + if (child->hasInnerFunctions && !child->closure()->clearActive()) I think you need a JS_CHECK_RECURSION here for pathologically nested functions, or you could turn this into a loop. @@ +5148,5 @@ > + * or any of its transitive parents. > + */ > +static void > +CheckClosureParent(JSContext *cx, JSObject *scope, JSScript *script) > +{ Same JS_CHECK_RECURSION comment. @@ +5188,5 @@ > + * functions or any of their transitive inner functions. > + */ > + if (closure->activeFrames || !closure->clearActive()) { > + script->reentrantClosure = true; > + MarkTypeObjectFlags(cx, fp->fun(), OBJECT_FLAG_REENTRANT_CLOSURE); Once a closure is marked reentrant (here or in CheckClosureParent), why do we continue to record the activeCall and inc activeFrames. Could we just return and could activeFrames just be a bool? Similarly, could we have an early return at the top if the script is already marked reentrant? That would help minimize the penalty for hot scripts that are reentrant. @@ +5317,5 @@ > types = (TypeScript *) cx->calloc_(sizeof(TypeScript)); > + if (!types) > + return false; > + types->global = (js::GlobalObject *) TypeScript::GLOBAL_POISON; > + types->function = fun; Can you factor out initialization of TypeScript into a constructor and use cx->new_ here and in-place new below? ::: js/src/jsinfer.h @@ +920,5 @@ > + * Information attached to a script about its closure state. This is used > + * to track whether a closure is non-reentrant, information we can use to > + * generate very fast accesses to the args and vars of the closure. > + * > + * A closure is non-reentrant if, at any point in time, only the most recent I think the use of the word "closure" here and in the names of TypeClosure and TypeScript::closure is confusing. A JSScript isn't a closure, nor is a Call object, which are the main concepts we are dealing with here; the SM concept corresponding to the classic use of the word "closure" is a function object. I see you have a TypeX = "Type Information Corresponding to an X" naming scheme, so I would call this a TypeScript, but clearly that's taken and this is really just a "tear-off" of a TypeScript. Perhaps TypeScriptReentrancy? It's kinda long, but it clearly indicates that this is a sub-part of a TypeScript... @@ +925,5 @@ > + * activation (i.e. call object) of the closure is live. Here, liveness for an > + * activation means the activation is on the stack, or a transitive inner > + * function parented to the activation is on the stack. > + * > + * Because inner functions can be (and, quite often, are) stored into object s/into/in/ @@ +975,5 @@ > + /* If this is a closure, call object for the most recent activation. */ > + JSObject *activeCall; > + > + /* > + * If this is a closure, direct pointers to the most recent activation's "If this script is known to only produce non-reentrant closures," ::: js/src/jsinterp.cpp @@ +941,1 @@ > TypeScript::SetThis(cx, script, fp->thisValue()); Could this be factored out into a TypeMonitorExecute analogous to TypeMonitorCall? ::: js/src/jsobj.cpp @@ +3014,5 @@ > { > JSScript *calleeScript = callee->getFunctionPrivate()->script(); > JSObject *res; > > + if (!calleeScript->ensureRanBytecode(cx, callee->getFunctionPrivate(), callee->getParent())) Could this be called implicitly by the uses of type information below? Ideally, the VM at large should not have to call ensureX() ever. ::: js/src/jsparse.cpp @@ +1199,5 @@ > + /* > + * If this is an eval script, don't treat the saved caller function > + * stored in the first object slot as an inner function. > + */ > + size_t start = outer->savedCallerFun ? 1 : 0; Could you add JS_ASSERT_IF(outer->savedCallerFun, arr->vector[0] == outer->getCallerFunction()) ? @@ +1210,5 @@ > JS_ASSERT(fun->isInterpreted()); > JSScript *inner = fun->script(); > + if (outer->hasHeavyweightFunction) { > + outer->hasInnerFunctions = true; > + inner->isInnerFunction = true; So its clear that hasInnerFunctions and isInnerFunction don't literally mean "has inner functions" and "is inner function". I was about to say that they mean "has inner upward-reaching function" and "has free vars", but that's not true because, for functions like: function f() { var x; function g() { function h() { x } }; ++x } f is heavyweight, but g is not, so this isn't true either. Is this intended (viz., that f.hasInnerFunctions && g.isInnerFunction && !g.hasInnerFunction && !h.isInnerFunction)? I'm not familiar enough with the whole patch to point to how it goes wrong; I guess it all depends on whether it is sound, but conservative, to say a function doesn't have children/parent when it actually does. ::: js/src/jsscript.h @@ +593,5 @@ > + * Performed when the script first runs, or first runs after a TypeScript > + * GC purge. If fun/scope are NULL then the script must already have types > + * with scope information. > + */ > + inline bool ensureRanBytecode(JSContext *cx, JSFunction *fun = NULL, JSObject *scope = NULL); Can this be ensureRanBytecodeAnalysis?

> ::: js/src/jsobj.h > @@ +761,5 @@ > > > > void rollbackProperties(JSContext *cx, uint32 slotSpan); > > > > + js::Value *getSlotAddress(uintN slot) { > > + JS_ASSERT(slot <= capacity); > > Should that be < capacity? (Was there something wrong with using > &getSlotRef?) callObjVarArray and callObjArgArray use this, and getSlotRef may botch if there are no actual vars/args and they are fetching a zero-length array. Comment added. > > undefined properties in this > > script */ > > bool hasSingletons:1; /* script has singleton objects */ > > + bool hasFunction:1; /* script has an associated function */ > > + bool hasHeavyweightFunction:1; /* function is heavyweight */ > > This bit can't be derived from function()->isHeavyweight() ? function() is only accessible if the script has had types computed, and the parser needs heavyweight knowledge to set isInnerFunction/isOuterFunction correctly. > ::: js/src/vm/Stack.h > @@ +874,5 @@ > > + * Mark any work needed in the function's epilogue as done. Only the args > > + * and call objects are reset if activationOnly is set. If activationOnly > > + * is *NOT* set, this call must be followed by a later functionEpilogue. > > + */ > > + inline void markFunctionEpilogueDone(bool activationOnly = false); > > Please replace these booleans with an enum or something named. I added comments for the places where 'true' was passed here. The 'activationOnly' thing is a hack so InvokeSessionGuard works, and rather than add more code here I think it would be better to wait for InvokeSessionGuard to be removed entirely. > ::: js/src/jsinfer.cpp > @@ +5020,5 @@ > > + if (fun) > > + script->types->function = fun; > > + > > + if (!script->compileAndGo) { > > + script->types->global = NULL; > > Shouldn't it already be null? Here the global has a token indicating there is no scope information for the script. > @@ +5048,5 @@ > > + * Walk the scope chain to the next call object, which will be the function > > + * the script is nested inside. > > + */ > > + while (!scope->isCall()) > > + scope = scope->getParent(); > > Watch out, strict eval sticks call objects on the scope chain. Does this > need to skip over them? No, functions defined within an eval do not get isInnerFunction set, so this traverse will not happen. Assert/comment added. > @@ +5129,5 @@ > > + JSScript *child = children; > > + while (child) { > > + if (child->closure()->activeFrames) > > + return false; > > + if (child->hasInnerFunctions && !child->closure()->clearActive()) > > I think you need a JS_CHECK_RECURSION here for pathologically nested > functions, or you could turn this into a loop. Turned into a loop, here and for CheckClosureParent. > @@ +5188,5 @@ > > + * functions or any of their transitive inner functions. > > + */ > > + if (closure->activeFrames || !closure->clearActive()) { > > + script->reentrantClosure = true; > > + MarkTypeObjectFlags(cx, fp->fun(), OBJECT_FLAG_REENTRANT_CLOSURE); > > Once a closure is marked reentrant (here or in CheckClosureParent), why do > we continue to record the activeCall and inc activeFrames. Could we just > return and could activeFrames just be a bool? Similarly, could we have an > early return at the top if the script is already marked reentrant? That > would help minimize the penalty for hot scripts that are reentrant. When there are several levels of nesting for closures, it is possible to have a non-reentrant outer closure with a reentrant inner closure, or a non-reentrant inner closure with a reentrant outer closure. JSLint follows the former pattern in a couple places (the only place I have seen reentrant closures in the wild, for that matter). Since we need to keep track of reentrance state for all closures in such a nesting, there isn't really a clean way to return early in these methods. FWIW, I haven't seen hot closures in the wild (usually it is the code inside them that's hot, and that's where overhead is the most important), and these have lots of other associated costs anyways like call object construction and inner function cloning. > ::: js/src/jsinfer.h > @@ +920,5 @@ > > + * Information attached to a script about its closure state. This is used > > + * to track whether a closure is non-reentrant, information we can use to > > + * generate very fast accesses to the args and vars of the closure. > > + * > > + * A closure is non-reentrant if, at any point in time, only the most recent > > I think the use of the word "closure" here and in the names of TypeClosure > and TypeScript::closure is confusing. A JSScript isn't a closure, nor is a > Call object, which are the main concepts we are dealing with here; the SM > concept corresponding to the classic use of the word "closure" is a function > object. I see you have a TypeX = "Type Information Corresponding to an X" > naming scheme, so I would call this a TypeScript, but clearly that's taken > and this is really just a "tear-off" of a TypeScript. Perhaps > TypeScriptReentrancy? It's kinda long, but it clearly indicates that this > is a sub-part of a TypeScript... Renamed to TypeScriptNesting. > ::: js/src/jsparse.cpp > @@ +1199,5 @@ > > + /* > > + * If this is an eval script, don't treat the saved caller function > > + * stored in the first object slot as an inner function. > > + */ > > + size_t start = outer->savedCallerFun ? 1 : 0; > > Could you add JS_ASSERT_IF(outer->savedCallerFun, arr->vector[0] == > outer->getCallerFunction()) ? I tried added this, and the getCallerFunction() call busted because savedCallerFun is broken. Filed bug 683904. > @@ +1210,5 @@ > > JS_ASSERT(fun->isInterpreted()); > > JSScript *inner = fun->script(); > > + if (outer->hasHeavyweightFunction) { > > + outer->hasInnerFunctions = true; > > + inner->isInnerFunction = true; > > So its clear that hasInnerFunctions and isInnerFunction don't literally mean > "has inner functions" and "is inner function". I was about to say that they > mean "has inner upward-reaching function" and "has free vars", but that's > not true because, for functions like: > function f() { var x; function g() { function h() { x } }; ++x } > f is heavyweight, but g is not, so this isn't true either. Is this intended > (viz., that f.hasInnerFunctions && g.isInnerFunction && !g.hasInnerFunction > && !h.isInnerFunction)? I'm not familiar enough with the whole patch to > point to how it goes wrong; I guess it all depends on whether it is sound, > but conservative, to say a function doesn't have children/parent when it > actually does. Comments updated. It's fine for the information to only reflect direct nesting in heavyweight functions --- we just won't be able to optimize NAME accesses involving nesting in eval or non-heavyweight function scripts.

Pushing this to JM so it can get some fuzz testing before going to m-c. http://hg.mozilla.org/projects/jaegermonkey/rev/554045e04d89

Followup spinoff issue. When deciding whether to generate type barriers for NAME accesses, the patch above assumes it has complete information for the possible types of closure args and vars. This is not necessarily the case: we occasionally purge TypeScripts (once a minute in the browser) and afterwards we have no information about the types of locals and args stored in call objects created before the purge. So acccesses in such situations always need a barrier. By making closure args/vars into heap property accesses, this will be improved: we can treat closure args/vars exactly like properties during analysis, and keep track of their possible types across TypeScript purges. http://hg.mozilla.org/projects/jaegermonkey/rev/4a0ab16c2ac4

(In reply to Brian Hackett from comment #16) > I added comments for the places where 'true' was passed here. The > 'activationOnly' thing is a hack so InvokeSessionGuard works, and rather > than add more code here I think it would be better to wait for > InvokeSessionGuard to be removed entirely. That's it, I'm going to remove InvokeSessionGuard on Monday. Too much hackery, too little win. > Renamed to TypeScriptNesting. Thanks! That sounds great. > I tried added this, and the getCallerFunction() call busted because > savedCallerFun is broken. Filed bug 683904. Nice.

Comment on attachment 557576 [details] [diff] [review] updated Great job, cool optimization.

Followup fixes for bugs which decoder found while fuzzing the JM repo. In the epilogue for an outer function the activeCall is used to update the active args/vars, which is only valid to do if the function is non-reentrant. Detection for scripts which are nested in a parent that uses 'with' or 'let' was moved out of resolveNameAccess to ensure the script's nesting info is always detached from its parent in such cases. http://hg.mozilla.org/projects/jaegermonkey/rev/cd9eddc210bb

bhackett, could this have significantly changed the number of allocations we do? Check out this graph: http://graphs-new.mozilla.org/graph.html#tests=[[29,63,8],[29,1,8],[29,28,8]]&sel=1314592403466.06,1315773915833.5513&displayrange=90&datatype=running It shows the tracemalloc maxheaps count for m-c (green), inbound (red), and jm (blue). It looks like 4a0ab16c2ac4 caused an upwards spike in jm around Sep 1, but it could have been one of the earlier patches, maybe 554045e04d89? The regressing patch looks to have been merged to m-c on sep 6 (http://hg.mozilla.org/mozilla-central/rev/445b1e86590c) and inbound on sep 7 (http://hg.mozilla.org/integration/mozilla-inbound/rev/aebd7d72106d). There's some bi-modality in the introduced behaviour, which strangely enough disappeared when we removed the "Latest Headlines" RSS feed from the bookmarks toolbar (bug 696163).

(In reply to Nicholas Nethercote [:njn] from comment #23) > bhackett, could this have significantly changed the number of allocations we > do? Check out this graph: > > http://graphs-new.mozilla.org/graph.html#tests=[[29,63,8],[29,1,8],[29,28, > 8]]&sel=1314592403466.06,1315773915833.5513&displayrange=90&datatype=running > > It shows the tracemalloc maxheaps count for m-c (green), inbound (red), and > jm (blue). It looks like 4a0ab16c2ac4 caused an upwards spike in jm around > Sep 1, but it could have been one of the earlier patches, maybe > 554045e04d89? The regressing patch looks to have been merged to m-c on sep > 6 (http://hg.mozilla.org/mozilla-central/rev/445b1e86590c) and inbound on > sep 7 (http://hg.mozilla.org/integration/mozilla-inbound/rev/aebd7d72106d). > > There's some bi-modality in the introduced behaviour, which strangely enough > disappeared when we removed the "Latest Headlines" RSS feed from the > bookmarks toolbar (bug 696163). No, the number of allocations shouldn't have significantly changed with this patch. These numbers are all from the WINNT platform, and other platforms do not show any change over this date range. This, combined with the fact that the regression only showed up some of the time (bimodal behavior) and that it mysteriously fixed itself later on, suggest that the regression is because we were (and still are, for that matter) near some threshold where a relatively minor change can significantly affect our behavior on this test and platform.

AFAICS the change mentioned in Comment 21 did not land in m-c, or did I misread something? If not, is this change still needed?