Last Comment Bug 721794 - Add SAMPLE_LABEL for cycle collector entry points
: Add SAMPLE_LABEL for cycle collector entry points
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla13
Assigned To: Andrew McCreight [:mccr8]
: Nathan Froyd [:froydnj]
Depends on:
Blocks: 705582 713227
  Show dependency treegraph
Reported: 2012-01-27 10:15 PST by Andrew McCreight [:mccr8]
Modified: 2012-02-01 06:00 PST (History)
5 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

add SPS labels for all major CC entry points (2.07 KB, patch)
2012-01-27 14:15 PST, Andrew McCreight [:mccr8]
bugs: review+
Details | Diff | Splinter Review

Description Andrew McCreight [:mccr8] 2012-01-27 10:15:34 PST
This will get called more with bug 721543, so we should get coverage of it in the SPS.
Comment 1 Andrew McCreight [:mccr8] 2012-01-27 14:15:34 PST
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.
Comment 2 Olli Pettay [:smaug] 2012-01-27 15:32:22 PST
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 Andrew McCreight [:mccr8] 2012-01-27 15:44:49 PST
(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 Olli Pettay [:smaug] 2012-01-27 15:48:09 PST
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 Andrew McCreight [:mccr8] 2012-01-27 15:53:44 PST
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 Benjamin Smedberg [:bsmedberg] 2012-01-30 10:25:15 PST
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?
Comment 7 Benoit Girard (:BenWa) 2012-01-30 11:01:03 PST
That's a good idea. I'll get a patch ready to apply this to the whole project.
Comment 8 Andrew McCreight [:mccr8] 2012-01-30 11:17:08 PST
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.
Comment 9 Andrew McCreight [:mccr8] 2012-01-31 13:48:42 PST
Comment 10 Andrew McCreight [:mccr8] 2012-02-01 06:00:57 PST

Note You need to log in before you can comment on or make changes to this bug.