1335461 - Pull out hang and stack classes into a separate source files

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P3)

Description

[Georg Fritzsche [:gfritzsche]]

Before we add content process support for stack capturing (bug 1299770), we need to factor the capture code out into a separate module.
Without this it will become hard to read and add even more noise to Telemetry.cpp.

This might require pulling out other classes or functions used by hangstats etc. into a shared module(s).
A good pattern to follow for the new module (for interface and internal style) is TelemetryEvent.cpp.

Comment 2

[Georg Fritzsche [:gfritzsche]]

See also bug 1272671, comment 0.

Comment 3

[Iaroslav Sheptykin]

Attachment: Bug 1335461: Pulling out stack capturing into standalone files.

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

Comment 4

[Iaroslav Sheptykin]

Comment on attachment 8843681 [details]
Bug 1335461: Pulling out stack capturing into standalone files.

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

Comment 5

[Iaroslav Sheptykin]

Hi Georg!

As promised, I submitted my first draft on refactoring `KeyedStackCapture` out of `Telemetry.cpp`. Unlike promised, publishing it with a huge delay, sorry about it.

The patch compiles locally but I didn't give it a proper manual test yet. Also, I am not happy with the way I extracted `CreateJSStackObject`. Maybe you can give me some input on that?

Comment 6

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8843681 [details]
Bug 1335461: Pulling out stack capturing into standalone files.

https://reviewboard.mozilla.org/r/117282/#review124800

::: toolkit/components/telemetry/CombinedStacks.h:9
(Diff revision 2)
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef CombinedStacks_h__
> +#define CombinedStacks_h__
> +
> +#include <algorithm>

You only need to include `<vector>`.

::: toolkit/components/telemetry/CombinedStacks.h:17
(Diff revision 2)
> +const size_t kMaxChromeStacksKept = 50;
> +// The maximum depth of a single chrome hang stack.
> +const size_t kMaxChromeStackDepth = 50;

I don't think we need to expose this in the header.

