721794 - Add SAMPLE_LABEL for cycle collector entry points

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

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

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: 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][[e-mail]]]

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]]

(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][[e-mail]]]

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]]

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]

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)]

That's a good idea. I'll get a patch ready to apply this to the whole project.

Comment 8

[Andrew McCreight [:mccr8]]

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]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9925881ddaa1

Comment 10

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/mozilla-central/rev/9925881ddaa1