Closed
Bug 1038038
Opened 10 years ago
Closed 10 years ago
ShapeTable space optimizations
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
People
(Reporter: n.nethercote, Assigned: n.nethercote)
Details
(Whiteboard: [MemShrink])
Attachments
(3 files, 1 obsolete file)
(deleted),
patch
|
bhackett1024
:
review+
lmandel
:
approval-mozilla-b2g30+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bhackett1024
:
review+
lmandel
:
approval-mozilla-b2g30+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bhackett1024
:
review+
lmandel
:
approval-mozilla-b2g30+
|
Details | Diff | Splinter Review |
I have three ShapeTable space optimizations coming up.
Assignee | ||
Comment 1•10 years ago
|
||
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)
Assignee | ||
Comment 2•10 years ago
|
||
(updated log message)
Attachment #8455146 -
Flags: review?(bhackett1024)
Assignee | ||
Updated•10 years ago
|
Attachment #8455144 -
Attachment is obsolete: true
Attachment #8455144 -
Flags: review?(bhackett1024)
Assignee | ||
Comment 3•10 years ago
|
||
A linear search of 15 items is likely to be competitive with a hash table
lookup.
Attachment #8455149 -
Flags: review?(bhackett1024)
Assignee | ||
Comment 4•10 years ago
|
||
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)
Assignee | ||
Comment 5•10 years ago
|
||
(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.
Assignee | ||
Comment 6•10 years ago
|
||
Together, these patches reduce memory usage starting up the browser and loading Gmail by about 400--500 KiB on Linux64.
Assignee | ||
Comment 7•10 years ago
|
||
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?
Comment 8•10 years ago
|
||
(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 9•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8455149 -
Flags: review?(bhackett1024) → review+
Updated•10 years ago
|
Attachment #8455151 -
Flags: review?(bhackett1024) → review+
Assignee | ||
Comment 10•10 years ago
|
||
> 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.
Comment 11•10 years ago
|
||
(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.
Assignee | ||
Comment 12•10 years ago
|
||
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
Assignee | ||
Comment 13•10 years ago
|
||
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+
Assignee | ||
Comment 15•10 years ago
|
||
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.
Comment 16•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4f65e26da8b4
https://hg.mozilla.org/mozilla-central/rev/6761558449da
https://hg.mozilla.org/mozilla-central/rev/bc0d9b08d24e
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Comment 17•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/e12c32a65348
https://hg.mozilla.org/releases/mozilla-aurora/rev/d94467fc9ade
https://hg.mozilla.org/releases/mozilla-aurora/rev/41d971477d80
This sounds like something that might be wanted for Dolphin?
status-b2g-v1.4:
--- → affected
status-b2g-v2.0:
--- → fixed
status-b2g-v2.1:
--- → fixed
status-firefox31:
--- → wontfix
status-firefox32:
--- → fixed
status-firefox33:
--- → fixed
Flags: needinfo?(wchang)
Assignee | ||
Comment 18•10 years ago
|
||
> 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.
Comment 19•10 years ago
|
||
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)
Assignee | ||
Comment 20•10 years ago
|
||
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?
Assignee | ||
Updated•10 years ago
|
Attachment #8455149 -
Flags: approval-mozilla-b2g32?
Attachment #8455149 -
Flags: approval-mozilla-b2g30?
Assignee | ||
Updated•10 years ago
|
Attachment #8455151 -
Flags: approval-mozilla-b2g32?
Attachment #8455151 -
Flags: approval-mozilla-b2g30?
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(n.nethercote)
Comment 21•10 years ago
|
||
This is already on b2g 2.0 via landing on Aurora. b2g32 won't be a thing until Monday's merge.
Assignee | ||
Comment 22•10 years ago
|
||
(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?
Comment 23•10 years ago
|
||
b2g30 is fine for that (v1.4 = 30, v2.0 = 32)
Assignee | ||
Comment 24•10 years ago
|
||
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?
Assignee | ||
Updated•10 years ago
|
Attachment #8455149 -
Flags: approval-mozilla-b2g32?
Assignee | ||
Updated•10 years ago
|
Attachment #8455151 -
Flags: approval-mozilla-b2g32?
Comment 25•10 years ago
|
||
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)
Comment 26•10 years ago
|
||
It is beneficial for 1.4 on the memory usage. Let's take this in v1.4.
Flags: needinfo?(itsay)
Comment 27•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8455149 -
Flags: approval-mozilla-b2g30? → approval-mozilla-b2g30+
Updated•10 years ago
|
Attachment #8455151 -
Flags: approval-mozilla-b2g30? → approval-mozilla-b2g30+
Comment 29•10 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•