Pull out hang and stack classes into a separate source files

RESOLVED FIXED in Firefox 55

Status

()

defect
P3
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: gfritzsche, Assigned: yarik.sheptykin, Mentored)

Tracking

(Blocks 2 bugs)

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 wontfix, firefox55 fixed)

Details

(Whiteboard: [measurement:client])

Attachments

(2 attachments, 1 obsolete attachment)

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.
See Also: → 1272671
Duplicate of this bug: 1272671
Summary: Pull out KeyedStackCapturer into a separate source file → Pull out hang and stack classes into a separate source files
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?
Attachment #8843681 - Flags: review?(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?
Attachment #8843681 - Flags: review?(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?
Attachment #8843681 - Flags: review?(gfritzsche)
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 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 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 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.
Attachment #8843681 - Flags: review?(gfritzsche)
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?
Flags: needinfo?(gfritzsche)
If its not easy to fix, i'm fine with just leaving it in the new review request.
Flags: needinfo?(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?
Flags: needinfo?(michael)
Flags: needinfo?(mconley)
(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.
Flags: needinfo?(michael)
(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?
Mentor: gfritzsche
Attachment #8868143 - Flags: review?(gfritzsche)
Attachment #8843681 - Attachment is obsolete: true
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?
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.
Flags: needinfo?(gfritzsche)
Hi Georg! You can review https://reviewboard.mozilla.org/r/117282/. This review contains the latest patch that builds locally.
I can't think of anything BHR-wise that'll be impacted negatively by this change.
Flags: needinfo?(mconley)
Blocks: 1367344
(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.
Flags: needinfo?(gfritzsche)
Attachment #8868143 - Attachment is obsolete: true
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.
Attachment #8843681 - Flags: review?(gfritzsche)
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/
Flags: needinfo?(gfritzsche)
Nice, that makes it easier, thanks!
I'm not seeing an attached tar file here though?
Flags: needinfo?(gfritzsche) → needinfo?(yarik.sheptykin)
I attached the file to the review hoping that reviewboard will make it magically available on bugzilla. Attaching the file manually.
Flags: needinfo?(yarik.sheptykin)
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
Attachment #8843681 - Flags: review+
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`.
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)
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?
See Comment 37
Flags: needinfo?(gfritzsche)
That is fine, i think you can just rebase & update the patch.
I can trigger landing for you.
Flags: needinfo?(gfritzsche)
I re-based the patch already and updated the review (see diff #7). I should be able to land now.
Flags: needinfo?(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 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 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.
Hi Georg!

I fixed the patch and resent it to try. The build completes on all platforms.
Flags: needinfo?(gfritzsche)
Pushed by georg.fritzsche@googlemail.com:
https://hg.mozilla.org/integration/autoland/rev/278354d1b831
Pulling out stack capturing into standalone files. r=gfritzsche
Blocks: 1370489
https://hg.mozilla.org/mozilla-central/rev/278354d1b831
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
54 RC build is released. Mark 54 won't fix.
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....
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
Thanks :gaston! I filed a bug 1371927 on this.
Depends on: 1371927
You need to log in before you can comment on or make changes to this bug.