1069666 - Implement a proper WeakSet

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tom S. (please needinfo tschuster)]

Right now our WeakSet implementation is based on a WeakMap. While this is easy to implement and prevents bugs, it seems like a proper WeakSet would be more efficient and use less memory.

Comment 1

[André Bargull [:anba]]

Attachment: bug1069666.patch

To reduce code duplications I've added the WeakCollectionObject class to act as an abstract super-class for WeakMapObject and WeakSetObject. The rest was more or less simply a matter of translating the WeakSet methods from self-hosted JS to C++.

I've also moved some function declarations from jsweakmap.h to WeakMapObject.h, because the definitions are in WeakMapObject.cpp. And I've updated method-called-on-incompatible.js to test error reporting for another self-hosted function, now that WeakSet.prototype.add is no longer implemented in self-hosted JS.


Constructing an empty WeakSet is now roughly 30% faster when tested in this µ-benchmark:

    var t = dateNow();
    for (var i = 0; i < 1000000; ++i) {
        new WeakSet();
    }
    print(dateNow() - t);


The WeakSet methods are just a bit faster, because we only save the function call to the corresponding WeakMap method.

Comment 2

[Till Schneidereit [:till]]

Comment on attachment 8901183 [details] [diff] [review]
bug1069666.patch

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

Thank you, this is very nice!

Comment 3

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=852f43a29022ae32c759b79f0fd9db2579e9a1c0

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/645be0c7780a
Implement WeakSet as proper class instead of delegating to WeakMap. r=till

Comment 5

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for breaking non-unified build at js/src/builtin/WeakMapObject-inl.h:19:

https://hg.mozilla.org/integration/mozilla-inbound/rev/05f088f22a02191c1d406a195637ca82416e188f

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=126456747&repo=mozilla-inbound

/home/worker/workspace/build/src/js/src/builtin/WeakMapObject-inl.h:19:18: error: 'ProxyObject' was not declared in this scope
/home/worker/workspace/build/src/js/src/builtin/WeakMapObject-inl.h:19:31: error: no matching function for call to 'JSObject::is()'
/home/worker/workspace/build/src/js/src/builtin/WeakMapObject-inl.h:19:31: error: template argument 1 is invalid
/home/worker/workspace/build/src/js/src/builtin/WeakMapObject-inl.h:20:18: error: the value of 'ProxyObject' is not usable in a constant expression
/home/worker/workspace/build/src/js/src/builtin/WeakMapObject-inl.h:20:31: error: no matching function for call to 'JSObject::as()'
/home/worker/workspace/build/src/js/src/jsobj.h:571:51: error: 'class_' is not a member of 'js::WeakCollectionObject'

Comment 6

[André Bargull [:anba]]

(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #5)
> Backed out for breaking non-unified build at
> js/src/builtin/WeakMapObject-inl.h:19:
> 

Ugh, sorry for that!

Comment 7

[André Bargull [:anba]]

Attachment: bug1069666.patch

Add missing includes for non-unified builds to WeakMapObject-inl.h and WeakMapObject.cpp. Carrying r+ from till.

Comment 8

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d04e43a137b716892cd3c505cf4d71e71984cff8

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b534df0d1ef0
Implement WeakSet as proper class instead of delegating to WeakMap. r=till

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/b534df0d1ef0