524411 - JetpackRuntime.Context needs removeUnloader method

Status: RESOLVED FIXED

(Mozilla Labs :: Jetpack Prototype, defect, P1)

Description

[Drew Willcoxon :adw]

Attachment: patch

jetpack.menu frequently needs to perform some cleanup when either a doc or Jetpack context is unloaded, whichever comes first.  So it registers an unload event listener with the doc and adds an unloader using JetpackRuntime.Context.addUnloader().  When the doc is unloaded first, the context still maintains a reference to the unloader even though it's unnecessary, and the unloader hangs around until the context is unloaded.  With lots of unloaders, this is lots of memory that could be freed.  If I had a removeUnloader(), I could call it when the doc is unloaded.

Comment 1

[Drew Willcoxon :adw]

Attachment: patch v2

Ran into a subtle problem where I was removing unloaders while the unloaders array was being iterated over.  With this change, that's not a problem.

How's this, Atul?

Comment 2

[Atul Varma [:atul]]

So one of the things I've noticed about Narwhal's 'unload' module--which Cuddlefish currently inherits--is that it doesn't appear to actually support the de-registration of unloader callbacks, perhaps because doing so in JS requires the use of Array.indexOf(), which isn't terribly efficient.

The alternative I've found in Cuddlefish is to have each module simply register a "singleton" unloader that's responsible for unloading the module; that singleton then can do whatever it wants to unload individual objects it's created, et cetera.

The analog here, I think, would be for jetpack.menu to add a single unloader to each unique context it gets, and then for each of those unloaders to unload whatever menus need to be unloaded.  That could add an annoying amount of complexity to your menu code, though, as well as to all other modules that need similar functionality.

Anyhow, just curious what your thoughts are on that, as it's also pretty relevant to Cuddlefish.

Comment 3

[Drew Willcoxon :adw]

Somebody has to keep track of unloaders somewhere.  Even if I keep a singleton unloader, the need arises for non-singletons in situations like I described in comment 0, unless you don't mind keeping references to objects that don't need to be referred to.  The singleton's job would devolve into interfacing with the API and then iterating over the attached non-singletons, who would do the real work.  It's a question of whose job is it to track them, the runtime's or everyone who needs to interface with the runtime.

> the de-registration of unloader callbacks, perhaps because doing so in JS
> requires the use of Array.indexOf(), which isn't terribly efficient.

Array.indexOf is what I'd use in my singleton if I had a singleton.  This is an implementation optimization issue anyway.  It's easy to imagine assigning IDs or hashcodes to unloaders and stuffing them into a hash.

Comment 4

[Drew Willcoxon :adw]

(In reply to comment #3)
> to be referred to.  The singleton's job would devolve into interfacing with the
> API and then iterating over the attached non-singletons, who would do the real
> work.

s/API/runtime/

Comment 5

[Atul Varma [:atul]]

Comment on attachment 408428 [details] [diff] [review]
patch v2

Looks good, though it'd ideally be good to have a unit test on this. Drew, you can go ahead and commit it.

Comment 6

[Drew Willcoxon :adw]

Attachment: patch v3

Unit test added to JetpackRuntimeTests.

Comment 7

[Atul Varma [:atul]]

Comment on attachment 408652 [details] [diff] [review]
patch v3

Looks great, thanks!

Comment 8

[Drew Willcoxon :adw]

http://hg.mozilla.org/labs/jetpack/rev/784c3d8f4565