1117017 - Initialize the IdSet lazily in jsiter.cpp:Snapshot()

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

Bug 1098618 noticed that IdSet was typically much bigger than it needed to be,
and reduced the initial set size accordingly.

But even better than replacing one allocation with a smaller allocation
is avoiding the allocation altogether. When running pdf.js we quickly create
hundreds of thousands of IdSets -- pdf.js passes lots of heterogenous data from
a worker to the main thread -- and 99.9% of the time they end up empty. This
can account for 5% of all heap allocations in the browser. So let's lazify the
allocation of their storage.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Initialize the IdSet lazily in jsiter.cpp:Snapshot()

Comment 2

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8543200 [details] [diff] [review]
Initialize the IdSet lazily in jsiter.cpp:Snapshot()

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

::: js/src/jsiter.cpp
@@ +108,5 @@
>          return true;
>  
>      if (!(flags & JSITER_OWNONLY) || pobj->is<ProxyObject>() || pobj->getOps()->enumerate) {
> +        if (!ht.initialized())
> +            ht.init(5);

Magic number. I didn't mind the original because there was a comment about the number 3. This should either become a #define, a static const, or (my preference) have a comment justifying the value 5.

@@ +276,5 @@
>  static bool
>  Snapshot(JSContext *cx, HandleObject pobj_, unsigned flags, AutoIdVector *props)
>  {
> +    // We initialize |ht| lazily (in Enumerate()) because it ends up unused
> +    // anywhere from 67--99.9% of the time.

What do you think of making this a Maybe<IdSet> instead, just to make it more clear what's going on in the parameter types? You'd save the (probably cheap) detail::HashTable constructor cost that way too. And I don't *think* that adds any dereferences. It's very slightly redundant, in that the mIsSome would always be bool(.table), but that doesn't seem like a big deal.

Comment 3

[Tom S [:evilpie]]

Comment on attachment 8543200 [details] [diff] [review]
Initialize the IdSet lazily in jsiter.cpp:Snapshot()

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

::: js/src/jsiter.cpp
@@ +108,5 @@
>          return true;
>  
>      if (!(flags & JSITER_OWNONLY) || pobj->is<ProxyObject>() || pobj->getOps()->enumerate) {
> +        if (!ht.initialized())
> +            ht.init(5);

You forgot to error check init.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: Initialize the IdSet lazily in jsiter.cpp:Snapshot()

This is the first time I've used Maybe<>, so I'd like another review. Thanks.

Comment 5

[Nicholas Nethercote [inactive]]

I could be convinced to rename |ht| as |maybeHt|, BTW.

Comment 6

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8544263 [details] [diff] [review]
Initialize the IdSet lazily in jsiter.cpp:Snapshot()

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

Cool, thanks! (And I prefer 'ht' to 'maybeHt'.)

Comment 7

[Nicholas Nethercote [inactive]]

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

Comment 8

[Carsten Book [:Tomcat]]

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