Closed Bug 989499 Opened 8 years ago Closed 6 years ago

Use FramePointerStackwalk on Windows

Categories

(Core :: Gecko Profiler, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: jrmuizel, Assigned: BenWa)

References

Details

Attachments

(2 files, 6 obsolete files)

No description provided.
Attachment #8398757 - Flags: review?(bgirard)
Attachment #8398758 - Flags: review?(bgirard)
Duplicate of this bug: 989498
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.
Attachment #8398757 - Flags: review?(bgirard) → review+
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 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.
Attachment #8398758 - Flags: review?(bgirard) → review-
Attachment #8398757 - Attachment is obsolete: true
Assignee: nobody → bgirard
Attachment #8398758 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Fixed unification bug
Attachment #8686816 - Attachment is obsolete: true
Attachment #8686849 - Attachment is obsolete: true
Attachment #8687323 - Flags: review?(jmuizelaar)
Attachment #8687323 - Flags: review?(jmuizelaar) → review+
Attachment #8686807 - Attachment is obsolete: true
Attachment #8687431 - Flags: review+
Attachment #8687323 - Attachment is obsolete: true
Attachment #8687433 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/b719feaefc02
https://hg.mozilla.org/mozilla-central/rev/f377859c6429
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Depends on: 1224822
You need to log in before you can comment on or make changes to this bug.