Closed Bug 1351920 Opened 7 years ago Closed 7 years ago

Make PROFILER_LABEL_DYNAMIC actually cheap

Categories

(Core :: Gecko Profiler, enhancement)

Product:
Core
Component:
Gecko Profiler
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: mstange, Assigned: mstange)

References

Details

Attachments

(2 files)

Bug 1351920 - Check privacy mode during sampling, not during PROFILER_LABEL_DYNAMIC.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1351920 - Remove the nsCString&& version of PROFILER_LABEL_DYNAMIC because it makes misleading promises about performance.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details 
This covers the rest of bug 1339897. The current PROFILER_LABEL_DYNAMIC has two problems:
 1. It checks the profiler privacy mode, which grabs the profiler state lock.
 2. It has alternative constructor that accepts an nsCString&& argument, and that constructor pretends to be cheap. But nsCString doesn't actually have a move constructor (bug 1155457), and if you pass an nsAutoCString (e.g. an NS_LossyConvertUTF8ToAscii), then you still allocate + copy.

With the patches I'm about to attach to this bug, SamplerStackFrameDynamicRAII no longer seems to show up in profiles at all.
Comment on attachment 8852746 [details]
Bug 1351920 - Check privacy mode during sampling, not during PROFILER_LABEL_DYNAMIC.

https://reviewboard.mozilla.org/r/124910/#review127468

::: tools/profiler/core/platform.cpp:437
(Diff revision 1)
>    int lineno = -1;
>  
>    // First entry has kind CodeLocation. Check for magic pointer bit 1 to
>    // indicate copy.
>    const char* sampleLabel = entry.label();
> -  const char* dynamicString = entry.getDynamicString();
> +  bool includeDynamicString = !aIsInPrivacyMode;

The alternative is to use gPS->FeaturePrivacy(aLock) here, which would require passing aLock as the first argument everywhere instead of aIsInPrivacyMode as the last argument. I would be inclined to do that because it makes it clear that gPSMutex is locked when all these functions are called, which is nice. But I'll let you decide.

::: tools/profiler/core/platform.cpp:443
(Diff revision 1)
> +  const char* dynamicString =
> +    includeDynamicString ? entry.getDynamicString() : nullptr;
>    char combinedStringBuffer[SAMPLER_MAX_STRING_LENGTH];
>  
>    if (entry.isCopyLabel() || dynamicString) {
>      if (dynamicString) {

You did this check differently in profiler_get_backtrace_noalloc() -- there you unconditionally got the dynamic string, and then checked includeDynamicString in this condition. Might as well make both of them the same. (Either way is fine.)

::: tools/profiler/core/platform.cpp:3004
(Diff revision 1)
> +  bool includeDynamicString = true;
> +
> +  {
> +    PS::AutoLock lock(gPSMutex);
> +    includeDynamicString = !gPS->FeaturePrivacy(lock);
> +  }

Move the includeDynamicString declaration and AutoLock block after the tlsPseudoStack check below.

::: tools/profiler/public/GeckoProfiler.h
(Diff revision 1)
>  
>  private:
>    void* Enter(const char* aInfo, js::ProfileEntry::Category aCategory,
>                uint32_t aLine, const char* aDynamicString)
>    {
> -    if (profiler_is_active_and_not_in_privacy_mode()) {

This is the only use of profiler_is_active_and_not_in_privacy_mode(). Remove it!

::: tools/profiler/public/GeckoProfiler.h:523
(Diff revision 1)
>  private:
>    void* Enter(const char* aInfo, js::ProfileEntry::Category aCategory,
>                uint32_t aLine, const char* aDynamicString)
>    {
> -    if (profiler_is_active_and_not_in_privacy_mode()) {
> -      return profiler_call_enter(aInfo, aCategory, this, true, aLine, aDynamicString);
> +    return profiler_call_enter(aInfo, aCategory, this, true, aLine, aDynamicString);

I think the value of the aCopy argument doesn't matter when a dynamic string is given -- is that right? (Not a comment, just a question.)

I just noticed that one of the args to profiler_call_enter() is aAnnotationString. It should be aDynamicString. Might as well change it in this patch.
Attachment #8852746 - Flags: review?(n.nethercote) → review+
Comment on attachment 8852747 [details]
Bug 1351920 - Remove the nsCString&& version of PROFILER_LABEL_DYNAMIC because it makes misleading promises about performance.

https://reviewboard.mozilla.org/r/124912/#review127472
Attachment #8852747 - Flags: review?(n.nethercote) → review+
Comment on attachment 8852746 [details]
Bug 1351920 - Check privacy mode during sampling, not during PROFILER_LABEL_DYNAMIC.

https://reviewboard.mozilla.org/r/124910/#review127468

> I think the value of the aCopy argument doesn't matter when a dynamic string is given -- is that right? (Not a comment, just a question.)
> 
> I just noticed that one of the args to profiler_call_enter() is aAnnotationString. It should be aDynamicString. Might as well change it in this patch.

Yes, that is right.
Pushed by mstange@themasta.com:
https://hg.mozilla.org/integration/autoland/rev/a78abee146ac
Check privacy mode during sampling, not during PROFILER_LABEL_DYNAMIC. r=njn
https://hg.mozilla.org/integration/autoland/rev/50740b6e2106
Remove the nsCString&& version of PROFILER_LABEL_DYNAMIC because it makes misleading promises about performance. r=njn
https://hg.mozilla.org/mozilla-central/rev/a78abee146ac
https://hg.mozilla.org/mozilla-central/rev/50740b6e2106
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.