Add SAMPLE_LABEL for cycle collector entry points

RESOLVED FIXED in mozilla13

Status

()

Core
XPCOM
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

Trunk
mozilla13
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
This will get called more with bug 721543, so we should get coverage of it in the SPS.
(Assignee)

Updated

6 years ago
Summary: Add SAMPLE_LABEL for forgetSkippable → Add SAMPLE_LABEL for cycle collector entry points
(Assignee)

Comment 1

6 years ago
Created attachment 592267 [details] [diff] [review]
add SPS labels for all major CC entry points

This covers all the major entry points, eg those that take any time at all.  In my testing, forgetSkippable takes less than a ms, but it isn't only called every 250ms, so I don't imagine the extra overhead of a label will be too bad.  I want to add it to the profiler in case it could become a large source of time under some circumstances.  I haven't tested this yet, but I can't imagine any problems.
Assignee: nobody → continuation

Comment 2

6 years ago
Sounds ok to me. forgetSkippable isn't particularly hot code.

But, is it ok to #include "sampler.h" in xpcom?
Should it at least be #ifdef'ed?
(Assignee)

Comment 3

6 years ago
(In reply to Olli Pettay [:smaug] from comment #2)
> But, is it ok to #include "sampler.h" in xpcom?
> Should it at least be #ifdef'ed?

Why wouldn't it be okay?  It is already used in one place in xpcom, in xpcom/threads/nsTimerImpl.cpp  I'm not sure what you mean about being #ifdef'ed?

Comment 4

6 years ago
Because xpcom can be used in code which doesn't use tools/
I have no idea why it would be ok to #include sampler.h in xpcom by default.
Benjamin might know.
(Assignee)

Comment 5

6 years ago
Ah, that's interesting.  In the worst case I can lift this out to nsJSEnvironment or whatnot, but there are a few callsites, and we'd have to remember to add the label to any new ones, which is why I was doing it like this.

Comment 6

6 years ago
Apart from the glue, which is special, this can rely on the profiler, that's not a problem. However, I really think that the header name "sampler.h" is very generic... can we make it mozilla/sampler.h as with most of our other new headers?
That's a good idea. I'll get a patch ready to apply this to the whole project.
(Assignee)

Comment 8

6 years ago
Comment on attachment 592267 [details] [diff] [review]
add SPS labels for all major CC entry points

Sounds like maybe this is okay then?  Just r- if I'm misunderstanding.
Attachment #592267 - Flags: review?(bugs)

Updated

6 years ago
Attachment #592267 - Flags: review?(bugs) → review+
(Assignee)

Comment 9

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/9925881ddaa1
Target Milestone: --- → mozilla13
(Assignee)

Comment 10

6 years ago
https://hg.mozilla.org/mozilla-central/rev/9925881ddaa1
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.