Some ExecutableAllocator changes

RESOLVED FIXED in Firefox 60

Status

()

defect
P1
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: jandem, Assigned: jandem)

Tracking

unspecified
mozilla60
Points:
---

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(3 attachments)

Assignee

Description

Last year
A few things I noticed after looking at bug 1438057.
Assignee

Comment 1

Last year
Attachment #8950872 - Flags: review?(tcampbell)
Assignee

Comment 2

Last year
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

Last year
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 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 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 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+

Comment 7

Last year
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
Priority: -- → P1
You need to log in before you can comment on or make changes to this bug.