1357829 - Make stack walking logic in ThreadStackHelper support windows x64

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement, P2)

Description

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

It was changed not to support x64 in bug 1346415 to avoid deadlocking issues caused by lowering the BHR threshold in infra with MozStackWalk. Ideally this should be fixed so that it works on x64, especially as we're encouraging more people to move to that version of firefox.

Comment 1

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

Attachment: Part 1: Expose profiler_suspend_sample_thread

This patch performs a refactoring to the internals of the profiler in order to
expose a function, profiler_suspend_sample_thread, which can be called from a
background thread to suspend, sample the native stack, and then resume the
target passed-in thread.

The interface was designed to expose as few internals of the profiler as
possible, exposing only a single callback which accepts the list of program
counters and stack pointers collected during the backtrace.

A method `profiler_current_thread_id` was also added to get the thread_id of the
current thread, which can then be passed by another thread into
profiler_suspend_sample_thread to sample the stack of that thread.

This is implemented in two parts:

1) Splitting SamplerThread into two classes: Sampler, and SamplerThread.

Sampler was created to extract the core logic from SamplerThread which manages
unix signals on android and linux, as well as suspends the target thread on all
platforms. SamplerThread was then modified to subclass this type, adding the
extra methods and fields required for the creation and management of the actual
Sampler Thread.

Some work was done to ensure that the methods on Sampler would not require
ActivePS to be present, as we intend to sample threads when the profiler is not
active for the Background Hang Reporter.

2) Moving the Tick() logic into the TickController interface.

A TickController interface was added to platform which has 2 methods: Tick and
Backtrace. The Tick method replaces the previous Tick() static method, allowing
it to be overridden by a different consumer of SuspendAndSampleAndResumeThread,
while the Backtrace() method replaces the previous MergeStacksIntoProfile
method, allowing it to be overridden by different consumers of
DoNativeBacktrace.

This interface object is then used to wrap implementation specific data, such as
the ProfilerBuffer, and is threaded through the SuspendAndSampleAndResumeThread
and DoNativeBacktrace methods.

This change added 2 virtual calls to the SamplerThread's critical section, which
I believe should be a small enough overhead that it will not affect profiling
performance. These virtual calls could be avoided using templating, but I
decided that doing so would be unnecessary.

MozReview-Commit-ID: AT48xb2asgV

Comment 2

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

Attachment: Part 2: Use profiler_suspend_sample_thread in the background hang monitor

This patch uses the profiler_suspend_sample_thread method which was added in
part 1.

With this patch, we no longer manually run code to pause the target thread,
instead using the profiler's provided code to do so. In addition, we no longer
manually walk the stack to collect native stack frames, instead relying on the
profiler's cross-platform stack walking logic.

This helps remove some of the code from ThreadStackHelper which was redundant
with the profiler. Much of the pseudostack code in ThreadStackHelper is also
redundant, and should hopefully be eliminated in a follow-up.

MozReview-Commit-ID: 4RjLHt6inH9

Comment 3

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

Attachment: Part 1: Expose profiler_suspend_sample_thread

This patch performs a refactoring to the internals of the profiler in order to
expose a function, profiler_suspend_sample_thread, which can be called from a
background thread to suspend, sample the native stack, and then resume the
target passed-in thread.

The interface was designed to expose as few internals of the profiler as
possible, exposing only a single callback which accepts the list of program
counters and stack pointers collected during the backtrace.

A method `profiler_current_thread_id` was also added to get the thread_id of the
current thread, which can then be passed by another thread into
profiler_suspend_sample_thread to sample the stack of that thread.

This is implemented in two parts:

1) Splitting SamplerThread into two classes: Sampler, and SamplerThread.

Sampler was created to extract the core logic from SamplerThread which manages
unix signals on android and linux, as well as suspends the target thread on all
platforms. SamplerThread was then modified to subclass this type, adding the
extra methods and fields required for the creation and management of the actual
Sampler Thread.

