856331 - Make breakpad unwinding runtime switchable

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Benoit Girard (:BenWa)]

With the refactoring switching between breakpad and NS_Stackwalk unwind is relatively easily. We just need to add a 'feature' and move some code from init/shutdown to start/stop.

Comment 1

[Benoit Girard (:BenWa)]

Attachment: patch

With this patch and some changes to the extension we can now toggle the use of the unwinder thread from the extension.

Comment 2

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=36314f03b7ed

Comment 3

[Benoit Girard (:BenWa)]

Attachment: patch rebased

This patch is required to start/stop the profiler. Otherwise it crashes.

Comment 4

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=7c6278382a83

Comment 5

[Benoit Girard (:BenWa)]

Attachment: patch rebased

I had to rebase this on top multi-threaded profile since it will be merged tomorrow.

Comment 6

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=8723e2794d1c

Comment 7

[Julian Seward [:jseward]]

Comment on attachment 739296 [details] [diff] [review]
patch rebased

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

r+ provided the two mentioned issues are fixed.

::: tools/profiler/UnwinderThread2.cpp
@@ +1538,5 @@
>    }
> +
> +  g_stackLimitsUsed = 0;
> +  g_seqNo = 0;
> +  free(g_buffers);

I would prefer if we didn't free g_buffers at this point.  There may
be a race with the sampling threads which could potentially cause them
to segfault.  There's a comment about this in UnwinderThread2.cpp,
starting with "These calloc-ations are never freed ..."

I think a safer approach, when the multithreading profiling support
has settled down, is to have a separate bug to assess thread safety
(via Helgrind) and memory leakyness of the profiler back end, and fix
up any races and leaks that we can find.

::: tools/profiler/platform.cpp
@@ +267,5 @@
>    tlsPseudoStack.set(stack);
>  
>    Sampler::RegisterCurrentThread("Gecko", stack, true);
>  
> +  sps_version2();

I didn't understand this .. did you intend to call it?  It
returns a bool which this call now ignores.

Comment 8

[Julian Seward [:jseward]]

> I would prefer if we didn't free g_buffers at this point.

I meant to add, "nor null it out.", but remembered too late.

Comment 9

[Benoit Girard (:BenWa)]

Attachment: patch v2

MOZ_PROFILER_NEW no longer regresses, fixed comments.

Comment 10

[Benoit Girard (:BenWa)]

Attachment: patch v3

Fixed a bug with restarting breakpad unwinding. I now reset exit_now.

Comment 11

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3dad6c069978

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3dad6c069978