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

RESOLVED FIXED in mozilla37

Status

()

Core
JavaScript Engine
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla37
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

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

Comment 1

3 years ago
Created attachment 8543200 [details] [diff] [review]
Initialize the IdSet lazily in jsiter.cpp:Snapshot()
Attachment #8543200 - Flags: review?(sphink)
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.
Attachment #8543200 - Flags: review?(sphink) → review+
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.
(Assignee)

Comment 4

3 years ago
Created attachment 8544263 [details] [diff] [review]
Initialize the IdSet lazily in jsiter.cpp:Snapshot()

This is the first time I've used Maybe<>, so I'd like another review. Thanks.
Attachment #8544263 - Flags: review?(sphink)
(Assignee)

Updated

3 years ago
Attachment #8543200 - Attachment is obsolete: true
(Assignee)

Comment 5

3 years ago
I could be convinced to rename |ht| as |maybeHt|, BTW.
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'.)
Attachment #8544263 - Flags: review?(sphink) → review+
(Assignee)

Comment 7

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/e95a4191ea5e
https://hg.mozilla.org/mozilla-central/rev/e95a4191ea5e
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.