Last Comment Bug 805003 - Implement Map.prototype.clear and Set.prototype.clear methods
: Implement Map.prototype.clear and Set.prototype.clear methods
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: All All
: -- normal (vote)
: mozilla19
Assigned To: Jason Orendorff [:jorendorff]
: general
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-24 07:02 PDT by Jason Orendorff [:jorendorff]
Modified: 2012-12-11 09:58 PST (History)
5 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (24.00 KB, patch)
2012-10-26 14:04 PDT, Jason Orendorff [:jorendorff]
no flags Details | Diff | Splinter Review
v2 (23.87 KB, patch)
2012-10-26 20:43 PDT, Jason Orendorff [:jorendorff]
luke: review+
Details | Diff | Splinter Review

Description Jason Orendorff [:jorendorff] 2012-10-24 07:02:50 PDT
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] 2012-10-26 14:04:34 PDT
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().
Comment 2 Luke Wagner [:luke] 2012-10-26 15:34:47 PDT
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] 2012-10-26 20:39:57 PDT
(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] 2012-10-26 20:43:41 PDT
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.
Comment 5 Jason Orendorff [:jorendorff] 2012-11-02 15:46:41 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc4f417ef79a
Comment 6 Jason Orendorff [:jorendorff] 2012-11-02 16:43:05 PDT
Follow-up fix (d'oh):
https://hg.mozilla.org/integration/mozilla-inbound/rev/2773d9489ace

Note You need to log in before you can comment on or make changes to this bug.