Closed Bug 1405541 Opened 2 years ago Closed 2 years ago

Split AUTO_PROFILER_LABEL_DYNAMIC into three macros

Categories

(Core :: Gecko Profiler, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: njn, Assigned: njn)

Details

Attachments

(2 files, 1 obsolete file)

It's easy to mess up the scoping so that (a) the label is pushed and then
immediately popped, and/or (b) the string doesn't live long enough. It's also
easy to do a utf16-to-utf8 conversion unnecessarily when the profiler is
inactive.

This patch splits that macro into three new ones that are harder to mess up.

- AUTO_PROFILER_LABEL_DYNAMIC_CSTR: same as current.
- AUTO_PROFILER_LABEL_DYNAMIC_NSCSTRING: for nsCStrings.
- AUTO_PROFILER_LABEL_DYNAMIC_LOSSY_NSSTRING: for nsStrings.
Attachment #8915003 - Flags: review?(mstange)
Comment on attachment 8915003 [details] [diff] [review]
Split AUTO_PROFILER_LABEL_DYNAMIC into three macros

Review of attachment 8915003 [details] [diff] [review]:
-----------------------------------------------------------------

::: tools/profiler/public/GeckoProfiler.h
@@ +447,5 @@
> +#define AUTO_PROFILER_LABEL_DYNAMIC_LOSSY_NSSTRING(label, category, nsStr) \
> +  mozilla::Maybe<NS_LossyConvertUTF16toASCII> asciiStr; \
> +  if (profiler_is_active()) { \
> +    asciiStr.emplace(nsStr); \
> +    mozilla::AutoProfilerLabel \

This still has the trivial scope problem. Were you planning to address that in a future patch?
Attachment #8915003 - Flags: review?(mstange) → review+
> This still has the trivial scope problem. Were you planning to address that
> in a future patch?

Ugh, good catch. I guess I need another Maybe<> and another emplace().
This time with a correct scope for the AutoProfilerLabel :)
Attachment #8917188 - Flags: review?(mstange)
Attachment #8915003 - Attachment is obsolete: true
Attachment #8917188 - Flags: review?(mstange) → review+
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/759a43ebc6bf
Split AUTO_PROFILER_LABEL_DYNAMIC into three macros. r=mstange.
The Windows failures are due to malformed JSON in the output profile. I did a try push with a debugging printf on the profile and saw that I had a corrupted dynamic string for a "PresShell::DoReflow" label. The problematic code is in layout/base/PresShell.cpp:

> #ifdef MOZ_GECKO_PROFILER
>   nsIURI* uri = mDocument->GetDocumentURI();
>   AUTO_PROFILER_LABEL_DYNAMIC_NSCSTRING(
>     "PresShell::DoReflow", GRAPHICS,
>     uri ? uri->GetSpecOrDefault() : NS_LITERAL_CSTRING("N/A"));
> #endif

GetSpecOrDefault() is in netwerk/base/nsIURI.idl:

> nsCString GetSpecOrDefault()
> {
>     nsCString spec;
>     nsresult rv = GetSpec(spec);
>     if (NS_FAILED(rv)) {
>         spec.AssignLiteral("[nsIURI::GetSpec failed]");
>     }
>     return spec;
> }

And here's the macro being used:

> #define AUTO_PROFILER_LABEL_DYNAMIC_NSCSTRING(label, category, nsCStr) \
>   const nsCString& promiseFlatCStr = PromiseFlatCString(nsCStr); \
>   mozilla::AutoProfilerLabel \
>     PROFILER_RAII(label, promiseFlatCStr.get(), __LINE__, \
>                   js::ProfileEntry::Category::category);

I'm not sure what's going wrong. As I understand it:

1. GetSpecOrDefault returns `spec` by value, which invokes the copy constructor, which does an Assign().

2. The `const nsCString&` in the macro ensures that the returned nsCString lives as long as the AutoProfilerLabel does.

So it should work. But I must be overlooking something. In step 1 the compiler is allowed to optimize away the copy constructor call, and I'm not sure what effect that might have.

mstange, any thoughts?
Flags: needinfo?(mstange)
Jeff found the bug. It's not the PromiseFlatCString object that's going away, it's nsCStr.

Here's an excerpt from the PromiseFlatCString documentation at http://searchfox.org/mozilla-central/source/xpcom/string/nsTPromiseFlatString.h#31-33 :

> A |nsPromiseFlat[C]String| doesn't necessarily own the characters it
> promises.  You must never use it to promise characters out of a string
> with a shorter lifespan. 

But we call it with a temporary.
Flags: needinfo?(mstange)
I changed AUTO_PROFILER_LABEL_DYNAMIC_NSCSTRING to be more like
AUTO_PROFILER_LABEL_DYNAMIC_LOSSY_NSSTRING, i.e. use Maybe<>.

But now I'm wondering if the lack of pushing when the profile is inactive is a
problem. Because we could end up with surprising gaps in the pseudostack. Would
it be better to just unconditionally push and accept the cost of the
assignment/conversion?
Attachment #8919527 - Flags: review?(mstange)
Comment on attachment 8919527 [details] [diff] [review]
(attempt 2) - Split AUTO_PROFILER_LABEL_DYNAMIC into three macros

Review of attachment 8919527 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks.

I don't think the gaps in the pseudostacks will be a problem. They only happen for functions that are on the callstack at the time when the profiler is started. So this will only occur in the samples at the start of your profile, unless you have a long hang with a deep callstack and you only started the profiler when the hang was already happening.
Attachment #8919527 - Flags: review?(mstange) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/80b59c38632ec62fe13de49db3c9e27611e2f6a0
Bug 1405541 (attempt 2) - Split AUTO_PROFILER_LABEL_DYNAMIC into three macros. r=mstange.
https://hg.mozilla.org/mozilla-central/rev/80b59c38632e
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.