Closed Bug 948554 Opened 7 years ago Closed 7 years ago

Record max incremental cycle collector slice pause time

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: mccr8, Assigned: mccr8)

References

Details

Attachments

(4 files)

No description provided.
This name is more precise, because we must call it once per slice,
not once per CC.  Being a method on the CC stats object is more consistent.
Attachment #8346009 - Flags: review?(bugs)
I was getting some weird too-big slice times, I think due to PRTime problems, so
I just converted these over to TimeStamp.
Attachment #8346011 - Flags: review?(bugs)
Attachment #8346009 - Flags: review?(bugs) → review+
Attachment #8346011 - Flags: review?(bugs) → review+
(In reply to Andrew McCreight [:mccr8] from comment #3)
> I was getting some weird too-big slice times, I think due to PRTime
> problems
Hmm, this sounds odd.
Attachment #8346012 - Flags: review?(bugs) → review+
Blocks: 939928
(In reply to Olli Pettay [:smaug] from comment #5)
> Hmm, this sounds odd.
Yeah, I may have just had some kind of other bug in my patch that I happened to fix later.  I'm not entirely sure.
backed out by Ryan for a Win7 debug xpcshell orange:

Assertion failure: !aOther.IsNull() (Cannot compute with aOther null value), at c:\builds\moz2_slave\m-in-w32-d-0000000000000000000\build\obj-firefox\dist\include\mozilla/TimeStamp.h:313

and maybe Win7 debug xpcshell timeouts.

https://hg.mozilla.org/integration/mozilla-inbound/rev/b464f609a193
Blocks: 911246
No longer blocks: IncrementalCC
I think I figured out what was triggering the assertion: under some circumstances, you
can end up calling ShouldTriggerCC() before sLastCCEndTime is set. But with that fixed,
it turns out that every call to TimeUntilNow does a null check on the argument, which
is annoying and error prone. So, in this patch, which I will land folded into part 2,
I just return 0 for TimeUntilNow() if the argument is null. It is used to answer questions
like "Has enough time passed?" and "Is this span of time larger than this other one?"
so it seems like a reasonable thing to do. As part of the fix, I also removed the
null check in the next patch.

try run: https://tbpl.mozilla.org/?tree=Try&rev=892582b078f9
Attachment #8351225 - Flags: review?(bugs)
Attachment #8351225 - Flags: review?(bugs) → review+
So this bug caused a regression in the GC API given that we now have a more precise timing available. Instead of milliseconds we are getting microseconds now. We discovered that changed behavior with memchaser and for now it is tracked in issue https://github.com/mozilla/memchaser/issues/184.

Andrew and Olli, is that an expected change or shall we hide that behind the API?
Flags: needinfo?(continuation)
Flags: needinfo?(bugs)
Isn't it actually bug 948554 that caused the regression?
Flags: needinfo?(continuation)
Flags: needinfo?(bugs)
Oh, sorry, that's this bug.  I got confused.  No, that's not expected behavior.
(In reply to Andrew McCreight [:mccr8] from comment #14)
> Oh, sorry, that's this bug.  I got confused.  No, that's not expected
> behavior.

Ok, so I will go ahead and file a new bug for that in a bit. Thanks for your feedback.
You need to log in before you can comment on or make changes to this bug.