805003 - Implement Map.prototype.clear and Set.prototype.clear methods

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

Allen Wirfs-Brock has specified a Map.prototype.clear method. It'll be in the next draft.

I think it should be non-generic. Allen hasn't shared the spec language yet.

(We want Set.prototype.clear too; Set wasn't in the last draft though.)

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: v1

AFAIK the spec is still under wraps, so I've made a few assumptions:

- The effect of .clear() on any live iterators is just like delete()ing all the entries. That is, the iterator remains live. Calling .next() on it will throw StopIteration. But if you first add a new entry to the Map/Set, .next() returns the new entry.

- Like the other methods, .clear() is non-generic.

- .clear() does not depend on the .iterator() or .remove() methods; deleting or monkeypatching them from Map/Set.prototype has no effect on .clear().

Comment 2

[Luke Wagner [:luke]]

Comment on attachment 675684 [details] [diff] [review]
v1

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

Great tests, lovely code.  Just one question before I'm ready to r+:

::: js/src/builtin/MapObject.cpp
@@ +230,5 @@
> +     * particular, those Ranges are still live and will see any entries added
> +     * after a successful clear().
> +     */
> +    bool clear() {
> +        if (dataLength != 0 || hashShift != HashNumberSizeBits - initialBucketsLog2()) {

The second disjunct is a bit non-obvious.  IIUC, the goal is to free memory in cases where we don't have any live elements, but we have unnecessary space allocated for dead elements.  Is that right?  If so:
 1. do you think it's necessary?  Since we shrink on remove(), it seems like, at worst, we'd only be 2x bigger than the state after init() (where x is very small).  Is there some other case I'm missing where we accumulate a lot of dead space?
 2. assuming it is necessary, could we use dataCapacity instead (which would subsume dataLength)?

@@ +237,5 @@
> +            uint32_t oldDataLength = dataLength;
> +
> +            hashTable = NULL;
> +            if (!init()) {
> +                hashTable = oldHashTable;

Your comment in init() is good, could you add a comment to the effect:
// init() only mutates members on success; see comment above.
?

Comment 3

[Jason Orendorff [:jorendorff]]

(In reply to Luke Wagner [:luke] from comment #2)
>  1. do you think it's necessary?  Since we shrink on remove(), it seems
> like, at worst, we'd only be 2x bigger than the state after init() (where x
> is very small).  Is there some other case I'm missing where we accumulate a
> lot of dead space?

No, you're right, it's not necessary. Updated patch coming.

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: v2

The assertions worrying about the table being returned to exactly its initial size had to be removed, along with the comment claiming clear() would "minimize" memory usage; and while I was at it, I realized the range->onClear() stuff should happen inside the if.

Comment 5

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dc4f417ef79a

Comment 6

[Jason Orendorff [:jorendorff]]

Follow-up fix (d'oh):
https://hg.mozilla.org/integration/mozilla-inbound/rev/2773d9489ace

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dc4f417ef79a
https://hg.mozilla.org/mozilla-central/rev/2773d9489ace

Comment 8

[Tom S]

Documented on https://developer.mozilla.org/en-US/docs/Firefox_19_for_developers
https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/Map
https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/Set

When you feel fancy you could add an example.