Closed
Bug 1438126
Opened 6 years ago
Closed 6 years ago
Some ExecutableAllocator changes
Categories
(Core :: JavaScript Engine: JIT, defect, P1)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: jandem, Assigned: jandem)
Details
Attachments
(3 files)
21.95 KB,
patch
|
tcampbell
:
review+
|
Details | Diff | Splinter Review |
6.60 KB,
patch
|
tcampbell
:
review+
|
Details | Diff | Splinter Review |
1.28 KB,
patch
|
tcampbell
:
review+
|
Details | Diff | Splinter Review |
A few things I noticed after looking at bug 1438057.
Assignee | ||
Comment 1•6 years ago
|
||
Attachment #8950872 -
Flags: review?(tcampbell)
Assignee | ||
Comment 2•6 years ago
|
||
Using EnumeratedArray to keep track of allocated bytes per CodeKind is a bit nicer and less error-prone than having separate fields for each kind. I wrote this patch because I thought I needed it for the next patch, but that's no longer true. It's still a nice refactoring though.
Attachment #8950875 -
Flags: review?(tcampbell)
Assignee | ||
Comment 3•6 years ago
|
||
ExecutableAllocator::m_smallPools can reference up to 4 pools: 64 KB pools that are shared for small allocations. m_smallPools holds a strong reference to each of these pools, so we purge() them after every GC to ensure we don't hold these small pools alive forever (see bug 811176). This purging is too aggressive IMO - I added some logging and we often remove pools from m_smallPools that are still in use and contain a ton of unused space that now won't ever be reused. The attached patch changes purge() to only release() pools that have a refcount of 1 (when we know release() will actually deallocate them). When I run Sunspider in the shell, we now allocate 21 small pools instead of 46 (per run). For Kraken that's 36 -> 18. On Octane the data is more noisy and the difference is smaller, but it's still an improvement: ~430 -> ~395 small pools.
Attachment #8950879 -
Flags: review?(tcampbell)
Comment 4•6 years ago
|
||
Comment on attachment 8950872 [details] [diff] [review] Part 1 - Make CodeKind an enum class Review of attachment 8950872 [details] [diff] [review]: ----------------------------------------------------------------- :)
Attachment #8950872 -
Flags: review?(tcampbell) → review+
Comment 5•6 years ago
|
||
Comment on attachment 8950875 [details] [diff] [review] Part 2 - Use mozilla::EnumeratedArray Review of attachment 8950875 [details] [diff] [review]: ----------------------------------------------------------------- EnumeratedArray all the things!
Attachment #8950875 -
Flags: review?(tcampbell) → review+
Comment 6•6 years ago
|
||
Comment on attachment 8950879 [details] [diff] [review] Part 3 - Don't purge pools that are still in use Review of attachment 8950879 [details] [diff] [review]: ----------------------------------------------------------------- Elegant fix.
Attachment #8950879 -
Flags: review?(tcampbell) → review+
Pushed by jandemooij@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/bdeae8c5eeb9 part 1 - Make CodeKind an enum class. r=tcampbell https://hg.mozilla.org/integration/mozilla-inbound/rev/303215766a6f part 2 - Use mozilla::EnumeratedArray in ExecutableAllocator to track bytes per kind. r=tcampbell https://hg.mozilla.org/integration/mozilla-inbound/rev/515a82d2ec7b part 3 - Don't release small ExecutablePools when they're still in use elsewhere. r=tcampbell
Updated•6 years ago
|
status-firefox60:
--- → affected
Priority: -- → P1
Comment 8•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/bdeae8c5eeb9 https://hg.mozilla.org/mozilla-central/rev/303215766a6f https://hg.mozilla.org/mozilla-central/rev/515a82d2ec7b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in
before you can comment on or make changes to this bug.
Description
•