Closed Bug 805003 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: JavaScript Engine, defect)

Other Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: jorendorff, Assigned: jorendorff)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 1 obsolete file)

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.)
Attached patch v1 (obsolete) — Splinter Review
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().
Assignee: general → jorendorff
Attachment #675684 - Flags: review?(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.
?
(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.
Attached patch v2Splinter Review
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.
Attachment #675684 - Attachment is obsolete: true
Attachment #675684 - Flags: review?(luke)
Attachment #675794 - Flags: review?(luke)
Attachment #675794 - Flags: review?(luke) → review+
https://hg.mozilla.org/mozilla-central/rev/dc4f417ef79a
https://hg.mozilla.org/mozilla-central/rev/2773d9489ace
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.