If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Make Restyle label splitted based on element id and log the callstack from javascript if available

RESOLVED FIXED in Firefox 39

Status

()

Core
Gecko Profiler
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: chiajung, Assigned: chiajung)

Tracking

Trunk
mozilla39
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(2 attachments, 15 obsolete attachments)

2.61 KB, patch
chiajung
: review+
Details | Diff | Splinter Review
5.80 KB, patch
chiajung
: review+
Details | Diff | Splinter Review
(Assignee)

Description

3 years ago
Created attachment 8558869 [details] [diff] [review]
WIP1

While debugging performance issues respect to Restyle with GeckoProfiler, we usually see large "Style #" chunk, and the chunk has a stack.

However, we usually have several pending restyles in 1 Restyle batch, and the stack is usually not enough to reveal each style change's effort, and which Restyle takes most time.

Here is a example output of the proposal:
http://people.mozilla.org/~bgirard/cleopatra/#report=a65b87cc3619d160d569ccf8f2205940a9cc93c5&filter=%5B{%22type%22%3A%22RangeSampleFilter%22,%22start%22%3A2974,%22end%22%3A3468},{%22type%22%3A%22RangeSampleFilter%22,%22start%22%3A3265,%22end%22%3A3427}%5D&selection=0,1,8,206,207,298,338

In this profile, the Style#1 takes 117ms, and the stack inside shows aw__changeState. In fact, there are 5 Restyles: statusbar: 37 ms, homescreen: 30 ms, appWindow: 20ms. And the aw__changeState is associated to appWindow.

My WIP needs cleanup for unused part, and need a better way to keep JSStack.
Currently, I use property to protect it, because it can cause memory pressure in some case. And I only logged for RestyleRoots, which is sometime not enough.
I would like to add some more information for PendingRestyles and self expended restyles.
(Assignee)

Updated

3 years ago
Assignee: nobody → chung
(Assignee)

Comment 1

3 years ago
Created attachment 8560364 [details] [diff] [review]
WIP v2

Sample output of WIP v2:
http://people.mozilla.org/~bgirard/cleopatra/#report=e17e8407dee9b94fdf6bef378ae25f77489468ef&filter=%5B{%22type%22%3A%22RangeSampleFilter%22,%22start%22%3A4011,%22end%22%3A4210}%5D&selection=0,1,252,253,254,364,382

In this profile, we can see 5 Restyles during [4104,4154] and each label has a JSStack in tool tip.

I want to make the profiler_label contain JSStack, too(Since we sometime have restyle but not Style graph). However, ProfilerBacktrace::StreamJSObject seems clear the content of the backtrack and make tooltip wrong. 

And we may need a way to log several JSStack at same time, since some JS change style properties 1-by-1 and make it shows only 1st stack. (Because each RestyleRoot restyles only once even multiple modification, and no way to merge 2 ProfilerBacktrace into 1.)

Furthermore, sometimes a StyleRoot can be expanded into several PendingRestyles(if Restyle_LaterSibling), we may need a way to show this information on GeckoProfiler, too. (This need the ability to serialize a ProfileBacktrace multiple times, too)
Attachment #8558869 - Attachment is obsolete: true
(Assignee)

Comment 2

3 years ago
Created attachment 8561274 [details] [diff] [review]
WIP v3

I can not stream the ProfileBacktrace object twice, but the output contains some redundant data for PROFILER_LABEL.

I think I should prune the data or create a simple function to create a clean stack.
Attachment #8560364 - Attachment is obsolete: true
(Assignee)

Comment 3

3 years ago
Created attachment 8561819 [details] [diff] [review]
v1

I finally managed to dump the stack both on graph and on profiler labels.
We may need to extend 
https://dxr.mozilla.org/mozilla-central/source/tools/profiler/GeckoProfilerImpl.h#382
to make the label complete while debug.

In this patch I decided to skip several frames in label while they are rather noisy. And I think we can record style change(old/new) and animation styles(some app forget to remove invisible animations and just waste CPU time) in another bug.
Attachment #8561274 - Attachment is obsolete: true
Attachment #8561819 - Flags: review?(bgirard)
I don't think this patch is complete. You add PrintJSStack but it's not used anywhere.
(Assignee)

Comment 5

3 years ago
Oh!? I forgot to remove them...:P

PrintJSStack consumes to much memory and crashes most app while launch(due to OOM), so I decided to use ProfilerBacktrace now.
Comment on attachment 8561819 [details] [diff] [review]
v1

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

This patch is very interesting but it needs to be cleaner before we can land it.

::: layout/base/RestyleManager.cpp
@@ +3504,2 @@
>    nsIContent* content = aFrame->GetContent();
> +  nsAutoCString idStr;

This code will do a lot of computation when the profiler is disabled.

@@ +3514,5 @@
> +
> +  std::ostringstream outstream;
> +  if (bt.get() != nullptr) {
> +    JSStreamWriter writer(outstream);
> +    bt->StreamJSObject(writer, false);

This is a hack, we should add a proper API for this. See https://bugzilla.mozilla.org/show_bug.cgi?id=1123237#c41 where the memory profiler will also need the same API.

@@ +3521,5 @@
> +  profiler_tracing("Paint", "Styles", bt.forget(), TRACING_INTERVAL_START);  // Since profiler_tracing release the Backtrace
> +                                                                          // We must not release it here
> +  std::string stack = outstream.str();
> +  size_t pos = stack.find("RunScript");
> +  if (pos != std::string::npos)

if (...) {
  ...
}

@@ +3525,5 @@
> +  if (pos != std::string::npos)
> +    pos = stack.find("\"location", pos);
> +  PROFILER_LABEL_PRINTF("ElementRestyler", "ComputeStyleChangeFor",
> +      js::ProfileEntry::Category::CSS, "Element: %s, Stack: %s",
> +      content && content->GetID() ? idStr.get() : "{null}", pos != std::string::npos ? stack.data()+pos : "{null}");

I think showing stack: null is confusing. Perhaps we should show (Native) instead? I believe this will show when you don't have any JS execution triggering the frame.

stack.data()+pos should be stack.data() + pos. Coding style has spaces around the operators.

::: layout/base/RestyleTracker.h
@@ +382,4 @@
>    // flag.  We need this to avoid enumerating the hashtable looking
>    // for such entries when we can't possibly have any.
>    bool mHaveLaterSiblingRestyles;
> +//#ifdef RESTYLE_LOGGING

remove the ifdef

@@ +382,5 @@
>    // flag.  We need this to avoid enumerating the hashtable looking
>    // for such entries when we can't possibly have any.
>    bool mHaveLaterSiblingRestyles;
> +//#ifdef RESTYLE_LOGGING
> +  typedef nsClassHashtable<nsISupportsHashKey, ProfilerBacktrace> PendingRestyleSource;

Maybe this should work like the other ProfilerBacktrace* restyle causes. Make this a ProfilerBacktrace* and you should be able to remove the .forget(). I don't think ProfilerBacktrace supports being copy constructed.

Otherwise we can make sure that ProfilerBacktrace can be copy constructed.

::: tools/profiler/ProfileEntry.cpp
@@ -525,4 @@
>    StreamJSObject(b);
>  }
>  
> -void ThreadProfile::StreamJSObject(JSStreamWriter& b)

