Closed Bug 1038038 Opened 5 years ago Closed 5 years ago

ShapeTable space optimizations

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33
blocking-b2g 2.0+
Tracking Status
firefox31 --- wontfix
firefox32 --- fixed
firefox33 --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: njn, Assigned: njn)

Details

(Whiteboard: [MemShrink])

Attachments

(3 files, 1 obsolete file)

I have three ShapeTable space optimizations coming up.
Attached patch Reduce ShapeTable::MIN_SIZE_LOG2 (obsolete) — Splinter Review
Loading Gmail, something like 90% of the cases where we create a ShapeTable are
triggered by initBoundFunction(), which results in an entryCount of 0.
Attachment #8455144 - Flags: review?(bhackett1024)
(updated log message)
Attachment #8455146 - Flags: review?(bhackett1024)
Attachment #8455144 - Attachment is obsolete: true
Attachment #8455144 - Flags: review?(bhackett1024)
A linear search of 15 items is likely to be competitive with a hash table
lookup.
Attachment #8455149 - Flags: review?(bhackett1024)
For exapmle, currently if you have an entryCount of 9 you end up with a
capacity of 32, when 16 would be more appropriate.
Attachment #8455151 - Flags: review?(bhackett1024)
(In reply to Nicholas Nethercote [:njn] from comment #1)
> Created attachment 8455144 [details] [diff] [review]
> Reduce ShapeTable::MIN_SIZE_LOG2

A more comprehensive way to address empty ShapeTables would be to allow dictionary mode shapes to not have a ShapeTable. But that would have been a more invasive change, so I did the really simple thing to start with.
Together, these patches reduce memory usage starting up the browser and loading Gmail by about 400--500 KiB on Linux64.
Another potential solution to the empty ShapeTable problem: would it be possible to move the BOUND_FUNCTION flag from BaseShape to Shape, so that initBoundFunction() doesn't need to convert the function to dictionary mode?
(In reply to Nicholas Nethercote [:njn] from comment #7)
> Another potential solution to the empty ShapeTable problem: would it be
> possible to move the BOUND_FUNCTION flag from BaseShape to Shape, so that
> initBoundFunction() doesn't need to convert the function to dictionary mode?

Bound functions are such a mess. FWIW, bug 1000780 seems to be the right long-term fix, unfortunately it's blocked on (JIT) bug 1002473 but hopefully I can get to that in a month or two..
Comment on attachment 8455146 [details] [diff] [review]
(part 1) - Reduce ShapeTable::MIN_SIZE_LOG2

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

Sorry for the delay.  If bound functions are the main thing causing this, maybe make a note of it so this change can be reverted once bound functions stop being a trainwreck.
Attachment #8455146 - Flags: review?(bhackett1024) → review+
Attachment #8455149 - Flags: review?(bhackett1024) → review+
Attachment #8455151 - Flags: review?(bhackett1024) → review+
> If bound functions are the main thing causing this,
> maybe make a note of it so this change can be reverted once bound functions
> stop being a trainwreck.

There wouldn't be much point in reverting it -- we'll end up with small tables like that much less often, but it doesn't cost us anything to have a smaller minimum size. But I'll make a note that bound functions are what cause small tables most often.
(In reply to Nicholas Nethercote [:njn] from comment #10)
> > If bound functions are the main thing causing this,
> > maybe make a note of it so this change can be reverted once bound functions
> > stop being a trainwreck.
> 
> There wouldn't be much point in reverting it -- we'll end up with small
> tables like that much less often, but it doesn't cost us anything to have a
> smaller minimum size. But I'll make a note that bound functions are what
> cause small tables most often.

Hmm, in that case maybe just remove the comment entirely, I think it would just be a distraction.
I ended up increasing MIN_ENTRIES from 7 to 11, rather than 15, because 15 may have slowed down Octane a bit. It's hard to tell because Octane is so noisy. Still, that patch was the smallest part of the improvement.

I remeasured browser-startup-with-gmail and now I see a ~1.1 MiB improvement in the main runtime. I'm now not sure what I measured in comment 6.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f65e26da8b4
https://hg.mozilla.org/integration/mozilla-inbound/rev/6761558449da
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc0d9b08d24e
These are three very simple patches that make a nice memory saving, and are worth backporting to whatever B2G branches are appropriate.
blocking-b2g: --- → 1.4?
I think it's too late for 1.4, but I definitely want this for 2.0.
blocking-b2g: 1.4? → 2.0+
Just to clarify: part 1 and 2 just make some overly-generous hashtables a bit smaller, and part 3 avoids the creation of some hashtables. So they can only reduce memory consumption.
> This sounds like something that might be wanted for Dolphin?

Yes -- any and every B2G branch that's still open should get this. It's very low risk and can only reduce memory usage.
Seems to be beneficial for dolphin performance, which our partner currently has concerns with. 
Nicholas, do you mind doing a request for 1.4 uplift approval?
Flags: needinfo?(wchang) → needinfo?(n.nethercote)
Comment on attachment 8455146 [details] [diff] [review]
(part 1) - Reduce ShapeTable::MIN_SIZE_LOG2

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

Requesting 1.4 uplift approval. These patches can help with memory usage and are low risk, as per comment 15.
Attachment #8455146 - Flags: approval-mozilla-b2g32?
Attachment #8455146 - Flags: approval-mozilla-b2g30?
Attachment #8455149 - Flags: approval-mozilla-b2g32?
Attachment #8455149 - Flags: approval-mozilla-b2g30?
Attachment #8455151 - Flags: approval-mozilla-b2g32?
Attachment #8455151 - Flags: approval-mozilla-b2g30?
Flags: needinfo?(n.nethercote)
This is already on b2g 2.0 via landing on Aurora. b2g32 won't be a thing until Monday's merge.
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #21)
> This is already on b2g 2.0 via landing on Aurora. b2g32 won't be a thing
> until Monday's merge.

Which flag do I need to request a 1.4 uplift?
b2g30 is fine for that (v1.4 = 30, v2.0 = 32)
Comment on attachment 8455146 [details] [diff] [review]
(part 1) - Reduce ShapeTable::MIN_SIZE_LOG2

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

Requesting 1.4 uplift approval. These patches can help with memory usage and are low risk, as per comment 15.
Attachment #8455146 - Flags: approval-mozilla-b2g32?
Attachment #8455149 - Flags: approval-mozilla-b2g32?
Attachment #8455151 - Flags: approval-mozilla-b2g32?
We are currently shutting down 1.4 and only taking blockers. As this is already 2.0+, ni Ivan and Wayne, who are managing triage for 1.4, to make the call on whether this bug should land on 1.4.
Flags: needinfo?(wchang)
Flags: needinfo?(itsay)
It is beneficial for 1.4 on the memory usage. Let's take this in v1.4.
Flags: needinfo?(itsay)
Comment on attachment 8455146 [details] [diff] [review]
(part 1) - Reduce ShapeTable::MIN_SIZE_LOG2

Approved as per comment 26.
Attachment #8455146 - Flags: approval-mozilla-b2g30? → approval-mozilla-b2g30+
Attachment #8455149 - Flags: approval-mozilla-b2g30? → approval-mozilla-b2g30+
Attachment #8455151 - Flags: approval-mozilla-b2g30? → approval-mozilla-b2g30+
thanks
Flags: needinfo?(wchang)
You need to log in before you can comment on or make changes to this bug.