Some work was done to ensure that the methods on Sampler would not require
ActivePS to be present, as we intend to sample threads when the profiler is not
active for the Background Hang Reporter.

2) Moving the Tick() logic into the TickController interface.

A TickController interface was added to platform which has 2 methods: Tick and
Backtrace. The Tick method replaces the previous Tick() static method, allowing
it to be overridden by a different consumer of SuspendAndSampleAndResumeThread,
while the Backtrace() method replaces the previous MergeStacksIntoProfile
method, allowing it to be overridden by different consumers of
DoNativeBacktrace.

This interface object is then used to wrap implementation specific data, such as
the ProfilerBuffer, and is threaded through the SuspendAndSampleAndResumeThread
and DoNativeBacktrace methods.

This change added 2 virtual calls to the SamplerThread's critical section, which
I believe should be a small enough overhead that it will not affect profiling
performance. These virtual calls could be avoided using templating, but I
decided that doing so would be unnecessary.

MozReview-Commit-ID: AT48xb2asgV

Comment 4

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

Attachment: Part 2: Use profiler_suspend_sample_thread in the background hang monitor

This patch uses the profiler_suspend_sample_thread method which was added in
part 1.

With this patch, we no longer manually run code to pause the target thread,
instead using the profiler's provided code to do so. In addition, we no longer
manually walk the stack to collect native stack frames, instead relying on the
profiler's cross-platform stack walking logic.

This helps remove some of the code from ThreadStackHelper which was redundant
with the profiler. Much of the pseudostack code in ThreadStackHelper is also
redundant, and should hopefully be eliminated in a follow-up.

MozReview-Commit-ID: 4RjLHt6inH9

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 8865008 [details] [diff] [review]
Part 2: Use profiler_suspend_sample_thread in the background hang monitor

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

Such simple.  Much deletion.  Very improvement.

Comment 6

[Nicholas Nethercote [inactive]]

Comment on attachment 8865008 [details] [diff] [review]
Part 2: Use profiler_suspend_sample_thread in the background hang monitor

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

::: xpcom/threads/ThreadStackHelper.cpp
@@ -133,5 @@
> -#ifdef MOZ_THREADSTACKHELPER_NATIVE
> -    | THREAD_GET_CONTEXT | THREAD_QUERY_INFORMATION
> -#endif
> -    , FALSE, 0);
> -  mStackTop = profiler_get_stack_top();

I think profiler_get_stack_top() can be removed now.

@@ +77,3 @@
>  }
>  
>  ThreadStackHelper::~ThreadStackHelper()

You could remove the destructor now.

@@ +104,5 @@
> +  GetStacksInternal(nullptr, &aNativeStack);
> +}
> +
> +void
> +ThreadStackHelper::GetPseudoAndNativeStack(Stack& aStack, NativeStack& aNativeStack)

I wish the various Stack/stack/aStack names in the BHR were instead PseudoStack/pseudoStack/aPseudoStack, for better contrast with the native stack. Might be too much of a change for this patch, but would be a nice follow-up.

Comment 7

[Nicholas Nethercote [inactive]]

Comment on attachment 8865007 [details] [diff] [review]
Part 1: Expose profiler_suspend_sample_thread

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

I've done a partial review, and I might not get a chance to look at this again until next week (due to jury duty), so here's what I have so far. Adding three new classes definitely makes things harder to follow, which I am concerned about, and I'm wondering if it could be simplified at all.

I also think the word "Tick" has now been stretched to the point of being meaningless and probably should be changed, though not in this patch.

