The default bug view has changed. See this FAQ.

Add SAMPLE_LABEL for cycle collector entry points

RESOLVED FIXED in mozilla13



5 years ago
5 years ago


(Reporter: mccr8, Assigned: mccr8)


Dependency tree / graph

Firefox Tracking Flags

(Not tracked)



(1 attachment)



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


5 years ago
Summary: Add SAMPLE_LABEL for forgetSkippable → Add SAMPLE_LABEL for cycle collector entry points

Comment 1

5 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
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?

Comment 3

5 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?
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.

Comment 5

5 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.
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.

Comment 8

5 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)
Attachment #592267 - Flags: review?(bugs) → review+

Comment 9

5 years ago
Target Milestone: --- → mozilla13

Comment 10

5 years ago
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.