1006300 - Clean up ArenaList

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Nicholas Nethercote [inactive]]

ArenaList is a tricky class, despite the fact it's so small, mostly because |cursor| is a pointer-to-a-pointer. It could do with some encapsulation and better documentation.

I've filed this in the "JavaScript: GC" module. I hope this is appropriate.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Encapsulate and add better documentation and checking for ArenaList

This patch privatizes and renames ArenaList::{head,cursor}, renames a few other
things, adds lots of comments and assertions, and simplifies various bits of
code.

I recommend reviewing jsgc.h before jsgc.cpp.

If you're wondering why I removed the comment in backgroundFinalize(), it's
because I tried to rewrite it and failed, because I couldn't understand it. If
you do understand it and want to rewrite it, please do!

Note that the assertion in deepCheck() is currently commented out because it
occasionally fails. I plan to investigate this further, but I don't think it
needs to block this patch.

This patch applies on top of the patches in bug 1004790 and bug 1004816.

Comment 2

[Jon Coppeard (:jonco)]

Comment on attachment 8417822 [details] [diff] [review]
Encapsulate and add better documentation and checking for ArenaList

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

I really like the encapsulation and all the additional comments.

However I'm not sure about the concept of having a cursor vs. not having a cursor.

I think of the cursor as pointing to a place between list elements or after the end of the list, rather than pointing to an arena directly.  This means there is always a cursor, even if it is sometimes pointing after the end of the list.  This makes more sense to me than the cursor/no cursor distinction.

What do you think about this?

(Maybe cursor is not the best best term for this concept but I can't think of a better one right now).

::: js/src/jsgc.cpp
@@ -1700,5 @@
> -    /*
> -     * After we finish the finalization al->cursor must point to the end of
> -     * the head list as we emptied the list before the background finalization
> -     * and the allocation adds new arenas before the cursor.
> -     */

This is saying that the cursor is pointing to after the end of the list at this point.  When arenas are queued for background finalization, all arenas are moved to arenaListsToSweep, leaving the ArenaList empty.  Then, if new areans are allocated before background finalization finishes they are always added to the front of the list, leaving the cursor still pointing after the end of the list.

::: js/src/jsgc.h
@@ +428,5 @@
> +        //
> +        // XXX: this is currently commented out because it fails moderately
> +        // often. I'm not sure if this is because (a) it's not true that all
> +        // full arenas must precede all non-full arenas, or (b) we have some
> +        // defective list-handling code.

I once tried to add checks list this, but I had to add exceptions for so many corner cases that the checking code became unreadable and I gave up.  Hopefully you will have better luck :)

I think I found that sometimes non-full arenas could precede non-full arenas in some circumstances, and that it didn't matter.  Even so, for the sake of sanity it would be good to track down how this happens and fix it.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: Encapsulate and add better documentation and checking for ArenaList

New version: renames some things and updates some comments to use the "cursor
sits between arenas" idea, and rewrites the previously removed comment.

Comment 4

[Jon Coppeard (:jonco)]

Comment on attachment 8418452 [details] [diff] [review]
Encapsulate and add better documentation and checking for ArenaList

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

Looks great, thanks for tidying this up!

Comment 5

[Nicholas Nethercote [inactive]]

Try push looks good:
https://tbpl.mozilla.org/?tree=Try&rev=b6253c535141

Comment 6

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/797e6727523f

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/797e6727523f