::: tools/profiler/core/platform-win32.cpp
@@ +105,5 @@
> +                                         TickSample& aSample)
> +{
> +  HANDLE profiled_thread = aSample.mPlatformData->ProfiledThread();
> +  if (profiled_thread == nullptr)
> +    return;

Add braces while you're moving this code.

::: tools/profiler/core/platform.cpp
@@ +1301,5 @@
> +  // counter. It is called within a signal and so must be re-entrant.
> +  void Tick(PSLockRef aLock, const TickSample& aSample) override;
> +
> +  void
> +  Backtrace(PSLockRef aLock,

Inline methods generally have the return type on the same line.

@@ +3190,5 @@
> +
> +  void Backtrace(PSLockRef aLock, const TickSample& aSample, NativeStack& aNativeStack) override
> +  {
> +    mCallback.ThreadSuspended(aNativeStack.pc_array, aNativeStack.sp_array, aNativeStack.count);
> +  }

I don't think this method is ever called? If not, please replace its body with MOZ_CRASH() or equivalent and a comment explaining why.

@@ +3214,5 @@
> +
> +    if (info->ThreadId() == aThreadId) {
> +      // Suspend, sample, and then resume the target thread.
> +      Sampler sampler(lock);
> +      TickSample sample(info, 0, 0);

Prior to this patch we've talked about two kinds of samples: (a) periodic samples, which are done off-thread in response to a timer, and (b) synchronous samples, which are done on-thread in response to an API call.

This patch introduces a new kind, which are done off-thread in response to an API call. They're more similar to (a) than (b), AFAICT. Some comments (e.g. on TickSample's constructors) are going to need updating, and we might need some new terminology.

::: tools/profiler/public/GeckoProfiler.h
@@ +320,5 @@
>  // The thread must have been previously registered with the profiler, otherwise
>  // this method will return nullptr.
>  PROFILER_FUNC(void* profiler_get_stack_top(), nullptr)
>  
> +PROFILER_FUNC(int profiler_current_thread_id(), 0)

Is this new function needed? I don't see it used in this patch or the subsequent patch.

@@ +330,5 @@
> +  // pointers if aSampleNative is true.
> +  //
> +  // NOTE: This method is called while the target thread is suspended. Do not
> +  // try to allocate or acquire any locks, or you could deadlock.
> +  virtual void ThreadSuspended(void** aPCs, void** aSPs, size_t aCount) = 0;

The one use of this class, in the subsequent patch, doesn't use aSPs. Are you anticipating future uses that will?

@@ +333,5 @@
> +  // try to allocate or acquire any locks, or you could deadlock.
> +  virtual void ThreadSuspended(void** aPCs, void** aSPs, size_t aCount) = 0;
> +};
> +
> +// This method suspends the thread identified by aThread, optionally samples it

s/aThread/aThreadId/

@@ +336,5 @@
> +
> +// This method suspends the thread identified by aThread, optionally samples it
> +// for its native stack, and then calls the consumer callback. The target thread
> +// is no longer suspended when this method returns.
> +PROFILER_FUNC_VOID(profiler_suspend_sample_thread(int aThreadId,

Can you rename this as profiler_suspend_and_sample_thread? That makes it clearer that "sample" is a verb. I initially tried to parse "sampler thread" as a noun phrase, because of the pre-existing SamplerThread class.

Comment 8

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

(In reply to Nicholas Nethercote [:njn] (on jury duty until ~May 17) from comment #7)
> Comment on attachment 8865007 [details] [diff] [review]
> Part 1: Expose profiler_suspend_sample_thread
> 
> Review of attachment 8865007 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I've done a partial review, and I might not get a chance to look at this
> again until next week (due to jury duty), so here's what I have so far.
> Adding three new classes definitely makes things harder to follow, which I
> am concerned about, and I'm wondering if it could be simplified at all.

I'm not sure how much more I can simplify this. The idea which I had was to 

a) Not require the SamplerThread object in order to collect a sample - however there was some information which I needed which was only stored on SamplerThread, so I created the Sampler base class which contained that piece of information which I needed.

b) Re-use the thread suspending logic between the different code paths (periodic samples and on demand samples), so I added a type (TickController) which could be subclassed to specify this behavior. I also modified DoNativeBacktrace to take this type so that I could control the callback behavior, but this could also be handled by allocating the NativeStack object in the caller, and passing in a reference to it, which I might do in a follow-up.

The other classes were just the 2 subclasses of TickController which specify the individual tick behaviors. I imagine that they will be clearer once a different name is used rather than Tick.

c) I introduced a new public API to the profiler which uses a distinct type for defining the callback in order to avoid exposing Profiler internals. This type is only used in the API, and not internally, so doesn't increase the complexity too much IMO. I also considered using std::function<> for this, and could change if it is more clear.

Do you have any recommendations for how I could simplify this without introducing these new helper types.

> I also think the word "Tick" has now been stretched to the point of being
> meaningless and probably should be changed, though not in this patch.

Agreed. I tried to not change terminology in this patch, but we should probably fix it in a follow-up.


> @@ +3190,5 @@
> > +
> > +  void Backtrace(PSLockRef aLock, const TickSample& aSample, NativeStack& aNativeStack) override
> > +  {
> > +    mCallback.ThreadSuspended(aNativeStack.pc_array, aNativeStack.sp_array, aNativeStack.count);
> > +  }
> 
> I don't think this method is ever called? If not, please replace its body
> with MOZ_CRASH() or equivalent and a comment explaining why.

This method is called in the second patch. It is part of the new API surface which I am providing from the profiler to collect the native backtrace for a specific thread.

> @@ +3214,5 @@
> > +
> > +    if (info->ThreadId() == aThreadId) {
> > +      // Suspend, sample, and then resume the target thread.
> > +      Sampler sampler(lock);
> > +      TickSample sample(info, 0, 0);
> 
> Prior to this patch we've talked about two kinds of samples: (a) periodic
> samples, which are done off-thread in response to a timer, and (b)
> synchronous samples, which are done on-thread in response to an API call.
> 
> This patch introduces a new kind, which are done off-thread in response to
> an API call. They're more similar to (a) than (b), AFAICT. Some comments
> (e.g. on TickSample's constructors) are going to need updating, and we might
> need some new terminology.

Perhaps we do. I can read over the comments and make sure that they are still up to date before you get back from jury duty.

> ::: tools/profiler/public/GeckoProfiler.h
> @@ +320,5 @@
> >  // The thread must have been previously registered with the profiler, otherwise
> >  // this method will return nullptr.
> >  PROFILER_FUNC(void* profiler_get_stack_top(), nullptr)
> >  
> > +PROFILER_FUNC(int profiler_current_thread_id(), 0)
> 
> Is this new function needed? I don't see it used in this patch or the
> subsequent patch.

This method is called in the subsequent patch to get the current thread id in ThreadStackHelper's constructor.

> @@ +330,5 @@
> > +  // pointers if aSampleNative is true.
> > +  //
> > +  // NOTE: This method is called while the target thread is suspended. Do not
> > +  // try to allocate or acquire any locks, or you could deadlock.
> > +  virtual void ThreadSuspended(void** aPCs, void** aSPs, size_t aCount) = 0;
> 
> The one use of this class, in the subsequent patch, doesn't use aSPs. Are
> you anticipating future uses that will?

I don't currently have any use cases which will, but I was trying to not make this information specific to the ThreadStackHelper use case, so provided this information. I would be OK with not providing this information through the new profiler_suspend_and_sample_thread API.

Comment 9

[Nicholas Nethercote [inactive]]

> Do you have any recommendations for how I could simplify this without introducing these new helper types.

Not at the moment. Everything you've said sounds reasonable. It's just that I read the body of 
profiler_suspend_sample_thread() and still have trouble visualizing how the pieces fit together. I'll think some more, though I may not be able to come up with anything simpler.

Comment 10

[Nicholas Nethercote [inactive]]

I looked at this some more and have a better idea about what I don't like (beyond the stretching of the meaning of "Tick"). 

For ProfilerThreadSuspendedCallback I wonder if just using a lambda would be simpler.

But my main concern involves TickController. We now have three ways of taking samples.

* Periodic (SamplerThread::Run): suspend named thread, take full sample, write full sample into the ProfileBuffer
  - uses ProfilerTickController
  - uses first TickSample constructor

* Synchronous (profiler_get_backtrace()): on current thread, get backtrace, write backtrace into the ProfileBuffer
  - uses ProfilerTickController
  - uses second TickSample constructor + PopulateContext()

* (New) Simple: suspend named thread, get backtrace, pass it to a callback
  - uses SimpleTickController
  - uses first TickSample constructor (with zero memory sizes)

So, the differences are partly covered by the TickController subclassing, and partly by the two constructor/versions of TickSample and the TickSample::mIsSynchronous field. It's an awkward arrangement.

But I can't specifically see how to improve it yet; I will have to poke around some. And part 2 does simplify the BHR a lot. So I'm ok if you land this now, and I will try to streamline it afterwards.

Comment 11

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

Attachment: Part 1: Expose profiler_suspend_and_sample_thread

I made some changes to try to get it to match your requests a bit better, and they're significant enough that I feel I should re-ask for review. Sorry about that :)

MozReview-Commit-ID: AT48xb2asgV

Comment 12

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

Attachment: Part 2: Use profiler_suspend_sample_thread in the background hang monitor

I had to disable this on linux because initializing LUL for the first time was holding the profiler lock for >1s during tests and causing stuff to time out.

I'm re-asking for review for that change.

MozReview-Commit-ID: 4RjLHt6inH9

Comment 13

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

Attachment: Part 3: Remove profiler_get_stack_top

MozReview-Commit-ID: C4DvuOvYSrs

Comment 14

[Nathan Froyd [:froydnj]]

Comment on attachment 8868759 [details] [diff] [review]
Part 2: Use profiler_suspend_sample_thread in the background hang monitor

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

r=me on the xpcshell.ini change only.

::: xpcom/threads/ThreadStackHelper.cpp
@@ +137,5 @@
>  #ifdef MOZ_THREADSTACKHELPER_NATIVE
> +    if (mNativeStackToFill) {
> +      while (aCount-- &&
> +             mNativeStackToFill->size() < mNativeStackToFill->capacity()) {
> +        mNativeStackToFill->push_back(reinterpret_cast<uintptr_t>(aPCs[aCount]));

I don't suppose there's any way to use vector::insert here, which would presumably convert this into a reserve() + memcpy?

Comment 15

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

(In reply to Nathan Froyd [:froydnj] from comment #14)
> Comment on attachment 8868759 [details] [diff] [review]
> Part 2: Use profiler_suspend_sample_thread in the background hang monitor
> 
> Review of attachment 8868759 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me on the xpcshell.ini change only.
> 
> ::: xpcom/threads/ThreadStackHelper.cpp
> @@ +137,5 @@
> >  #ifdef MOZ_THREADSTACKHELPER_NATIVE
> > +    if (mNativeStackToFill) {
> > +      while (aCount-- &&
> > +             mNativeStackToFill->size() < mNativeStackToFill->capacity()) {
> > +        mNativeStackToFill->push_back(reinterpret_cast<uintptr_t>(aPCs[aCount]));
> 
> I don't suppose there's any way to use vector::insert here, which would
> presumably convert this into a reserve() + memcpy?

Potentially. The backing buffer should already be reserved (as if it was not reserved, then we could deadlock due to allocations - the main thread is blocked at this time. That's why I assert that I won't exceed capacity). I could probably just write this as getting the pointer and memcpy-ing the first aCount members out of it, then setting the length on the vector (assuming that there's a method to do that).

Comment 16

[Nicholas Nethercote [inactive]]

Comment on attachment 8868758 [details] [diff] [review]
Part 1: Expose profiler_suspend_and_sample_thread

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

This is much improved! Moving MergeStacksIntoProfile() was a really good idea, that helped simplify things up a lot.

::: tools/profiler/core/platform.cpp
@@ +760,5 @@
> +  // NOTE: This method is called when the target thread of the sample is paused.
> +  // Do not allocate or attempt to grab any locks during this function call.
> +  virtual void Tick(PSLockRef aLock,
> +                    const TickSample& aSample) = 0;
> +};

Now that you've reduced TickController to a single method, I'll make the same suggestion as I did for ProfilerThreadSuspendedCallback: can it be changed to a closure? (You can tell I'm not a big fan of single method classes :)

I'm not sure if it will work out nicely, and I'll let you decide, but please consider it.

@@ +1250,4 @@
>  {
> +public:
> +  explicit ProfilerTickController(PSLockRef aLock, ProfileBuffer* aBuffer = nullptr)
> +    : mBuffer(aBuffer ? aBuffer : ActivePS::Buffer(aLock))

I'd prefer it if you made |aBuffer| non-optional; it makes the code simpler and no more verbose.

@@ +1290,4 @@
>    } else
>  #endif
>    {
> +    DoSampleStackTrace(aLock, mBuffer, aSample);

Now that MergeStacksIntoProfile is called by Tick() for the native case, I think it would be reasonable to inline DoSampleStackTrace() here so the non-native case more obviously matches the native case.

@@ +3074,5 @@
> +      DoNativeBacktrace(aLock, nativeStack, aSample);
> +    }
> +#endif
> +
> +    mCallback(nativeStack.pc_array, nativeStack.count);

Is it useful calling mCallback in the case where we haven't done a native unwind? It's just going to be a no-op anyway, right?

Comment 17

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

Attachment: Part 2: Use profiler_suspend_sample_thread in the background hang monitor

This patch uses the profiler_suspend_sample_thread method which was added in
part 1.

With this patch, we no longer manually run code to pause the target thread,
instead using the profiler's provided code to do so. In addition, we no longer
manually walk the stack to collect native stack frames, instead relying on the
profiler's cross-platform stack walking logic.

This helps remove some of the code from ThreadStackHelper which was redundant
with the profiler. Much of the pseudostack code in ThreadStackHelper is also
redundant, and should hopefully be eliminated in a follow-up.

MozReview-Commit-ID: 4RjLHt6inH9

Comment 18

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

Attachment: Part 4: Remove TickController

I replaced the TickController with a lambda in this function. I also tried to
apply the other changes you suggested.

> Is it useful calling mCallback in the case where we haven't done a native
> unwind? It's just going to be a no-op anyway, right?

We use the callback to collect pseudostacks even when we aren't collecting
native stacks. I wanted to re-use the thread pausing infrastructure from the
profiler in this code. That's why we call the callback even if we didn't collect
any native stacks.

MozReview-Commit-ID: 2IHa6ybR9ug

Comment 19

[Nicholas Nethercote [inactive]]

Comment on attachment 8868834 [details] [diff] [review]
Part 4: Remove TickController

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

Looks good.

One question: you used std::function in part 1, but a template parameter here. Is that because the std::function is going to be called infrequently, while the template parameter function is going to be called frequently?

Comment 20

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

(In reply to Nicholas Nethercote [:njn] from comment #19)
> One question: you used std::function in part 1, but a template parameter
> here. Is that because the std::function is going to be called infrequently,
> while the template parameter function is going to be called frequently?

You can only use a template parameter when the caller and callee are in the same compilation unit. I used std::function in part 1 in order to create an abstraction boundary (as I didn't want to specialize all of the profiler for each consumer of that public API, or move the implementation details into a header), while in part 2 I used a template parameter to potentially avoid virtual function call overhead during profiler ticks.  Basically it was free to do (as platform.cpp and platform-$TARGET.cpp are compiled in the same compilation unit), and could theoretically improve performance.

Comment 21

[Nathan Froyd [:froydnj]]

(In reply to Michael Layzell [:mystor] from comment #20)
> Basically it was free
> to do (as platform.cpp and platform-$TARGET.cpp are compiled in the same
> compilation unit), and could theoretically improve performance.

Please do not do this: relying on cargo-culting includes is bad enough, but relying on things winding up in the same unified file for template instantiation purposes potentially makes life painful later on.  (Fuzzing also likes non-unified builds, if they can get them to work, and throwing obstacles in their way like this makes them grumpy.)

Comment 22

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

(In reply to Nathan Froyd [:froydnj] from comment #21)
> Please do not do this: relying on cargo-culting includes is bad enough, but
> relying on things winding up in the same unified file for template
> instantiation purposes potentially makes life painful later on.  (Fuzzing
> also likes non-unified builds, if they can get them to work, and throwing
> obstacles in their way like this makes them grumpy.)

They are part of the same compliation unit explicitly. `platform.cpp` directly #include-s platform-$TARGET.cpp. http://searchfox.org/mozilla-central/rev/f55349994fdac101d121b11dac769f3f17fbec4b/tools/profiler/core/platform.cpp#1868-1879

I'm not trying to do anything tricksy, and I'm not taking advantage of unified builds - I'm just taking advantage of a decision which was already made to make the platform.cpp file be built as one compilation unit to directly use internal functions and types. Is that OK?

Comment 23

[Nathan Froyd [:froydnj]]

(In reply to Michael Layzell [:mystor] from comment #22)
> I'm not trying to do anything tricksy, and I'm not taking advantage of
> unified builds - I'm just taking advantage of a decision which was already
> made to make the platform.cpp file be built as one compilation unit to
> directly use internal functions and types. Is that OK?

That's what I get for grumping about things without looking carefully.  Yes, that's totally OK!  Sorry for the noise.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/86ebe868d443
Part 1: Expose profiler_suspend_and_sample_thread, r=njn
https://hg.mozilla.org/integration/mozilla-inbound/rev/378d473c9619
Part 2: Use profiler_suspend_sample_thread in the background hang monitor, r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/cebe4d7abeda
Part 3: Remove profiler_get_stack_top, r=njn
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ea202bb1103
Part 4: Remove TickController, r=njn

Comment 25

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

https://hg.mozilla.org/mozilla-central/rev/86ebe868d443
https://hg.mozilla.org/mozilla-central/rev/378d473c9619
https://hg.mozilla.org/mozilla-central/rev/cebe4d7abeda
https://hg.mozilla.org/mozilla-central/rev/8ea202bb1103

Comment 26

[Iris Hsiao [:ihsiao]]

Backed out for build busted in xpcshell\selftest.py on windows 8 x64 opt like https://treeherder.mozilla.org/logviewer.html#?job_id=100172125&repo=mozilla-inbound&lineNumber=27059

Comment 27

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/8e98dab5054d
Backed out 4 changesets for build bustage in xpcshell\selftest.py on windows 8 x64 opt. a=backout

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f51d1afdf9d
Part 1: Expose profiler_suspend_and_sample_thread, r=njn
https://hg.mozilla.org/integration/mozilla-inbound/rev/6123a6b2b749
Part 2: Use profiler_suspend_sample_thread in the background hang monitor, r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/14e520f092c0
Part 3: Remove profiler_get_stack_top, r=njn
https://hg.mozilla.org/integration/mozilla-inbound/rev/980a145fce86
Part 4: Remove TickController, r=njn

Comment 29

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/8f51d1afdf9d
https://hg.mozilla.org/mozilla-central/rev/6123a6b2b749
https://hg.mozilla.org/mozilla-central/rev/14e520f092c0
https://hg.mozilla.org/mozilla-central/rev/980a145fce86