remove all the profiler changes they are not needed for this.
Attachment #8561819 - Flags: review?(bgirard) → review-
Created attachment 8564017 [details] [diff] [review]
Part 1: Expose the element id
We discussed this in person:
- We're going to split out 1) the part where we get the node name, 2) the part where we get a restyle cause.
- We're going to put part 2) behind a profiler feature since it's more costly to collect.

Updated

3 years ago
Attachment #8564017 - Flags: review?(dholbert)
Attachment #8564017 - Flags: feedback?(chung)
Comment on attachment 8564017 [details] [diff] [review]
Part 1: Expose the element id

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

You should expose the restyle hint as well
(Assignee)

Comment 10

3 years ago
Comment on attachment 8564017 [details] [diff] [review]
Part 1: Expose the element id

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

This is basically the same patch of mine.
In this part, we simply log element id (if available) in profiler label, and split the Style bar into smaller chunks.
Attachment #8564017 - Flags: feedback+
(Assignee)

Updated

3 years ago
Attachment #8564017 - Flags: feedback?(chung) → feedback+
Comment on attachment 8564017 [details] [diff] [review]
Part 1: Expose the element id

(Sorry for the few days of delay here -- I was out on Friday and Monday, and I'm catching up on reviews now.)

># Node ID f76ef9f7800919c91b94d5a639025d9b1cbb20d5
># Parent  2f5c5ec1a24b9da27ee21e737239289f0f7105ec
>[PATCH] Bug 1129249 - Make Restyle label splitted based on element id

I don't quite understand what this commit message is saying -- needs some clarification.

Maybe it means to say something like "Include element ID in profiler logging for restyle events"?

>+++ b/layout/base/RestyleManager.cpp
>@@ -3494,20 +3494,32 @@ ElementRestyler::RestyleChildrenOfDispla
> 
> void
> ElementRestyler::ComputeStyleChangeFor(nsIFrame*          aFrame,
>                                        nsStyleChangeList* aChangeList,
>                                        nsChangeHint       aMinChange,
>                                        RestyleTracker&    aRestyleTracker,
>                                        nsRestyleHint      aRestyleHint)
> {
>   nsIContent* content = aFrame->GetContent();
[...]
>+  nsAutoCString idStr;
>+  if (content) {
>+    nsIAtom * id = content->GetID();
>+    if (id)
>+      id->ToUTF8String(idStr);
>+  }

Two stylistic nits:
 1) s/nsIAtom * id/nsIAtom* id/
 2) Add braces around one-line "if" statement.

And one more important functional nit:
It looks like we're doing this string-copying *unconditionally* here, regardless of whether the profiler is running.  That's a bit wasteful, since 99.99% of the time that this is called in the real world, we won't be profiling.  Can we wrap this whole chunk in a "if you are actually profiling" check of some sort? (e.g. maybe this should be inside of #ifndef MOZ_ENABLE_PROFILER_SPS -- but that's probably still not enough, since (I think?) that flag will be on by default in our release builds, and the fact remains that those builds aren't going to be using this output 99.99% of the time.)

>+  const char* elemName = "?";
>+  if (content && content->GetID()) {
>+    elemName = idStr.get();
>+  }

Two things:
 1) This re-check of "content && content->GetId()" seems redundant. (We already checked those things above, where we initialized idStr.) Seems like it'd be cleaner to move the "elemName" decl up top, to where we declare "idStr", and then set it to idStr.get() in the same clause where we populate idStr.

 2) the naming difference between "elemName" vs. "idStr" don't make sense to me, since these variables represent the same thing AFAICT. Maybe call them "elemIdStr" and "elemIDCharPtr"?  (or something along those lines, to make it clearer that they're the same.)

>+  profiler_tracing("Paint", "Styles", TRACING_INTERVAL_START);
>+  PROFILER_LABEL_PRINTF("ElementRestyler", "ComputeStyleChangeFor",
>+      js::ProfileEntry::Category::CSS, "Element: %s", elemName);

(Why "Paint" here? We're not necessarily painting... Though I guess we have "Paint" everywhere.)

>+        profiler_tracing("Paint", "Styles", TRACING_INTERVAL_END);
>         return;
>       }
>     }
>   }
>+  profiler_tracing("Paint", "Styles", TRACING_INTERVAL_END);

Do we need this call at every return path? (looks like it)

