Closed Bug 1438126 Opened 6 years ago Closed 6 years ago

Some ExecutableAllocator changes

Categories

(Core :: JavaScript Engine: JIT, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: jandem, Assigned: jandem)

Details

Attachments

(3 files)

A few things I noticed after looking at bug 1438057.
Attachment #8950872 - Flags: review?(tcampbell)
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)
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+
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.

Attachment

General

Created:
Updated:
Size: