If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Remove a test from GCAlloc::Alloc

NEW
Assigned to

Status

Tamarin
Garbage Collection (mmGC)
P3
normal
7 years ago
5 years ago

People

(Reporter: Tommy Reilly, Assigned: Tony Printezis)

Tracking

unspecified
Future
x86
Mac OS X
Bug Flags:
flashplayer-injection -
flashplayer-qrb +
flashplayer-bug -

Details

(Whiteboard: PACMAN)

(Reporter)

Description

7 years ago
Currently there are two tests on the hot path, one for CollectionWork and one for the quick list being empty.   We used to only check for work in AllocBlock but that can be too rare, but on every alloc is probably too common.   Even if AllocBlock isn't hit (because we're in a steady state consuming/freeing off the freelists) the quicklist's will be consumed fairly frequently.  So why not move the CollectionWork test under the quicklist==null block?

Comment 1

7 years ago
(In reply to comment #0)
> We used to only check for work in AllocBlock
> but that can be too rare, but on every alloc is probably too common.

The burden of proof (analytical, not empirical) is on you.

> Even if AllocBlock isn't hit (because we're in a steady state
> consuming/freeing off the freelists) the quicklist's will be consumed
> fairly frequently.

You have to substantiate that, and "fairly frequently" is way too vague for me.

The #1 requirement here is incrementality of collection.  Most of the policy changes made for 10.1 were made to guarantee something about the interval of mark work, and frequent checks for collection work are absolutely crucial.  We can only move that under the quicklist check if we can make guarantees about the rate of quicklist expiry, and I am not (yet) convinced that you can make those guarantees without some external agent that forces the quicklist to be empty at reliable intervals.

Don't misunderstand me - I'd love to get rid of one of the tests on the hot path, and when I wrote the code I hated having more than one.  Even so, any analysis here needs to start with the requirements for the code and proceed from there.  If the requirements are not sufficiently well known / understood then by all means let's improve that first.
(Reporter)

Comment 2

7 years ago
Agreed, another factor here is that we already call CollectionWork from GC thread enter/leave with no gating so I think we probably call it too much at present.   

Even without that opportunistic slice we'd need to guarantee we didn't impact incrementality with this change as a lot can happen in one frame and we wouldn't want to suffer big pauses or excessive heap growth.
(Reporter)

Updated

7 years ago
Whiteboard: PACMAN

Updated

7 years ago
Target Milestone: --- → Future

Updated

6 years ago
Priority: -- → P3
Target Milestone: Future → Q1 12 - Brannan

Updated

6 years ago
Assignee: nobody → lhansen

Updated

6 years ago
Duplicate of this bug: 557901

Updated

6 years ago
Flags: flashplayer-qrb+
Flags: flashplayer-injection-
Flags: flashplayer-bug-

Comment 4

6 years ago
With the debugger-for-free patches applied it is possible to move the accounting onto the slow path, and without hand waving.

The crux is that no collection need be done as long as no allocation is done.  Also, so long as all the allocators do is free objects then there's also no reason to run the collector - free memory is free memory, nobody gets hurt if it just sits around.  It is only when some allocator runs out of memory that anything interesting happens.  At that point the free lists can be coalesced until we find some free memory, and accounting can happen during that coalescing.

(Finally, with explicit deletion gone all freeing will happen from the GC, which tightens the argument further - we control all points when the free lists can grow long, and we can deal with that explicitly if we need to.)
Assignee: lhansen → fklockii
((Food for thought) for Tony.)  Feel free to close as "WONTFIX", as you like.
Assignee: fklockii → tony.printezis
Target Milestone: Q1 12 - Brannan → Future
You need to log in before you can comment on or make changes to this bug.