::: toolkit/components/telemetry/CombinedStacks.cpp:14
(Diff revision 2)
> +  size_t
> +  CombinedStacks::GetModuleCount() const {

We don't indent inside the namespaces, the methods definitions should just be without any leading spaces.

::: toolkit/components/telemetry/KeyedStackCapturer.h:25
(Diff revision 2)
> +#include "jsapi.h"
> +
> +namespace mozilla {
> +namespace Telemetry {
> +
> +  /**

No indentation here.

::: toolkit/components/telemetry/KeyedStackCapturer.cpp:19
(Diff revision 2)
> + *
> + * @param aChar a character to validate.
> + * @return True, if the char is valid, False - otherwise.
> + */
> +bool
> +IsKeyCharValid(const char aChar)

The utility functions should be in an unnamed namespace (or static), so they are not visible/linkable from outside this file.

::: toolkit/components/telemetry/Telemetry.h:433
(Diff revision 2)
> +// WIP: This is required by KeyedStackCapturer, but also by some parts of
> +// Telemetry. I could not find a better way of sharing the implementation yet.
> +JSObject *
> +CreateJSStackObject(JSContext *cx, const CombinedStacks &stacks);

Adding this to Telemetry.h makes it public to other modules.
Lets figure out a different way to share this.

This function seems to be specific to `CombinedStacks`, can we share it from CombinedStacks.h and implement it in CombinedStacks.cpp?

::: toolkit/components/telemetry/Telemetry.cpp:86
(Diff revision 2)
>  #include "nsNativeCharsetUtils.h"
>  #include "nsProxyRelease.h"
>  
>  #if defined(MOZ_GECKO_PROFILER)
>  #include "shared-libraries.h"
> -#define ENABLE_STACK_CAPTURE
> +#include "KeyedStackCapturer.h"

Lets only include this when `ENABLE_STACK_CAPTURE` is set.

::: toolkit/components/telemetry/moz.build:51
(Diff revision 2)
>      '!TelemetryHistogramEnums.h',
>      '!TelemetryScalarEnums.h',
> +    'CombinedStacks.h',
>      'ipc/TelemetryComms.h',
>      'ipc/TelemetryIPC.h',
> +    'KeyedStackCapturer.cpp',

This should not go into the exports - only headers go here.
Additionally, its enough to call this through the Telemetry.h interface, so no need to add this to exports.

::: toolkit/components/telemetry/moz.build:58
(Diff revision 2)
> +    'CombinedStacks.cpp',
>      'ipc/TelemetryIPC.cpp',
>      'ipc/TelemetryIPCAccumulator.cpp',
> +    'KeyedStackCapturer.cpp',

Will they both build without profiling and stack capturing enabled?

Comment 7

[Iaroslav Sheptykin]

Comment on attachment 8843681 [details]
Bug 1335461: Pulling out stack capturing into standalone files.

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

Comment 8

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8843681 [details]
Bug 1335461: Pulling out stack capturing into standalone files.

https://reviewboard.mozilla.org/r/117282/#review138522

This looks close for the re-organization!
I concentrated on the headers this time.
After rebasing this to the current Firefox code i will look more closely at the C++ changes.

::: toolkit/components/telemetry/CombinedStacks.h:12
(Diff revision 3)
> +#define CombinedStacks_h__
> +
> +#include <vector>
> +
> +#include "ProcessedStack.h"
> +#include "jsapi.h"

You don't need to include this in the header, you can forward-declare `JSObject` here.

::: toolkit/components/telemetry/CombinedStacks.h:17
(Diff revision 3)
> +// The maximum number of chrome hangs stacks that we're keeping.
> +const size_t kMaxChromeStacksKept = 50;
> +// The maximum depth of a single chrome hang stack.
> +const size_t kMaxChromeStackDepth = 50;

Lets move these into the C++ files.
We can use separate constants in CombinedStacks.cpp, HangReports.cpp, ...

::: toolkit/components/telemetry/HangReports.h:9
(Diff revision 3)
> +#include <algorithm>
> +#include "mozilla/HangAnnotations.h"
> +#include "mozilla/Move.h"
> +#include "ProcessedStack.h"
> +#include "nsTArray.h"
> +#include "nsString.h"
> +#include "nsClassHashtable.h"
> +#include "CombinedStacks.h"
> +#include "mozilla/MemoryReporting.h"

I don't think all of these are needed, e.g.:
- `<algorithm>` - did you mean `<vector>`?
- `"mozilla/Move.h"`
- `"mozilla/MemoryReporting.h"`

::: toolkit/components/telemetry/HangReports.h:22
(Diff revision 3)
> +#include "mozilla/MemoryReporting.h"
> +
> +namespace mozilla {
> +namespace Telemetry {
> +
> +using namespace HangMonitor;

`using namespace` in headers is generally not a good idea - types and naming become confusing for anyone importing this.
Let's not do this.

::: toolkit/components/telemetry/KeyedStackCapturer.h:20
(Diff revision 3)
> +#include "mozilla/StackWalk.h"
> +#include "nsClassHashtable.h"
> +#include "mozilla/Mutex.h"
> +#include "CombinedStacks.h"
> +#include "ProcessedStack.h"
> +#include "jsapi.h"

I think you don't need to include ProcessedStack.h, jsapi.h, StackWalk.h, nsPrintfCString.h.

::: toolkit/components/telemetry/KeyedStackCapturer.cpp:19
(Diff revision 3)
> + *
> + * @param aChar a character to validate.
> + * @return True, if the char is valid, False - otherwise.
> + */
> +bool
> +IsKeyCharValid(const char aChar)

These functions have external linkage the way they are defined here.
Please make them either `static` or put them in an unnamed namespace.

::: toolkit/components/telemetry/Telemetry.h:18
(Diff revision 3)
>  #include "nsStringGlue.h"
>  #include "nsXULAppAPI.h"
>  
>  #include "mozilla/TelemetryHistogramEnums.h"
>  #include "mozilla/TelemetryScalarEnums.h"
> +#include "mozilla/CombinedStacks.h"

Why does that need to be included?
Is it enough to include it into the .cpp?

::: toolkit/components/telemetry/Telemetry.cpp:104
(Diff revision 3)
>  using namespace mozilla::HangMonitor;
>  using Telemetry::Common::AutoHashtable;
> +
>  using mozilla::dom::Promise;
>  using mozilla::dom::AutoJSAPI;
> +using namespace mozilla::Telemetry;

Apparently you added this, is it needed here?

::: toolkit/components/telemetry/moz.build:48
(Diff revision 3)
>  
>  EXPORTS.mozilla += [
>      '!TelemetryEventEnums.h',
>      '!TelemetryHistogramEnums.h',
>      '!TelemetryScalarEnums.h',
> +    'CombinedStacks.h',

I don't think you need to add this header here.

`CombinedStacks` isn't used outside Telemetry:
https://dxr.mozilla.org/mozilla-central/search?q=CombinedStacks&redirect=false

Which class exactly do you need an export for?

Comment 9

[Iaroslav Sheptykin]

Comment on attachment 8843681 [details]
Bug 1335461: Pulling out stack capturing into standalone files.

https://reviewboard.mozilla.org/r/117282/#review138522

> Apparently you added this, is it needed here?

The build breaks at Telemetry.cpp in multiple places without this. For example `TelemetryImp` [refers](https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/Telemetry.cpp#959) to HangReports, which used to be in the same namespace but now is moved into `mozilla::Telemetry`.

Comment 10

[Iaroslav Sheptykin]

Comment on attachment 8843681 [details]
Bug 1335461: Pulling out stack capturing into standalone files.

https://reviewboard.mozilla.org/r/117282/#review124800

> Will they both build without profiling and stack capturing enabled?

I built FF twice with the following configurations in .mozconfig:

1. with profiling:
```
export MOZILLA_OFFICIAL=1
export MOZ_TELEMETRY_REPORTING=1
export MOZ_ENABLE_PROFILER_SPS=1
export MOZ_DEBUG_SYMBOLS=1
ac_add_options --enable-debug-symbols="-gdwarf-2"
ac_add_options --enable-profiling

```
2. without profiling:
```
export MOZILLA_OFFICIAL=1
export MOZ_TELEMETRY_REPORTING=1
export MOZ_DEBUG_SYMBOLS=1

```

Both builds succeeded. When I tested stack capturing with 2 it surprizingly succeeded. The stack was captured and it was visible on `about:telemetry` dashboard. It looks like profiler and stack capture are still built in. What am I doing wrong?

Comment 11

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8843681 [details]
Bug 1335461: Pulling out stack capturing into standalone files.

https://reviewboard.mozilla.org/r/117282/#review124800

> Lets only include this when `ENABLE_STACK_CAPTURE` is set.

Actually, ENABLE_STACK_CAPTURE is currently effectively just an alias for MOZ_GECKO_PROFILER.
So, we can also just drop this define.

> I built FF twice with the following configurations in .mozconfig:
> 
> 1. with profiling:
> ```
> export MOZILLA_OFFICIAL=1
> export MOZ_TELEMETRY_REPORTING=1
> export MOZ_ENABLE_PROFILER_SPS=1
> export MOZ_DEBUG_SYMBOLS=1
> ac_add_options --enable-debug-symbols="-gdwarf-2"
> ac_add_options --enable-profiling
> 
> ```
> 2. without profiling:
> ```
> export MOZILLA_OFFICIAL=1
> export MOZ_TELEMETRY_REPORTING=1
> export MOZ_DEBUG_SYMBOLS=1
> 
> ```
> 
> Both builds succeeded. When I tested stack capturing with 2 it surprizingly succeeded. The stack was captured and it was visible on `about:telemetry` dashboard. It looks like profiler and stack capture are still built in. What am I doing wrong?

For build variant 2, is profiling actually off?
Have you tried with `ac_add_options --disable-profiling`?

There might also be build side-effects from this and the preceding line:
https://dxr.mozilla.org/mozilla-central/source/toolkit/moz.configure#36

Comment 12

[Iaroslav Sheptykin]

Comment on attachment 8843681 [details]
Bug 1335461: Pulling out stack capturing into standalone files.

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

Comment 13

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8843681 [details]
Bug 1335461: Pulling out stack capturing into standalone files.

https://reviewboard.mozilla.org/r/117282/#review142406

This looks pretty good now, only two nits that i saw.
It's great to see Telemetry.cpp cleaned up that way.

The next step, slightly painful step would be to rebase that to the latest Firefox code.

::: toolkit/components/telemetry/KeyedStackCapturer.cpp:16
(Diff revision 4)
> +using namespace mozilla;
> +using namespace Telemetry;

I don't think you need these.

::: toolkit/components/telemetry/Telemetry.cpp:111
(Diff revision 4)
> -const size_t kMaxChromeStacksKept = 50;
> +using mozilla::Telemetry::KeyedStackCapturer;
> +using mozilla::Telemetry::CombinedStacks;
> +using mozilla::Telemetry::ComputeAnnotationsKey;
> +
>  // The maximum depth of a single chrome hang stack.
>  const size_t kMaxChromeStackDepth = 50;

This seems unused.

Comment 14

[Iaroslav Sheptykin]

Attachment: Bug 1335461: Pulling out stack capturing into standalone files.

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

Comment 15

[Iaroslav Sheptykin]

Georg, I think I messed up our review. There are now two reviews open. One contains 4 versions of the patch and your feedback. The other is newly created and contains the 5th version with the feedback addressed and code re-based on the latest central. Not sure how to merge these guys into one. Do you have an idea?

Comment 16

[Iaroslav Sheptykin]

Comment on attachment 8868143 [details]
Bug 1335461: Pulling out stack capturing into standalone files.

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

Comment 17

[Georg Fritzsche [:gfritzsche]]

If its not easy to fix, i'm fine with just leaving it in the new review request.

Comment 18

[Georg Fritzsche [:gfritzsche]]

Michael & Mike, i think you touched some of the affected code most recently.
This patch pulls hang & stack capture related code out of Telemetry.cpp in separate files.

How can we make sure that the refactor & rebasing didn't miss any details?
Are there any specific tests or manual test scenarios to run?
Would one of you be up to a (casual) review to see if anything stands out?

Comment 19

[Georg Fritzsche [:gfritzsche]]

The latest changes are here:
https://reviewboard.mozilla.org/r/139748/diff/2#index_header

Comment 20

[Nika Layzell [:nika] (ni? for response)]

(In reply to Georg Fritzsche [:gfritzsche] from comment #18)
> Michael & Mike, i think you touched some of the affected code most recently.
> This patch pulls hang & stack capture related code out of Telemetry.cpp in
> separate files.
> 
> How can we make sure that the refactor & rebasing didn't miss any details?
> Are there any specific tests or manual test scenarios to run?
> Would one of you be up to a (casual) review to see if anything stands out?

Immediate thing which I'm noticing is that the code for the Background Hang Reporter is remaining inside of Telemetry.cpp, and it uses CombinedStacks as well. I am OK with it staying in Telemetry.cpp (as it will make my rebase for my local patches which change it much easier) but I was expecting it to move out of tree with the description of this bug.

Other than that it looked mostly fine. I didn't scour over it line by line to make sure that nothing was deleted or changed though.

Comment 21

[Georg Fritzsche [:gfritzsche]]

(In reply to Michael Layzell [:mystor] from comment #20)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #18)
> > Michael & Mike, i think you touched some of the affected code most recently.
> > This patch pulls hang & stack capture related code out of Telemetry.cpp in
> > separate files.
> > 
> > How can we make sure that the refactor & rebasing didn't miss any details?
> > Are there any specific tests or manual test scenarios to run?
> > Would one of you be up to a (casual) review to see if anything stands out?
> 
> Immediate thing which I'm noticing is that the code for the Background Hang
> Reporter is remaining inside of Telemetry.cpp, and it uses CombinedStacks as
> well. I am OK with it staying in Telemetry.cpp (as it will make my rebase
> for my local patches which change it much easier) but I was expecting it to
> move out of tree with the description of this bug.

Good point. If that's easier for you we can just take this to a follow-up.

> Other than that it looked mostly fine. I didn't scour over it line by line
> to make sure that nothing was deleted or changed though.

Thanks for checking! Do we have any good tests to run here?

Comment 22

[Iaroslav Sheptykin]

Comment on attachment 8843681 [details]
Bug 1335461: Pulling out stack capturing into standalone files.

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

Comment 23

[Iaroslav Sheptykin]

Comment on attachment 8843681 [details]
Bug 1335461: Pulling out stack capturing into standalone files.

https://reviewboard.mozilla.org/r/117282/#review142406

> This seems unused.

This is being used in https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/Telemetry.cpp#3080 
I didn't pull out `ProcessedStack` out of `Telementry.cpp`. Should we try to do that?

Comment 24

[Iaroslav Sheptykin]

Georg, I figured a way of updating the original Review. It the end it was just reading the docs. [It](https://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/reviewboard.html) says:

```
review id
    A unique identifier identifying a review request series. This is commonly derived from a bug number and username.
```

Based on this review id reviewbord decides which review to assign a request to. The `review_id` looks something like this: `review id:  bz://448604/MattN`. The review id for the original review is `bz://1335461/yarik` where as the review id of the other (newer) patch was: `bz://1335461/isheptykin`. Because I published the patch from a different environment my username was different. New `review_id` was created and new review was opened. We can close and ignore the newer review now and get back to working on the original review. This will preserve the history of feedback and patch history.

Adding needinfo just to make sure you read this and review the right thing.

Comment 25

[Iaroslav Sheptykin]

Hi Georg! You can review https://reviewboard.mozilla.org/r/117282/. This review contains the latest patch that builds locally.

Comment 26

[Mike Conley (:mconley) (:⚙️)]

I can't think of anything BHR-wise that'll be impacted negatively by this change.

Comment 27

[Georg Fritzsche [:gfritzsche]]

(In reply to Iaroslav Sheptykin from comment #23)
> I didn't pull out `ProcessedStack` out of `Telementry.cpp`. Should we try to
> do that?

Lets get this finished and landed with what you pulling out right now.
Then we can look at the remaining things we break out in a follow-up bug (see also comment 20).
I think this will be easier now.

Comment 28

[Iaroslav Sheptykin]

Comment on attachment 8843681 [details]
Bug 1335461: Pulling out stack capturing into standalone files.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/117282/diff/5-6/

Comment 29

[Iaroslav Sheptykin]

Comment on attachment 8843681 [details]
Bug 1335461: Pulling out stack capturing into standalone files.

https://reviewboard.mozilla.org/r/117282/#review124800

> For build variant 2, is profiling actually off?
> Have you tried with `ac_add_options --disable-profiling`?
> 
> There might also be build side-effects from this and the preceding line:
> https://dxr.mozilla.org/mozilla-central/source/toolkit/moz.configure#36

I tried building with and without profiling but adding / removing `ac_add_options --enable-profiling`. Both variants build and stack capturing behaves as one would expect. With profiling on calls to `Telemetry.captureStack()` capture a stack an it appears on `about:telemetry`. Without profiling `Telemetry.captureStack()` is available but has no effect on `about:telementry` and pings.

Comment 30

[Iaroslav Sheptykin]

Hi Georg!

I added a tar file containing all partial diffs between extracted code and telemetry. The tar is attached to the review. If you have problems using it let me know. If you want to reproduce the diffs or generate new ones please download these two scripts:

https://gist.github.com/yariksheptykin/2b9f4bd34dd12acada98996a1a8e72ea#file-dif-lines-sh
https://gist.github.com/yariksheptykin/645aa43ccb22844131e631c21cb5209b#file-generate-refactoring-diffs-sh

use it like this:
1. apply review patch

2. Run this
```
cd toolkit/components/telemetry
wget https://gist.githubusercontent.com/yariksheptykin/645aa43ccb22844131e631c21cb5209b/raw/0527a5e3c93fc9ce83217c1b89138ad3f9c78f5b/generate-refactoring-diffs.sh
wget https://gist.githubusercontent.com/yariksheptykin/2b9f4bd34dd12acada98996a1a8e72ea/raw/011887ce5c7c280ea27b7f02ddd3eb2ccd8cccec/dif-lines.sh
bash ./generate-refactoring-diffs.sh
```

3. Results will land in /tmp/diffs/

Comment 31

[Georg Fritzsche [:gfritzsche]]

Nice, that makes it easier, thanks!
I'm not seeing an attached tar file here though?

Comment 32

[Iaroslav Sheptykin]

Attachment: Line-wise diffs of refactored code

I attached the file to the review hoping that reviewboard will make it magically available on bugzilla. Attaching the file manually.

Comment 33

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8843681 [details]
Bug 1335461: Pulling out stack capturing into standalone files.

https://reviewboard.mozilla.org/r/117282/#review148438

This looks good to me, thanks!

Pullling this code out of Telemetry.cpp is a big win for us. Lets get this landed and iterate on other improvements in follow-up bugs.

::: toolkit/components/telemetry/KeyedStackCapturer.h:9
(Diff revision 6)
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef KeyedStackCapturer_h__
> +#define KeyedStackCapturer_h__
> +
> +#ifdef MOZ_GECKO_PROFILER

Commenting out whole source file contents is not common. Lets file a follow-up bug to improve this.

I think we can:
- guard the KeyedStackCapturer.h include in Telemetry.cpp with #ifdef
- modify Telemetrys moz.build to only use the KeyedStackCapturer source files when MOZ_GECKO_PROFILER is defined

Comment 34

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8843681 [details]
Bug 1335461: Pulling out stack capturing into standalone files.

https://reviewboard.mozilla.org/r/117282/#review138522

> The build breaks at Telemetry.cpp in multiple places without this. For example `TelemetryImp` [refers](https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/Telemetry.cpp#959) to HangReports, which used to be in the same namespace but now is moved into `mozilla::Telemetry`.

You can solve this by using `using` statements for the specific symbols, e.g. `using mozilla::Telemetry::HangReports`.

Comment 35

[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 cd222fe7eb22 -d d2b02f7d0e73: rebasing 399260:cd222fe7eb22 "Bug 1335461: Pulling out stack capturing into standalone files. r=gfritzsche" (tip)
merging toolkit/components/telemetry/Telemetry.cpp
merging toolkit/components/telemetry/moz.build
warning: conflicts while merging toolkit/components/telemetry/Telemetry.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 36

[Iaroslav Sheptykin]

Comment on attachment 8843681 [details]
Bug 1335461: Pulling out stack capturing into standalone files.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/117282/diff/6-7/

Comment 37

[Iaroslav Sheptykin]

Hi Georg!

The patch could not be landed because of the conflicts. Our work collided with [Bug 1363883](https://bugzilla.mozilla.org/show_bug.cgi?id=1363883). Looking at the [chageset](https://hg.mozilla.org/mozilla-central/rev/875b676f6c83) I noticed that `CombinedStacks` was modified. I transplanted the changes into .h and .cpp files and updated the review. No other patch chunks failed. Should I re-generate the line diffs?

Comment 38

[Iaroslav Sheptykin]

See Comment 37

Comment 39

[Georg Fritzsche [:gfritzsche]]

That is fine, i think you can just rebase & update the patch.
I can trigger landing for you.

Comment 40

[Iaroslav Sheptykin]

I re-based the patch already and updated the review (see diff #7). I should be able to land now.

Comment 41

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8843681 [details]
Bug 1335461: Pulling out stack capturing into standalone files.

https://reviewboard.mozilla.org/r/117282/#review148696

::: toolkit/components/telemetry/CombinedStacks.h:25
(Diff revisions 6 - 7)
>   * This class is conceptually a list of ProcessedStack objects, but it represents them
>   * more efficiently by keeping a single global list of modules.
>   */
>  class CombinedStacks {
>  public:
> -  CombinedStacks() : mNextIndex(0) {}
> +  explicit CombinedStacks(size_t aMaxStacksCount = kMaxChromeStacksKept)

This does not build, `kMaxChromeStacksKept` is undeclared.
As far as i can see, `CombinedStacks` is now being used with two different values:
- `kMaxChromeStacksKept`
- `kMaximumNativeHangStacks`

Comment 42

[Iaroslav Sheptykin]

Comment on attachment 8843681 [details]
Bug 1335461: Pulling out stack capturing into standalone files.

https://reviewboard.mozilla.org/r/117282/#review148696

> This does not build, `kMaxChromeStacksKept` is undeclared.
> As far as i can see, `CombinedStacks` is now being used with two different values:
> - `kMaxChromeStacksKept`
> - `kMaximumNativeHangStacks`

Goog catch, Georg! I am on it.

Comment 43

[Iaroslav Sheptykin]

Comment on attachment 8843681 [details]
Bug 1335461: Pulling out stack capturing into standalone files.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/117282/diff/7-8/

Comment 44

[Iaroslav Sheptykin]

Comment on attachment 8843681 [details]
Bug 1335461: Pulling out stack capturing into standalone files.

https://reviewboard.mozilla.org/r/117282/#review148750

::: toolkit/components/telemetry/CombinedStacks.cpp:42
(Diff revisions 7 - 8)
>  size_t
>  CombinedStacks::AddStack(const Telemetry::ProcessedStack& aStack) {
>    // Advance the indices of the circular queue holding the stacks.
>    size_t index = mNextIndex++ % mMaxStacksCount;
>    // Grow the vector up to the maximum size, if needed.
> -  if (mStacks.size() < mMaxStacksCount) {
> +  if (mStacks.size() < kMaxChromeStacksKept) {

Oh, have overseen this. Fixing.

Comment 45

[Iaroslav Sheptykin]

Comment on attachment 8843681 [details]
Bug 1335461: Pulling out stack capturing into standalone files.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/117282/diff/8-9/

Comment 46

[Iaroslav Sheptykin]

Hi Georg!

I fixed the patch and resent it to try. The build completes on all platforms.

Comment 47

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/278354d1b831
Pulling out stack capturing into standalone files. r=gfritzsche

Comment 48

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/278354d1b831

Comment 49

[Gerry Chang [:gchang]]

54 RC build is released. Mark 54 won't fix.

Comment 50

[Landry Breuil (:gaston)]

Fwiw, this wont build on platforms without the profiler:


41:12.72 /usr/home/mozbot/buildslave/mozilla-central-freebsd-amd64/build/toolkit/components/telemetry/KeyedStackCapturer.cpp:167:1: error: extraneous closing brace ('}')
41:12.72 } // namespace mozilla

Some #ifdef/#endif to move around....

Comment 51

[Landry Breuil (:gaston)]

And this is probably not enough to fix, as this is in the log:

41:16.94 /usr/home/mozbot/buildslave/mozilla-central-freebsd-amd64/build/toolkit/components/telemetry/Telemetry.cpp:102:27: error: no member named 'KeyedStackCapturer' in namespace 'mozilla::Telemetry'
41:16.94 using mozilla::Telemetry::KeyedStackCapturer;
41:16.94       ~~~~~~~~~~~~~~~~~~~~^
41:17.30 1 error generated.

cf http://buildbot.rhaalovely.net/builders/mozilla-central-freebsd-amd64/builds/1215/steps/build/logs/stdio

Comment 52

[Iaroslav Sheptykin]

Thanks :gaston! I filed a bug 1371927 on this.