GCPolicyManager::signal assert firing

RESOLVED FIXED in Q3 11 - Serrano

Status

Tamarin
Garbage Collection (mmGC)
P1
normal
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: Tommy Reilly, Assigned: Tommy Reilly)

Tracking

unspecified
Q3 11 - Serrano
x86
Mac OS X
Bug Flags:
flashplayer-qrb +
flashplayer-bug +

Details

(Whiteboard: has-patch)

Attachments

(3 attachments)

(Assignee)

Description

8 years ago
Apparently there's no guard against starting an incremental mark when an allocation occurs from a prereap call.  Question is whether there be such a guard or whether the signal code is being too picky.

Updated

8 years ago
Flags: flashplayer-qrb+
Target Milestone: --- → Future

Updated

8 years ago
Priority: -- → P1
Target Milestone: Future → flash10.2

Updated

8 years ago
Assignee: nobody → lhansen

Comment 1

8 years ago
The signal() code depends for its correctness on events not being nested.  But given that OOM handling can trigger a collection pretty much any time, maybe that should be revisited.

Comment 2

8 years ago
Actually the OOM code calls Collect() and Collect() has a guard that prevents it from doing anything if a reap is ongoing.

Comment 3

8 years ago
Created attachment 445102 [details] [diff] [review]
Patch

Just guard calls to StartIncrementalMark and assert in StartIncrementalMark that reaping is not ongoing.
Attachment #445102 - Flags: review?(treilly)

Updated

8 years ago
Status: NEW → ASSIGNED
Whiteboard: has-patch
(Assignee)

Comment 4

8 years ago
the nested if's a little ugly can we use && or a "canStartMarking()" helper?

Updated

8 years ago
Attachment #445102 - Flags: review?(treilly) → review?(fklockii)
is this one of those "mom said no so I'll go ask dad?"  :)

but seriously, i know that tommy's gone; just surprised you did not incorporate his comments before switching the reviewer.

----

Unrelated: do you have a suggestion as to how to expose this bug?   Should I make the zct miniscule?  (will that make the prereap calls more frequent?  Would that suffice?)

Comment 6

8 years ago
(In reply to comment #5)
> is this one of those "mom said no so I'll go ask dad?"  :)

Well, not quite.

> but seriously, i know that tommy's gone; just surprised you did not incorporate
> his comments before switching the reviewer.

Observe that combining the tests using '&&' or a predicate is wrong for the first change, because the other arms of that decision tree will currently be tried only if marking is true, but with the proposed change would be tried if marking is true /or/ reaping is true, ie, more frequently.  That seems wrong; patch stands as submitted.

> Unrelated: do you have a suggestion as to how to expose this bug?   Should I
> make the zct miniscule?  (will that make the prereap calls more frequent? 
> Would that suffice?)

I don't think the zct is the issue per se.  See the recent thread that Antti started that Steven looped us into, there's a test case there.
Comment on attachment 445102 [details] [diff] [review]
Patch

Ah!  That pesky "else" dangling after the close brocket.

Now that I see that the nested if is necessary, I agree that its better to keep the logic looking the same in both cases.

(I'd recommend revising the bracketing to use either 
if (..) { 
  .. 
} else if (..) {
 ..
}

or: 

if (..)
{
  ..
}
else if (..)
{
  ..
}

but right now the mishmash of 

if (..) {
  ..
}
else if (..) {
  ..
}

is what led me to miss the crucial reason you used nested if here.

But its a minor detail; r+
Attachment #445102 - Flags: review?(fklockii) → review+

Comment 8

8 years ago
tamarin-redux changeset:   4707:29670bf4f87e

Comment 9

8 years ago
tamarin-redux-argo changeset:   4039:a2a51dbf7d9e
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Comment 10

8 years ago
Created attachment 446459 [details] [diff] [review]
Patch for tamarin-argo
(In reply to comment #7)
> 
> (I'd recommend revising the bracketing to use either 

Lars, you win (of course).  I was reading one of Scott Meyers books and saw a code fragment like:

if (..) {
  .. 
}
else {
  ..
}

So I acknowledge that the brace and line break style is accepted practice; we'll leave the question of "best" unanswered.

Comment 12

8 years ago
IMHO the style:


if (..)
{
  ..
}
else if (..)
{
  ..
}


makes for the most readable code; in the days of 80x24 screens, conserving vertical space was a good tradeoff, but not so much these days.
The issue I was raising was not trying to conserve vertical space, but rather forcing the reader to recognize that there is a decision tree rather than a single guarded command.  (Maybe I am making excuses for my own failure to review carefully.)
(Assignee)

Updated

7 years ago
Duplicate of this bug: 586989
(Assignee)

Comment 15

7 years ago
This patch isn't complete.  We can start a ReapZCT when we're marking and then call CollectionWork from an allocation and call IncrementalMark and hit the same assert.   Maybe CollectionWork should completely short circuit when Reaping?
(Assignee)

Updated

7 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 16

7 years ago
Created attachment 465656 [details] [diff] [review]
don't do any marking if we're reaping
Assignee: lhansen → treilly
Attachment #465656 - Flags: review?(lhansen)

Comment 17

7 years ago
Comment on attachment 465656 [details] [diff] [review]
don't do any marking if we're reaping

Please add a comment to the code summarizing briefly the findings that led to the change.
Attachment #465656 - Flags: review?(lhansen) → review+
(Assignee)

Comment 18

7 years ago
http://hg.mozilla.org/tamarin-redux/rev/4a1fce3e968f
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago7 years ago
Resolution: --- → FIXED

Updated

7 years ago
Flags: flashplayer-bug+
You need to log in before you can comment on or make changes to this bug.