894669 - Add analysis for variables unnecessarily entrained by inner functions

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: patch

For code patterns like:

function foo() {
  var a, b;
  function bar() { return a; }
  function baz() { return b; }
  function what() { return 3; }
}

foo is heavyweight, and its call object holds the values of both a and b.  Instances of bar and baz need to refer to this call object through their environment, and will thus also hold the values of a and b alive.  Strictly speaking, bar doesn't need to hold b alive, and baz doesn't need to hold a alive.  However, these edges exist in the GC graph and can cause leaks, such as the one through homescreenFrame in bug 893012.

Removing these edges from the graph could be done, though it would be a large undertaking and would increase the memory used by call objects.  Middle grounds are possible like doing a better job with inner functions that have no free variables.  (The |what| function above will entrain both a and b for no particular reason.)

In any case, a good first step is to better understand the scope of the problem.  The attached patch adds a --dump-entrained-variables option to the JS shell which dumps variables in outer scripts whose value is unnecessarily entrained by inner scripts.  A sample line of output for the homescreenFrame bug is:

Script setDisplayedApp (test.js:696) has variables entrained by apploaded (test.js:797) :: callback homescreenFrame

I ran this on the associated file (gaia/master/apps/system/js/window_manager.js) and got 114 different inner/outer pairs with unnecessarily entrained variables.  However, 101 of these are for the WindowManager run-once closure at the top level, whose locals are essentially global variables.  The remaining 13 are more manageable.

Comment 1

[Luke Wagner [:luke]]

Comment on attachment 776753 [details] [diff] [review]
patch

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

::: js/src/vm/ScopeObject.cpp
@@ +2268,5 @@
> +    if (!remainingNames.init())
> +        return false;
> +
> +    Shape *shape = script->bindings.callObjShape();
> +    while (!shape->isEmptyShape()) {

Not a big deal, but BindingsIter (filtering on iter->aliased()) is the abstraction I've been trying to get used instead of the raw shapes.

Comment 2

[Andrew McCreight [:mccr8]]

Thanks for fixing this so quickly!  It sounds really useful.

Comment 3

[Brian Hackett [Laid off!]]

I forgot to say earlier, but --dump-entrained-variables only works in debug builds.  This could be relaxed if need be.

https://hg.mozilla.org/integration/mozilla-inbound/rev/399362c3e135

Comment 4

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/75ae4e009a0c
for causing build errors like https://tbpl.mozilla.org/php/getParsedLog.php?id=25355300&tree=Mozilla-Inbound

Comment 5

[Brian Hackett [Laid off!]]

(In reply to Wes Kocher (:KWierso) from comment #4)
> Backed out in
> http://hg.mozilla.org/integration/mozilla-inbound/rev/75ae4e009a0c
> for causing build errors like
> https://tbpl.mozilla.org/php/getParsedLog.php?id=25355300&tree=Mozilla-
> Inbound

Those build errors don't have anything to do with this push, and show up even after the backout.  Did someone change build settings under the hood?  Can you backout this backout once inbound is stable again?

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Re-landed.
https://hg.mozilla.org/integration/mozilla-inbound/rev/edbf03dce90c

Comment 7

[Andrew McCreight [:mccr8]]

I wrote up a little script to implement a basic whitelisting filter, and I'm going to go through and start creating a whitelist.

https://github.com/amccreight/closure-entrained

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/edbf03dce90c

Comment 9

[David Bruant]

Sorry if my comment is naive, but if it's possible to build an analysis tool to find variables unnecessarily entrained by inner functions, isn't it possible to make that these variables stop being unnecessarily entrained?

Comment 10

[Jeff Walden [:Waldo]]

Sure it's possible.  But you're talking about two qualitatively different kinds of effort: one *before* compilation time, in terms of the developer rewriting the source; and one *during* compilation time, and possibly as well at runtime, in terms of SpiderMonkey generating different bytecode or native code.  It's generally faster to have to do less (and perform less analysis in the leadup, to recognize the optimization opportunity -- and note it *is* an optimization, not a bug, from the point of view of language semantics), so there's good reason to provide an analysis tool for developers to use, even if at some point, in some set of narrowly-recognized circumstances, the same task can be performed without developer help.

Comment 11

[David Bruant]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #10)
> and note it *is* an optimization, not a bug, from the point of view of language
> semantics)
Agreed. Freeing memory at all is an optimization from the language semantics POV  ;-)

> It's generally faster to have to do less (and perform less
> analysis in the leadup, to recognize the optimization opportunity 
I feel a bit of bookkeeping (maybe already done in SpiderMonkey?) can prevent the leak without hurting the initial load time.
I imagine somewhere is the knowledge of which "in-memory function scope slots" corresponds to which function scope variable. What you can do is wait for some condition (threshold of memory kept alive by a given function, or warmth of some part of code, or other) and perform the analysis (both static and dynamic) to know which reference can be broken and break it.

The downside would be that the leak keep happening, but it's for a shorter time or it's only keeping a small amount of memory alive. The upside is that no additional work is perform initially.