989499 - Use FramePointerStackwalk on Windows

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Attachment: Add a way to get the top of the stack

Comment 2

[Jeff Muizelaar [:jrmuizel]]

Attachment: Use FramePointerStackWalk on Windows

Comment 4

[Benoit Girard (:BenWa)]

Comment on attachment 8398757 [details] [diff] [review]
Add a way to get the top of the stack

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

::: tools/profiler/StackTop.cpp
@@ +19,5 @@
> +  pthread_t thread = pthread_self();
> +  return pthread_get_stackaddr_np(thread);
> +#elif defined(XP_WIN)
> +#if defined(_MSC_VER) && defined(_M_IX86)
> +  // offset 0x18 from the FS segment register gives a pointer to

This could of used a reference.

::: tools/profiler/StackTop.h
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

@@ +4,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef MOZ_STACK_TOP_H
> +#define MOZ_STACK_TOP_H

Please use the mozilla include guard style: StackTop_h

::: tools/profiler/platform.cpp
@@ +820,5 @@
>  
>    PseudoStack* stack = new PseudoStack();
>    tlsPseudoStack.set(stack);
>    bool isMainThread = is_main_thread_name(aName);
> +  stackTop = GetStackTop(stackTop);

This line is ugly: maybe the incoming stackTop should be stackTopApproximate or something.

Comment 5

[Benoit Girard (:BenWa)]

Comment on attachment 8398757 [details] [diff] [review]
Add a way to get the top of the stack

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

And you forgot to include StackTop to the moz.build file

Comment 6

[Benoit Girard (:BenWa)]

Comment on attachment 8398758 [details] [diff] [review]
Use FramePointerStackWalk on Windows

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

Looks good but we should run some test and check that the quality vs. speed trade off is good. But it also means we don't risk deadlocking during startup (other times too?) so that on its own is nearly worth it.

Comment 7

[Benoit Girard (:BenWa)]

Attachment: add a way to get stack top (fix errors & rebased)

Comment 8

[Benoit Girard (:BenWa)]

Attachment: Use FramePointerStackwalk on windows (fixed & rebased)

Comment 9

[Benoit Girard (:BenWa)]

Attachment: Use FramePointerStackwalk on windows (fixed & rebased)

Fixed unification bug

Comment 10

[Benoit Girard (:BenWa)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=965cc2eee075

Comment 11

[Benoit Girard (:BenWa)]

Attachment: Use FramePointerStackwalk on windows (fixed & rebased)

Comment 12

[Benoit Girard (:BenWa)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=511998e197fd

Comment 13

[Benoit Girard (:BenWa)]

Attachment: Part 1: Add a way to get the stack top. r=BenWa

Comment 14

[Benoit Girard (:BenWa)]

Attachment: Part 2: Use FramePointerStackwalk on windows. r=rjmuizel

Comment 15

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b719feaefc02c0cfef1fb774f208b6cc1a7e006d
Bug 989499 - Part 1: Add a way to get the stack top. r=BenWa

https://hg.mozilla.org/integration/mozilla-inbound/rev/f377859c64293ec3f727412d07854038a591753b
Bug 989499 - Part 2: Use FramePointerStackwalk on windows. r=rjmuizel

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/b719feaefc02
https://hg.mozilla.org/mozilla-central/rev/f377859c6429

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c325a821106

Comment 18

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

https://hg.mozilla.org/mozilla-central/rev/9c325a821106