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

RESOLVED FIXED in mozilla19

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

({dev-doc-complete})

Other Branch
mozilla19
dev-doc-complete
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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.)
(Assignee)

Comment 1

5 years ago
Created attachment 675684 [details] [diff] [review]
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().
Assignee: general → jorendorff
Attachment #675684 - Flags: review?(luke)

Comment 2

5 years ago
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.
?
(Assignee)

Comment 3

5 years ago
(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.
(Assignee)

Comment 4

5 years ago
Created attachment 675794 [details] [diff] [review]
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.
Attachment #675684 - Attachment is obsolete: true
Attachment #675684 - Flags: review?(luke)
Attachment #675794 - Flags: review?(luke)

Updated

5 years ago
Attachment #675794 - Flags: review?(luke) → review+
(Assignee)

Comment 5

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc4f417ef79a
(Assignee)

Comment 6

5 years ago
Follow-up fix (d'oh):
https://hg.mozilla.org/integration/mozilla-inbound/rev/2773d9489ace
https://hg.mozilla.org/mozilla-central/rev/dc4f417ef79a
https://hg.mozilla.org/mozilla-central/rev/2773d9489ace
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla19

Updated

5 years ago
Keywords: dev-doc-needed
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.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.