1351920 - Make PROFILER_LABEL_DYNAMIC actually cheap

Status: RESOLVED FIXED

(Core :: Gecko Profiler, enhancement)

Description

[Markus Stange [:mstange]]

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 1

[Markus Stange [:mstange]]

Attachment: Bug 1351920 - Check privacy mode during sampling, not during PROFILER_LABEL_DYNAMIC.

When the profiler is running in privacy mode, we don't want to include dynamic
strings from PROFILER_LABEL_DYNAMIC to end up in the profile.
Rather than checking this every time we enter a scope marked with
PROFILER_LABEL_DYNAMIC, with this patch we will push the dynamic string into
the pseudo stack entry regardless, and then check the privacy mode during
sampling and ignore the dynamic string as necessary.
This way we can avoid taking the profiler state lock in PROFILER_LABEL_DYNAMIC
and also save a branch.

Review commit: https://reviewboard.mozilla.org/r/124910/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/124910/

Comment 2

[Markus Stange [:mstange]]

Attachment: Bug 1351920 - Remove the nsCString&& version of PROFILER_LABEL_DYNAMIC because it makes misleading promises about performance.

Review commit: https://reviewboard.mozilla.org/r/124912/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/124912/

Comment 3

[Nicholas Nethercote [inactive]]

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.

Comment 4

[Nicholas Nethercote [inactive]]

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

Comment 5

[Markus Stange [:mstange]]

Comment on attachment 8852746 [details]
Bug 1351920 - Check privacy mode during sampling, not during PROFILER_LABEL_DYNAMIC.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124910/diff/1-2/

Comment 6

[Markus Stange [:mstange]]

Comment on attachment 8852747 [details]
Bug 1351920 - Remove the nsCString&& version of PROFILER_LABEL_DYNAMIC because it makes misleading promises about performance.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124912/diff/1-2/

Comment 7

[Markus Stange [:mstange]]

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.

Comment 8

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s bf75a2673958 -d f62917e98036: rebasing 386102:bf75a2673958 "Bug 1351920 - Check privacy mode during sampling, not during PROFILER_LABEL_DYNAMIC. r=njn"
merging tools/profiler/core/platform.cpp
merging tools/profiler/public/GeckoProfiler.h
rebasing 386103:f095d75ddfde "Bug 1351920 - Remove the nsCString&& version of PROFILER_LABEL_DYNAMIC because it makes misleading promises about performance. r=njn" (tip)
merging layout/base/PresShell.cpp
merging tools/profiler/public/GeckoProfiler.h
warning: conflicts while merging tools/profiler/public/GeckoProfiler.h! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 9

[Markus Stange [:mstange]]

Comment on attachment 8852746 [details]
Bug 1351920 - Check privacy mode during sampling, not during PROFILER_LABEL_DYNAMIC.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124910/diff/2-3/

Comment 10

[Markus Stange [:mstange]]

Comment on attachment 8852747 [details]
Bug 1351920 - Remove the nsCString&& version of PROFILER_LABEL_DYNAMIC because it makes misleading promises about performance.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124912/diff/2-3/

Comment 11

[Markus Stange [:mstange]]

Comment on attachment 8852746 [details]
Bug 1351920 - Check privacy mode during sampling, not during PROFILER_LABEL_DYNAMIC.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124910/diff/3-4/

Comment 12

[Markus Stange [:mstange]]

Comment on attachment 8852747 [details]
Bug 1351920 - Remove the nsCString&& version of PROFILER_LABEL_DYNAMIC because it makes misleading promises about performance.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124912/diff/3-4/

Comment 13

[Pulsebot]

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

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a78abee146ac
https://hg.mozilla.org/mozilla-central/rev/50740b6e2106