I'd much prefer we use an RAII guard object to set this & automatically clean it up when we return, so that we don't need to boilerplate & we don't get bugs from forgetting to add this call at future return-paths.
Attachment #8564017 - Attachment description: Part 1: Expose the element name → Part 1: Expose the element id
(In reply to Daniel Holbert [:dholbert] from comment #11)
> (Why "Paint" here? We're not necessarily painting... Though I guess we have
> "Paint" everywhere.)

(by "everywhere" I meant "in almost all other calls to profiler_tracing"; so I guess it's reasonable enough to have it here as well. I couldn't find documentation for profiler_tracing, so I don't really know what "Paint" means in this context; is it saying "We're beginning a thing that might trigger a paint"?)
Comment on attachment 8564017 [details] [diff] [review]
Part 1: Expose the element id

(r- for now, per comment 11)
Attachment #8564017 - Flags: review?(dholbert) → review-
Do you want to address these comments chiajung since it's your patch?
Flags: needinfo?(chung)
(Assignee)

Comment 15

3 years ago
(In reply to Benoit Girard (:BenWa) from comment #14)
> Do you want to address these comments chiajung since it's your patch?

Sure, I can fix them :)
Flags: needinfo?(chung)
(Assignee)

Comment 16

3 years ago
Created attachment 8568398 [details] [diff] [review]
part 1 (v2)

This is part 1, so I removed all confusing things.
To avoid cost, I add a new profiler feature -- restyle. This patch only affect code behavior when enable the feature now.
Attachment #8561819 - Attachment is obsolete: true
Attachment #8564017 - Attachment is obsolete: true
Attachment #8568398 - Flags: review?(dholbert)
Attachment #8568398 - Flags: review?(bgirard)
Comment on attachment 8568398 [details] [diff] [review]
part 1 (v2)

(I'm just reviewing the RestyleManager code here; I'm assuming BenWa will take the profiler code.)

>+  const char* elemName = nullptr;
>+  nsAutoCString idStr;
>+  if (profiler_feature_active("restyle") && content) {
>+    elemName = "?";
>+    nsIAtom* id = content->GetID();
>+    if (id) {
>+      id->ToUTF8String(idStr);
>+      elemName = idStr.get();
>+    }
>+  }

As noted in comment 11, the naming-difference between elemName vs. idStr doesn't make sense to me. (When elemName contains anything meaningful, it's the ID; so it seems like it should have "id" in its variable name.)

Also: I worry slightly that someone might come along and "optimize" the idStr declaration to be inside of the "if" statements, since that's where it's directly used -- which would be bad because it'd leave a dangling pointer to freed stack-memory in elemName, after idStr goes out of scope.

To address these concerns, could you:
 (1) Get rid of elemName
 (2) Drop the elemName =  "?"; line, and replace it with an "else { idStr.AssignLiteral("?"); }, after the "if (id)" check.
 (3) Replace all elemName null-checks with 'content' null-checks, and replace elemName in the PROFILER_LABEL_PRINTF call with idStr.get().

>+  PROFILER_LABEL_PRINTF("ElementRestyler", "ComputeStyleChangeFor",
>+      js::ProfileEntry::Category::CSS, elemName ? "Element: %s" : "%s", elemName ? elemName : "");

I'd prefer a newline after each comma here, to make this more readable. Two ternary statements on a single line makes for pretty hard-to-read code.

So this would end up as:
  PROFILER_LABEL_PRINTF("ElementRestyler", "ComputeStyleChangeFor",
                        js::ProfileEntry::Category::CSS,
                        content ? "Element: %s" : "%s",
                        content ? idStr.get() : "");

r=me with that
Attachment #8568398 - Flags: review?(dholbert) → review+
OS: Linux → All
Hardware: x86_64 → All
Version: unspecified → Trunk
Comment on attachment 8568398 [details] [diff] [review]
part 1 (v2)

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

::: layout/base/RestyleManager.cpp
@@ +3524,4 @@
>    nsIContent* content = aFrame->GetContent();
> +  const char* elemName = nullptr;
> +  nsAutoCString idStr;
> +  if (profiler_feature_active("restyle") && content) {

IMO just checking if profiling is enabled should be sufficient here. This is a tad costly but not greatly so. I think this should be always on.

Now the remaining changes you have should be behind this "restyle" feature so you should keep the platform.cpp changes in this patch for part 2.
Attachment #8568398 - Flags: review?(bgirard) → review+
(Assignee)

Comment 19

3 years ago
(In reply to Daniel Holbert [:dholbert] from comment #17)
> Comment on attachment 8568398 [details] [diff] [review]
> part 1 (v2)
> 
> (I'm just reviewing the RestyleManager code here; I'm assuming BenWa will
> take the profiler code.)
> 
> >+  const char* elemName = nullptr;
> >+  nsAutoCString idStr;
> >+  if (profiler_feature_active("restyle") && content) {
> >+    elemName = "?";
> >+    nsIAtom* id = content->GetID();
> >+    if (id) {
> >+      id->ToUTF8String(idStr);
> >+      elemName = idStr.get();
> >+    }
> >+  }
> 
> As noted in comment 11, the naming-difference between elemName vs. idStr
> doesn't make sense to me. (When elemName contains anything meaningful, it's
> the ID; so it seems like it should have "id" in its variable name.)
> 
> Also: I worry slightly that someone might come along and "optimize" the
> idStr declaration to be inside of the "if" statements, since that's where
> it's directly used -- which would be bad because it'd leave a dangling
> pointer to freed stack-memory in elemName, after idStr goes out of scope.
> 
> To address these concerns, could you:
>  (1) Get rid of elemName
>  (2) Drop the elemName =  "?"; line, and replace it with an "else {
> idStr.AssignLiteral("?"); }, after the "if (id)" check.
>  (3) Replace all elemName null-checks with 'content' null-checks, and
> replace elemName in the PROFILER_LABEL_PRINTF call with idStr.get().
> 
> >+  PROFILER_LABEL_PRINTF("ElementRestyler", "ComputeStyleChangeFor",
> >+      js::ProfileEntry::Category::CSS, elemName ? "Element: %s" : "%s", elemName ? elemName : "");
> 
> I'd prefer a newline after each comma here, to make this more readable. Two
> ternary statements on a single line makes for pretty hard-to-read code.
> 
> So this would end up as:
>   PROFILER_LABEL_PRINTF("ElementRestyler", "ComputeStyleChangeFor",
>                         js::ProfileEntry::Category::CSS,
>                         content ? "Element: %s" : "%s",
>                         content ? idStr.get() : "");
> 
> r=me with that
OK
(In reply to Benoit Girard (:BenWa) from comment #18)
> Comment on attachment 8568398 [details] [diff] [review]
> part 1 (v2)
> 
> Review of attachment 8568398 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/RestyleManager.cpp
> @@ +3524,4 @@
> >    nsIContent* content = aFrame->GetContent();
> > +  const char* elemName = nullptr;
> > +  nsAutoCString idStr;
> > +  if (profiler_feature_active("restyle") && content) {
> 
> IMO just checking if profiling is enabled should be sufficient here. This is
> a tad costly but not greatly so. I think this should be always on.
> 
> Now the remaining changes you have should be behind this "restyle" feature
> so you should keep the platform.cpp changes in this patch for part 2.

OK, and since bug 1123237 may not land very soon and I think they are not dependent, I will include some stack stuff in part 2, too.

Updated

3 years ago
Blocks: 1126646
(Assignee)

Comment 20

3 years ago
Created attachment 8569653 [details] [diff] [review]
part 1(final)

Carry r+. Let's land part 1 first.

Try ticket:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=85e0c0b5e529
Attachment #8568398 - Attachment is obsolete: true
Attachment #8569653 - Flags: review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3eb286a2f3f
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e3eb286a2f3f
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
(Assignee)

Comment 23

3 years ago
Created attachment 8571251 [details] [diff] [review]
part2(v1)

Part 2. In this part I store JS cause of restyle in profiler label and split the "Style Bar" on the graph into smaller ones with PseudoStack.

I tried the method in bug 1123237, but all stack labels of JS are js::RunScript, which seems less useful.

BTW, I don't know why pseudo stack of ProfilerBacktrace contains only tons of js::RunScript now.
Attachment #8571251 - Flags: review?(dholbert)
Attachment #8571251 - Flags: review?(bgirard)
Comment on attachment 8571251 [details] [diff] [review]
part2(v1)

>From 5921944a69d08f3b2fa48b430b5b78dbedaf69ee Mon Sep 17 00:00:00 2001
>From: chiajung hung <chung@mozilla.com>
>Date: Mon, 2 Mar 2015 17:30:26 +0800
>Subject: [PATCH] Bug 1129249 - Add a "restyle" feature to GeckoProfiler and
> split the style label in               GeckoProfiler front-end based on the
> restyleSource

This commit message looks a bit mangled (odd whitespace and newlining).

(Maybe a git artifact? Anyway, needs a tweak before this can land to hg.m.o. In particular, the main commit message should all be on a single line.)

>+++ b/layout/base/RestyleManager.cpp
>+class ProfilerTrackingStyleRAII {
>+public:
>+  ProfilerTrackingStyleRAII(ProfilerBacktrace* pb = nullptr) {
>+    profiler_tracing("Paint", "Styles", pb, TRACING_INTERVAL_START);
>+  }
>+  ~ProfilerTrackingStyleRAII() {
>+    profiler_tracing("Paint", "Styles", TRACING_INTERVAL_END);
>+  }

A few things:
 (1) This should be a class that can be used for all profiler_tracing() calls. Its constructor should take 2 const char* args, instead of hardcoding "Paint" & "Styles", and cache the args in const char* member-vars for use in the destructor.  (One minor concern with that: it's conceivable that someone could mess this up by allocating a string, passing it into the constructor, and then deallocating it before the destructor runs. But that's pretty unlikely, given that we generally use hardcoded string-constants for these calls, and those are never deallocated -- and also, this class is stack-allocated/RAII & hence will be auto-destructed before anything declared before it is auto-destructed. So I'm not actually concerned about this problem; someone would have to be *really* trying to shoot themself in the foot to hit it.)

 (2) Given (1), this class should live in a profiler header somewhere, not in RestyleManager.cpp.

 (3) This class needs some macros to enforce that it's *actually used* in an RAII way -- see https://developer.mozilla.org/en-US/docs/Using_RAII_classes_in_Mozilla#Assertions and http://mxr.mozilla.org/mozilla-central/source/mfbt/GuardObjects.h#22 -- in particular, you need a MOZ_STACK_CLASS annotation and the same MOZ_[...]GUARD[...] macros used in nsAutoScriptBlocker in that MDN article.


You'll need to #include "mozilla/Attributes.h" and "mozilla/GuardObjects.h" wherever you define your RAII class, to get these annotations.

I'd also suggest spinning off a helper-bug to create the RAII class & use it everywhere that's appropriate, and then this patch can just layer on top of that & make use of the class.

> void
> ElementRestyler::ComputeStyleChangeFor(nsIFrame*          aFrame,
>                                        nsStyleChangeList* aChangeList,
>@@ -3521,8 +3531,10 @@ ElementRestyler::ComputeStyleChangeFor(nsIFrame*          aFrame,
>                                        RestyleTracker&    aRestyleTracker,
>                                        nsRestyleHint      aRestyleHint)
> {
>+  nsAutoPtr<ProfilerTrackingStyleRAII> profilerRAII;
[...]
>+      profilerRAII = new ProfilerTrackingStyleRAII(rs->mBacktrace);

This won't work with MOZ_STACK_CLASS. Best to never create RAII objects on the heap, since that allows you to use them in a non-RAII way (and have them last forever, accidentally).  In this case nsAutoPtr would make that pretty safe, but still best to prevent that sort of issue altogether.

You really want "Maybe<profilerRAII> profilerRAII;" here, and profilerRAII.emplace(rs->mBacktrace); at the instantiation-point.  (This is similar to nsAutoPtr, except it uses stack memory, so there's nothing to leak and no way to let the RAII thing stick around forever.)

>+  nsAutoPtr<const char> stack;
[...]
>+      stack = rs->mStack;
[...]
>+      rs->mStack = nullptr;

I don't understand what's going on here.

In particular:
 (0) "stack" & "mStack" are probably better named "jsStack", to make it clearer what they refer to. In particular: despite the name, this is not a generalized stack data-structure.
 (1) Your 'stack' variable here is a nsAutoPtr to a *single* const char.  But it looks like mStack (where it gets assigned from) is actually a character *array* (a string). So, 'stack' should perhaps really be a nsAutoArrayPtr? (I don't know where the memory was allocated, & whether it was allocated with "new char", "new char[]", "malloc", or something else, so I don't know what's right.)
 (2) I don't really understand how stack/mStack's lifetime is managed, or why we need to steal it from rs here. Why can't we just trust "rs" to manage this string, and just co-opt it for our usage in this function?  (i.e. why do we need this 'stack' local variable in the first place?)
Attachment #8571251 - Flags: review?(dholbert) → review-
(Assignee)

Comment 25

3 years ago
we have part 2
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 26

3 years ago
Created attachment 8573738 [details] [diff] [review]
part2(v2)

@Jeff, 
Can you help review profiler changes?

@Daniel
I think PrintJSStack() allocate memory by js_malloc, so I use js_free to free it now. There are several places in codebase use profiler_tracing with START/END directly. And some of them live in different scope, for example: [1] only start the profiler_tracing, and end in [2] which can not be replaced with the RAII. I found about 6~7 places can be replaced with it, and since it affect serveral files, I think it derserves a separate bug.

[1]https://dxr.mozilla.org/mozilla-central/source/gfx/layers/client/ClientLayerManager.cpp#254
[2]https://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/ShadowLayers.cpp#670
Attachment #8571251 - Attachment is obsolete: true
Attachment #8571251 - Flags: review?(bgirard)
Attachment #8573738 - Flags: review?(jmuizelaar)
Attachment #8573738 - Flags: review?(dholbert)
(Assignee)

Comment 27

3 years ago
Created attachment 8573741 [details] [diff] [review]
part 2(v2)

Wrong file attached
Attachment #8573738 - Attachment is obsolete: true
Attachment #8573738 - Flags: review?(jmuizelaar)
Attachment #8573738 - Flags: review?(dholbert)
(Assignee)

Updated

3 years ago
Attachment #8573741 - Flags: review?(jmuizelaar)
Attachment #8573741 - Flags: review?(dholbert)
Comment on attachment 8573741 [details] [diff] [review]
part 2(v2)

mstange should be a better reviewer.
Attachment #8573741 - Flags: review?(jmuizelaar) → review?(mstange)
(In reply to Chiajung Hung [:chiajung] from comment #26)
> @Daniel
> I think PrintJSStack() allocate memory by js_malloc, so I use js_free to
> free it now.

So, let's step back -- I'm unclear on how/why we're using PrintJSStack() here, in the first place.

I only see one actual call to PrintJSStack() in our current codebase, in some android debug-logging in PowerManagerService.cpp.  Are you sure it's something we need & want to be using in the profiler?  If so, where is the resulting pretty-printed stack exposed in the profiler? (And why do we need that pretty-printed stack here, but not in any of the pre-existing profiler spots?)

Supposing that we do actually want to be calling PrintJSStack -- I'm not sure if js_free is the right way to free its buffer.  It looks like it's ultimately a wrapper for JS::FormatStackDump() (after several layers of abstraction), and from some brief MXR searching, it looks like we free FormatStackDump()'s result using JS_smprintf_free(): 
http://mxr.mozilla.org/mozilla-central/source/js/src/builtin/TestingFunctions.cpp?rev=cac62569779c&mark=1970-1970,1974-1974#1970
...though JS_smprintf_free is *actually* just a wrapper for js_free. Bleh. Not sure why we bother with that; maybe someone on the JS team knows. But we should probably be consistent about how we free it (and that should be documented somewhere -- if you do end up using PrintJSStack here, could you file a bug on documenting it, so that it's clearer what its correct usage is?)


> There are several places in codebase use profiler_tracing with
> START/END directly. And some of them live in different scope, for example:
> [1] only start the profiler_tracing, and end in [2] which can not be
> replaced with the RAII.

Gotcha, so maybe we can't drop *all* the profiler_tracing calls. It looks like we can simplify a lot of them, though. (& yeah, separate bug makes sense for that).
Two more notes on PrintJSStack:
 - According to hg blame, PrintJSStack() was added in bug 600304, just as a debugging aid on Android, where we can't call the debugging function "DumpJSStack()". This reduces my confidence that this is something we should actually be calling as part of normal (non-debugging) execution.  (Note that we have 0 calls to DumpJSStack() in our codebase -- it's intended to just be called from GDB, and I think the same is true of PrintJSStack.)

 - Looks like PrintJSStack's helper-method "xpc_PrintJSStack" does have a *little* bit of documentation[1] which confirms that JS_smprintf_free is the correct way to free its returned buffer, if we do end up keeping the call.

[1] http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/xpcprivate.h#2974
(In reply to Chiajung Hung [:chiajung] from comment #23)
> Created attachment 8571251 [details] [diff] [review]
> part2(v1)
> 
> Part 2. In this part I store JS cause of restyle in profiler label and split
> the "Style Bar" on the graph into smaller ones with PseudoStack.
> 
> I tried the method in bug 1123237, but all stack labels of JS are
> js::RunScript, which seems less useful.

That's what's in the PseudoStack, but during stack postprocessing we merge JS function names in between the js::RunScript pseudostack entries, at least when you're profiling with the "js" feature selected. How are you starting the profiler?

In other words, you shouldn't need PrintJSStack at all, just enable js profiling in the profiler.
(Assignee)

Comment 32

3 years ago
(In reply to Markus Stange [:mstange] from comment #31)
> (In reply to Chiajung Hung [:chiajung] from comment #23)
> > Created attachment 8571251 [details] [diff] [review]
> > part2(v1)
> > 
> > Part 2. In this part I store JS cause of restyle in profiler label and split
> > the "Style Bar" on the graph into smaller ones with PseudoStack.
> > 
> > I tried the method in bug 1123237, but all stack labels of JS are
> > js::RunScript, which seems less useful.
> 
> That's what's in the PseudoStack, but during stack postprocessing we merge
> JS function names in between the js::RunScript pseudostack entries, at least
> when you're profiling with the "js" feature selected. How are you starting
> the profiler?
> 
> In other words, you shouldn't need PrintJSStack at all, just enable js
> profiling in the profiler.
Thanks for the information.

I enable the profiler by following command before
./profiler.sh start -p b2g -f restyle

I will try to get the stack from PseudoStack with this new command and see the different.
./profiler.sh start -p b2g -f restyle,js
(Assignee)

Updated

3 years ago
Depends on: 1123237
(Assignee)

Comment 33

3 years ago
I tried the patch in bug 1123237, and enable profiler with js feature.
It works as expected, so I block this by bug 1123237, and will upload a new version based on it later.
(Assignee)

Comment 34

3 years ago
Created attachment 8575799 [details] [diff] [review]
part 2(v3)

After some more test, I think the js stack string is redundant, since profile like:
http://people.mozilla.org/~bgirard/cleopatra/#report=e5a0a45abdfa05c4f85dc54e57589956c11a30cd&filter=[{%22type%22%3A%22RangeSampleFilter%22,%22start%22%3A7010,%22end%22%3A7584},{%22type%22%3A%22RangeSampleFilter%22,%22start%22%3A7280,%22end%22%3A7424}]

contains all I need in the graph now.

Style#5 and 6 shows the stack of self expanding restyle now, which make the stack in the detailed profile useless now, so I decided to drop it now.
Attachment #8573741 - Attachment is obsolete: true
Attachment #8573741 - Flags: review?(mstange)
Attachment #8573741 - Flags: review?(dholbert)
(Assignee)

Comment 35

3 years ago
Created attachment 8575801 [details] [diff] [review]
part 2(v3-1)
Attachment #8575799 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8575801 - Flags: review?(mstange)
Attachment #8575801 - Flags: review?(dholbert)
Comment on attachment 8575801 [details] [diff] [review]
part 2(v3-1)

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

Looks good to me.
Attachment #8575801 - Flags: review?(mstange) → review+
Comment on attachment 8575801 [details] [diff] [review]
part 2(v3-1)

>+++ b/tools/profiler/TableTicker.h
>@@ -77,6 +77,7 @@ class TableTicker: public Sampler {
>     mTaskTracer = hasFeature(aFeatures, aFeatureCount, "tasktracer");
>     mLayersDump = hasFeature(aFeatures, aFeatureCount, "layersdump");
>     mDisplayListDump = hasFeature(aFeatures, aFeatureCount, "displaylistdump");
>+    mProfileRestyle = hasFeature(aFeatures, aFeatureCount, "restyle");
[...]
>+  bool ProfileRestyle() const { return mProfileRestyle; }
[...]
>   bool mDisplayListDump;
>+  bool mProfileRestyle;

Trivial nit: these TableTicker changes don't belong in this patch -- they've already been made in mozilla-central, as part of your patch 1, here:
  http://hg.mozilla.org/mozilla-central/diff/e3eb286a2f3f/tools/profiler/TableTicker.h

(So, this part of the patch doesn't apply to current trunk. The rest seems to apply, though.)

More review comments coming shortly.
With this patch applied, I get this build error locally:
{
In file included from $OBJDIR/layout/generic/Unified_cpp_layout_generic1.cpp:2:
In file included from $SRCDIR/layout/generic/nsFirstLetterFrame.cpp:17:
In file included from $SRCDIR/layout/generic/../base/RestyleManager.h:17:
In file included from $SRCDIR/layout/generic/../base/RestyleTracker.h:19:
../../dist/include/ProfilerBacktrace.h:18:23: error: unknown type name 'JSStreamWriter'
  void StreamJSObject(JSStreamWriter& b);
                      ^
}

Looks like ProfilerBacktrace.h doesn't #include or forward-declare all its dependencies, and this is just the first spot to uncover that problem.

Please add "class JSStreamWriter;" to ProfilerBacktrace.h (above its other forward-decl) -- this fixes this build error for me.
(I filed bug 1142181 on reordering ProfilerBacktrace.cpp's #includes so that we can catch issues like comment 38 more directly. If you'd like to take that & add the JSStreamWriter forward-decl as part of that patch [instead of adding the forward-decl here], that works for me.)
Comment on attachment 8575801 [details] [diff] [review]
part 2(v3-1)

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

::: layout/base/RestyleManager.cpp
@@ +3534,5 @@
>      }
> +
> +    if (profiler_feature_active("restyle")) {
> +      aRestyleTracker.mRestyleSources.RemoveAndForget(content->AsElement(), rs);
> +      if (rs.get()) {

Just use:
  if (rs) {

@@ +3538,5 @@
> +      if (rs.get()) {
> +        profilerRAII.emplace("Paint", "Styles", rs->mBacktrace);
> +        rs->mBacktrace = nullptr; // profiler_tracing calls delete this object
> +      } else {
> +        profilerRAII.emplace("Paint", "Styles", nullptr);

If we get rid of "RestyleSource" and just replace it with a ProfilerBacktrace as suggested below, then we don't actually need this "if/else" anymore. (since RemoveAndForget's failure "nullptr" value will be the right thing to pass into profilerRAII.emplace for the "else" clause).

::: layout/base/RestyleTracker.h
@@ +373,5 @@
> +
> +  struct RestyleSource {
> +    ProfilerBacktrace* mBacktrace;
> +    uint32_t mCount;
> +  };

Two distinct things here:
 (1) mCount:
What's the point of "mCount" here? We set it and increment it, but never actually read it. So maybe it should just go away? (and in that case, maybe the RestyleSource class is an unnecessary layer of encapsulation, since then it'd just contain one thing -- a ProfilerBacktrace pointer)

...and this second thing is only relevant if we're keeping this class (so ignore, if you act on (1) & remove mCount & this class):
 (2) mBacktrace is a heap-allocated thing, and it'd be nice to be sure we're not leaking it, when RestyleSource goes away. To be sure about that, I think this struct (RestyleSource) needs a destructor that: (a) Asserts that mBacktrace is null (can we assert that? Are we guaranteed to hit the code in RestyleManager that nulls it out?) (b) Calls profiler_free_backtrace(mBacktrace), in case it's not null.

For (2) to be safe, we *also* need a constructor, so that a default-constructed "RestyleSource foo;" won't just inadvertantly free random memory when it's destructed. Maybe just add a constructor that takes a ProfilerBacktrace*, and initialize mBacktrace to that & mCount to 1? (in the init list)

(Side-note: I also filed bug 1142197 on putting ProfilerBacktrace instances in UniquePtrs instead of raw pointers. Once we've fixed that bug, that'll give us (2) automatically; but we may still want (1) as a sanity-check.)

@@ +402,5 @@
> +
> +    if (profiler_feature_active("restyle")) {
> +      RestyleSource* rs = new RestyleSource;
> +      rs->mBacktrace = profiler_get_backtrace();
> +      rs->mCount = 1;

Per my above suggestion, this would then become:
  RestyleSource* rs = new RestyleSource(profiler_get_backtrace());

@@ +403,5 @@
> +    if (profiler_feature_active("restyle")) {
> +      RestyleSource* rs = new RestyleSource;
> +      rs->mBacktrace = profiler_get_backtrace();
> +      rs->mCount = 1;
> +      mRestyleSources.Put(aElement, rs);

We should assert that mRestyleSources doesn't already have a value for aElement before we do this.

@@ +409,5 @@
>      return false;
> +  } else if (profiler_feature_active("restyle")) {
> +    RestyleSource* rs = nullptr;
> +    mRestyleSources.Get(aElement, &rs);
> +    if (rs) {

(0) This whole "else if..." clause may be unnecessary, if we do away with mCount (since this clause is just about updating mCount)

But if we happen to keep this code:
(1) Don't bother initializing "rs" to nullptr.
(2) The correct way to check for "nsClassHashtable::Get" success is to check its return-value -- not its outparam.

::: tools/profiler/GeckoProfiler.h
@@ +231,5 @@
>  };
>  
> +class ProfilerBacktrace;
> +
> +class MOZ_STACK_CLASS GeckoProfilerTrackingRAII {

This class-name probably wants s/Tracking/Tracing/ (since this is about the "profiler_tracing" function call, not "tracking")
Comment on attachment 8575801 [details] [diff] [review]
part 2(v3-1)

Marking r- (instead of "r+ with comments addressed), because if we do remove "mCount" (as I think we maybe should, since it's unused), and then remove RestyleSource as a result of that, then this patch will look different-enough that it's worth one more sanity-check review before landing.
Attachment #8575801 - Flags: review?(dholbert) → review-
Also: I'm not 100% sure that ComputeStyleChangeFor() is the right place to be doing all of this.

If I break there in GDB (after a ":hover"-triggered style change) and go up a few stack-levels, I get to RestyleTracker::DoProcessRestyles, which has this code, which seems like it may be a better place to be doing this:
>         nsAutoPtr<RestyleData> data;
>         if (!GetRestyleData(element, data)) {
>           LOG_RESTYLE("skipping, already restyled");
>           continue;
>         }
> 
>         ProcessOneRestyle(element, data->mRestyleHint, data->mChangeHint);
>         AddRestyleRootsIfAwaitingRestyle(data->mDescendants);
http://mxr.mozilla.org/mozilla-central/source/layout/base/RestyleTracker.cpp?rev=c6c310429562&mark=302-309#302

If we could hook in here instead of ComputeStyleChangeFor, then we could hang our backtrace off of the existing "RestyleData" struct, instead of needing an all-new "RestyleSource" struct & an all-new "mRestyleSources" map.

Also, we'd then be keeping all of this profiling code in one set of files -- RestyleTracker.* -- instead of having it spread between RestyleTracker.h and RestyleManager.cpp.
Flags: needinfo?(chung)
That sounds similar to what I played around with a few weeks ago: https://pastebin.mozilla.org/8825369
I wasn't going to land that patch, but it seems like something like that is what we need here.
Hmm, though I guess ProcessOneRestyle() *can* include stuff like RecreateFramesForContent() (via RestyleElement), and we may (?) not want to consider that as "restyle" work. (It can be expensive, and is a different sort of work from recomputing style data.)

(Though -- if (a) we allow profiler_tracing() calls to be nested, and (b) we have appropriate "profiler_tracing()" calls around the expensive parts of RecreateFramesForContent to classify them as whatever profiler-category is appropriate, then this would be a non-issue. Not sure if either of those are true.)

(Hypothetically if we were to move this profiling code up to the ProcessOneRestyle neighborhood, we'd need to catch *both* ProcessOneRestyle callers, as Markus's pastebin does -- and we'd also need to be able to transfer the ProfilerBacktrace* from RestyleData to RestyleEnumerateData (a temporary object), as his patch does.)
(I'm not too worried about my concern from the beginning of comment 44, FWIW -- the possibility of counting RecreateFramesForContent() in the "restyle" category. While RecreateFramesForContent is technically more of a frame-tree-surgery sort of thing, it does happen *during* restyle code, so it's reasonable in that sense to count it.  And if we end up wanting to separate it out, I'll bet we can extend the profiler to support that -- to let us 'push' a new profiler_tracing context when we enter RecreateFramesForContent, e.g. -- if that doesn't already work.)
(In reply to Daniel Holbert [:dholbert] from comment #40)
> Two distinct things here:
>  (1) mCount:
> What's the point of "mCount" here? 
[...]
>  (2) mBacktrace is a heap-allocated thing, and it'd be nice to be sure we're
> not leaking it, when RestyleSource goes away. To be sure about that, I think
> this struct (RestyleSource) needs a destructor that: (a) Asserts that
> mBacktrace is null (can we assert that? Are we guaranteed to hit the code in
> RestyleManager that nulls it out?) (b) Calls
> profiler_free_backtrace(mBacktrace), in case it's not null.
[...]
> (Side-note: I also filed bug 1142197 on putting ProfilerBacktrace instances
> in UniquePtrs instead of raw pointers. Once we've fixed that bug, that'll
> give us (2) automatically; but we may still want (1) as a sanity-check.)


Sorry, I got my indexing into an inconsistent state when editing my writing here. At the end, I meant to say that UniquePtr will give us "(b) automatically", and we may still want "(a)". (not (2)/(1))
(Assignee)

Comment 47

3 years ago
Created attachment 8576459 [details] [diff] [review]
part 2(v4)

Address most comment above and move the implementation to RestyleTracker. This make it clearer and compact.
Flags: needinfo?(chung)
(Assignee)

Comment 48

3 years ago
Comment on attachment 8576459 [details] [diff] [review]
part 2(v4)

The count is originally for a case like
function changeStyle() {
    elt.style.visibility = "visible";
    elt.style.width = "100%";
    elt.style.height = "100%";
}

each line in this function trigger a restyle and all such manipulations in JS code before restyle should contribute a stack. So we want to count how many spot to show something like "(X more...)" on the stack. 

BTW, if we want record all these stacks somehow, someday we may need keep an array of ProfileBacktrace later.
Attachment #8576459 - Flags: review?(dholbert)
(Assignee)

Updated

3 years ago
Attachment #8576459 - Flags: review?(dholbert)
(In reply to Chiajung Hung [:chiajung] from comment #48)
> The count is originally for a case like
[...]
> So we want to count how
> many spot to show something like "(X more...)" on the stack. 

Makes sense -- I figured it was something like that.

> BTW, if we want record all these stacks somehow, someday we may need keep an
> array of ProfileBacktrace later.

Yup, true.

I looked at the new patch -- thanks for addressing those comments! I did notice you've still got the mRestyleSources map -- per second half of comment 42, I still think it'd be nice to hang the ProfileBacktrace off of RestyleData & RestyleEnumerateData, so we can co-opt the existing map instead of needing to maintain a separate one.

(This new member-var on RestlyeData/RestyleEnumerateData should probably have type "mozilla::UniquePtr<ProfilerBacktrace>", which will give us cleanup & ownership transferring mostly-for-free -- so that we don't need a whole other map for it -- and will get us partway to bug 1142197.)

(In case you're not familiar with UniquePtr: it's basically the same as nsAutoPtr, but it's got stronger guarantees & it's much more recent. There's a lot of old code that still uses nsAutoPtr, and we just stick with nsAutoPtr in some cases for consistency, but I'd suggest UniquePtr for these ProfilerTracing member-variables, in preparation for bug 1142197.)
(So we'd need to pass the ProfilerBacktrace into ProcessOneRestyle, via a new function-parameter e.g. "mozilla::UniquePtr<ProfilerTracing> aRestyleSource")
(Assignee)

Comment 51

3 years ago
Created attachment 8576542 [details] [diff] [review]
part 2(v4-1)

Make ProfilerBacktrace always goes to profiler_free_backtrace even if it is not used.
Attachment #8575801 - Attachment is obsolete: true
Attachment #8576459 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8576542 - Flags: review?(dholbert)
Comment on attachment 8576542 [details] [diff] [review]
part 2(v4-1)

Looks good -- just needs to handle the one other call to ProcessOneRestyle, a bit further down.

(which is why we need to add this to RestyleEnumerateData as well -- the code further down converts all of the RestyleData instances to RestyleEnumerateData before it calls ProcessOneRestyle.)
Attachment #8576542 - Flags: review?(dholbert) → feedback+
(Assignee)

Comment 53

3 years ago
Created attachment 8577107 [details] [diff] [review]
part 2(v5)
Attachment #8576542 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8577107 - Flags: review?(dholbert)
(Assignee)

Updated

3 years ago
Depends on: 1142181
No longer depends on: 1123237
Comment on attachment 8577107 [details] [diff] [review]
part 2(v5)

Almost there! This is just missing one thing -- as noted in my parenthetical in comment 49, please keep track of ProfilerBacktraces in these structs using a UniquePtr. (Most of my review notes in this comment are going to be related to that, FWIW) That way, ownership-transfering is automatic, and in the unlikely event that we create one of these structs and then bail out before clearing its mBacktrace, we can be sure it'll be cleaned up.

Note that this means we'll implicitly be cleaning up mBacktrace using "delete" (in the UniquePtr destructor) instead of "profiler_free_backtrace", which is slightly inconsistent.  But I checked with aklotz in IRC, and he says he's fine with this, since (a) profiler_free_backtrace is really just "delete" after all, and (b) bug 1142197 will get rid of profiler_free_backtrace, and (c) UniquePtr lets us make this code so much cleaner.

>+++ b/layout/base/RestyleTracker.cpp
> struct RestyleEnumerateData : RestyleTracker::Hints {
>   nsRefPtr<dom::Element> mElement;
>+  ProfilerBacktrace* mBacktrace;
> };

So this should be UniquePtr.

> #ifdef RESTYLE_LOGGING
>   collector->count++;
>@@ -303,6 +306,11 @@ RestyleTracker::DoProcessRestyles()
>           continue;
>         }
> 
>+        Maybe<GeckoProfilerTracingRAII> profilerRAII;
>+        if (profiler_feature_active("restyle")) {
>+          profilerRAII.emplace("Paint", "Styles", data->mBacktrace);
>+          data->mBacktrace = nullptr; // profiler_tracing call free the backtrace

This last line (nulling out) should become unnecessary, once you've tweaked GeckoProfilerTracingRAII as I describe below.

>+          Maybe<GeckoProfilerTracingRAII> profilerRAII;
>+          if (profiler_feature_active("restyle")) {
>+            profilerRAII.emplace("Paint", "Styles", currentRestyle->mBacktrace);
>+            currentRestyle->mBacktrace = nullptr; // profiler_tracing call free the backtrace

Same here.

>diff --git a/layout/base/RestyleTracker.h b/layout/base/RestyleTracker.h
>index a58ee1d..b295156 100644
>--- a/layout/base/RestyleTracker.h
>+++ b/layout/base/RestyleTracker.h
>@@ -16,6 +16,8 @@
> #include "nsContainerFrame.h"
> #include "mozilla/SplayTree.h"
> #include "mozilla/RestyleLogging.h"
>+#include "ProfilerBacktrace.h"
>+#include "GeckoProfiler.h"
> 
> namespace mozilla {
> 
>@@ -279,11 +281,19 @@ public:
>     RestyleData() {
>       mRestyleHint = nsRestyleHint(0);
>       mChangeHint = NS_STYLE_HINT_NONE;
>+      mBacktrace = nullptr;

No need for this 3rd line, after you make this a UniquePtr; UniquePtr is null by default.

>     RestyleData(nsRestyleHint aRestyleHint, nsChangeHint aChangeHint) {
>       mRestyleHint = aRestyleHint;
>       mChangeHint = aChangeHint;
>+      mBacktrace = nullptr;
>+    }

Same here.

>+    ~RestyleData() {
>+      if (mBacktrace) {
>+        profiler_free_backtrace(mBacktrace);
>+      }
>     }

You don't need this destructor; just make mBacktrace a UniquePtr.

>     // that we called AddPendingRestyle for and found the element this is
>     // the RestyleData for as its nearest restyle root.
>     nsTArray<nsRefPtr<Element>> mDescendants;
>+    ProfilerBacktrace* mBacktrace;

Per above notes, this should be UniquePtr<ProfilerBacktrace>.

>+class MOZ_STACK_CLASS GeckoProfilerTracingRAII {
>+public:
>+  GeckoProfilerTracingRAII(const char* aCategory, const char* aInfo, ProfilerBacktrace* aBacktrace = nullptr
>+                            MOZ_GUARD_OBJECT_NOTIFIER_PARAM)
                             ^
Indentation is off here -----^

The 3rd arg should probably be UniquePtr<ProfilerBacktrace>, to explicitly take ownership of the passed-in backtrace.

(I think you can still provide a default-argument, e.g. something like:
  UniquePtr<ProfilerBacktrace> aBacktrace = UniquePtr<ProfilerBacktrace>(nullptr)
but I'm not sure)

>+    : mCategory(aCategory)
>+    , mInfo(aInfo)
>+  {
>+    MOZ_GUARD_OBJECT_NOTIFIER_INIT;
>+    profiler_tracing(mCategory, mInfo, aBacktrace, TRACING_INTERVAL_START);

This should now be "aBacktrace.release()" (which gives up ownership).

(Once we've made the profiler APIs more UniquePtr<ProfilerBacktrace>-aware, in bug 1142197, we'll be able to drop the "release" here.)
Attachment #8577107 - Flags: review?(dholbert) → feedback+
(Assignee)

Comment 55

3 years ago
Created attachment 8577854 [details] [diff] [review]
part 2(v5-1)

Since UniquePtr has private operator=(), and the RAII need mBacktrace.release(), I can not pass const UniquePtr<ProfilerBacktrace>& nor UniquePtr<ProfilerBacktrace> into ctor. As a result, I removed the default parameter for it.

Therefore, while clean up profiler_tracing calls in codebase, we should insert a ctor ProfilerRAII(const char*, const char*) for it, later.
Attachment #8577107 - Attachment is obsolete: true
Attachment #8577854 - Flags: review?(dholbert)
Comment on attachment 8577854 [details] [diff] [review]
part 2(v5-1)

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

Looks great -- thanks! Just a few final notes.

r=me with the following:

::: layout/base/RestyleTracker.cpp
@@ +306,5 @@
>          }
>  
> +        Maybe<GeckoProfilerTracingRAII> profilerRAII;
> +        if (profiler_feature_active("restyle")) {
> +          profilerRAII.emplace("Paint", "Styles", data->mBacktrace);

You should be passing "Move(data->mBacktrace)" here. (See notes below on GeckoProfilerTracingRAII constructor).

@@ +346,5 @@
>            LOG_RESTYLE_INDENT();
> +
> +          Maybe<GeckoProfilerTracingRAII> profilerRAII;
> +          if (profiler_feature_active("restyle")) {
> +            profilerRAII.emplace("Paint", "Styles", currentRestyle->mBacktrace);

Same here -- you should be passing "Move(currentRestyle->mBacktrace)" here. Also, you'll need to wrap that to a new line to avoid going over 80 characters.

(See notes below on GeckoProfilerTracingRAII constructor)

::: layout/base/RestyleTracker.h
@@ +392,5 @@
>  
>    if (!existingData) {
> +    RestyleData* rd = new RestyleData(aRestyleHint, aMinChangeHint);
> +    if (profiler_feature_active("restyle")) {
> +      rd->mBacktrace.reset(profiler_get_backtrace());

(Side note: right now, profiler_get_backtrace() is a wrapper for mozilla_sampler_get_backtrace(), which ultimately returns a "new ProfilerBacktrace()" expression. In bug 1142197, that call-stack should probably do allocation using "MakeUnique" instead of "new", which returns a smart-typed pointer instead of a raw pointer -- and then this line you're adding here will probably change to "=" instead of ".reset()".)

::: tools/profiler/GeckoProfiler.h
@@ +234,5 @@
> +class ProfilerBacktrace;
> +
> +class MOZ_STACK_CLASS GeckoProfilerTracingRAII {
> +public:
> +  GeckoProfilerTracingRAII(const char* aCategory, const char* aInfo, mozilla::UniquePtr<ProfilerBacktrace>& aBacktrace

This line is too long (looks like ~118 characters). Wrap it before "mozilla::UniquePtr".

Also: this shouldn't be a reference -- it should just be a UniquePtr. At least, the UniquePtr documentation says:
 * [...] To unconditionally transfer ownership of a UniquePtr
 * into a method, use a |UniquePtr| argument.
http://mxr.mozilla.org/mozilla-central/source/mfbt/UniquePtr.h#148

So, please drop the "&". For this to work, you have to use "Move()" at the callsite -- seem my notes above for that. (As a bonus, this makes it more explicit that the pointer is being given up, at the callsite.)
Attachment #8577854 - Flags: review?(dholbert) → review+
(Assignee)

Comment 57

3 years ago
Created attachment 8579904 [details] [diff] [review]
part 2(final)

Fix commit comment, and various UniquePtr usage in last review, carry r+.

Try ticket:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d59c6b9f4828
Attachment #8577854 - Attachment is obsolete: true
Attachment #8579904 - Flags: review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/703caa5d845f
Keywords: checkin-needed

Comment 59

3 years ago
This seems to have broken non-sps builds.

Updated

3 years ago
Depends on: 1145988
https://hg.mozilla.org/mozilla-central/rev/703caa5d845f
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
Depends on: 1149065
You need to log in before you can comment on or make changes to this bug.