Record max incremental cycle collector slice pause time

RESOLVED FIXED in mozilla29

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

Trunk
mozilla29
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

Comment hidden (empty)
(Assignee)

Comment 2

5 years ago
Created attachment 8346009 [details] [diff] [review]
part 1 - Rename PrepareForCycleCollection and make it a method.

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)
(Assignee)

Comment 3

5 years ago
Created attachment 8346011 [details] [diff] [review]
part 2 - Convert some of nsJSEnvironment to TimeStamp for greater precision.

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)
(Assignee)

Comment 4

5 years ago
Created attachment 8346012 [details] [diff] [review]
part 3 - Capture max slice time in logs and telemetry.
Attachment #8346012 - Flags: review?(bugs)

Updated

5 years ago
Attachment #8346009 - Flags: review?(bugs) → review+

Updated

5 years ago
Attachment #8346011 - Flags: review?(bugs) → review+

Comment 5

5 years ago
(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.

Updated

5 years ago
Attachment #8346012 - Flags: review?(bugs) → review+
(Assignee)

Updated

5 years ago
Blocks: 939928
(Assignee)

Comment 6

5 years ago
(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.
(Assignee)

Comment 8

5 years ago
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
(Assignee)

Updated

5 years ago
Blocks: 911246
No longer blocks: 850065
(Assignee)

Comment 9

5 years ago
Created attachment 8351225 [details] [diff] [review]
part 2b - Make TimeUntilNow return 0 for null args.

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)

Updated

5 years ago
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)
(Assignee)

Comment 13

5 years ago
Isn't it actually bug 948554 that caused the regression?
Flags: needinfo?(continuation)
Flags: needinfo?(bugs)
(Assignee)

Comment 14

5 years ago
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.
Depends on: 972856
You need to log in before you can comment on or make changes to this bug.