Yes, a (small?) CL to get sampling on Windows to parity with how it's
already working on Linux sounds good.
(I'm not familiar with the specifics of how sampling works or should work.)
On Mon, Aug 3, 2020 at 1:58 PM Alex Kodat <alexko...@gmail.com> wrote:
> OK, now I'm replying to myself a second time. Really sorry. I realized we
> don't need any enhancement to sampling other than getting the non-check for
> Isolate locked by debugged (suspended) thread fixed in Windows. For
> sampling when Isolate is unlocked I can simply get time before and after an
> unlock on a profiled thread, do a StackTrace::CurrentStackTrace() and use
> that information to merge with the CpuProfiler::StopProfiling results,
> converting timer units to ticks. Not exactly rocket science.
>
> So the simple question then is should I do a PR to fix the sampling issue
> on Windows?
>
> Thanks and sorry about all the noise.
>
> On Sunday, August 2, 2020 at 3:04:03 PM UTC-5, Alex Kodat wrote:
>>
>> We're using the CpuProfiler in an Isolate that can have multiple threads
>> running and and taking turns in the Isolate via Unlocker. Frequently, we
>> want to profile what's happening on a thread so we do a StartProfiling for
>> a thread, let it run for a while, and then collect the results via
>> StopProfiling. Works great.
>>
>> However, I've noticed a difference between Windows and Linux in the
>> profiles, specifically, Windows collects samples for a thread when it's
>> inside an Unlocker bracket whereas Linux does not. So if we have a native
>> function sleep(), that unlocks the Isolate, in Windows I see a high hit
>> count for stacks that have functionName "sleep" and sourceType callback in
>> the bottom stack entry. In Linux, there are no hits where I'm inside the
>> sleep native function. For our purposes, the Windows behavior is much nicer
>> as we can easily see how much of the thread's time is being spent in sleep.
>> In Linux, there is absolutely no way of knowing because the information is
>> not captured.
>>
>> Unfortunately, Windows nicer (IMO) behavior comes at a cost.
>> Specifically, if indeed other threads do run in the Isolate while the
>> profiled thread is sampled when unlocked, there is a tendency for core
>> dumps while getting the stack trace. This is unsurprising because the stack
>> trace needs to look in the Isolate's heap to get things like function names
>> and that's not really a good idea when a thread doesn't have the Isolate
>> lock.
>>
>> The following code samples are all from src/libsampler/sampler.cc.
>>
>> Under Linux the profiler protects itself in SamplerManager::DoSample
>> (called by the SIGPROF handler) with:
>>
>> for (Sampler* sampler : samplers) {
>> ...
>> if (v8::Locker::IsActive() && !Locker::IsLocked(isolate)) continue;
>> sampler->SampleStack(state);
>> }
>>
>> That Locker::isLocked is a bit of a misnomer and really means
>> Locker::isLockedByCurrentThread and, because our SIGPROF handler runs on
>> the profiled thread, if that tread is in an Unlocker bracket in a native
>> function, isLocked returns false and we don't collect a sample.
>>
>> In Windows, the sampler does not run on the profiled thread and, instead,
>> does a SuspendThread of the profiled thread from another thread and then
>> uses GetThreadContext:
>>
>> void Sampler::DoSample() {
>> HANDLE profiled_thread = platform_data()->profiled_thread();
>> if (profiled_thread == nullptr) return;
>>
>> const DWORD kSuspendFailed = static_cast<DWORD>(-1);
>> if (SuspendThread(profiled_thread) == kSuspendFailed) return;
>>
>> // Context used for sampling the register state of the profiled thread.
>> CONTEXT context;
>> memset(&context, 0, sizeof(context));
>> context.ContextFlags = CONTEXT_FULL;
>> if (GetThreadContext(profiled_thread, &context) != 0) {
>> ...
>> SampleStack(state);
>> }
>>
>> There is no check if the Isolate is actually locked by the profiled
>> thread so we can get a sample for it but also, unfortunately, seg faults
>> and the like if another thread is running.
>>
>> So it seems that there really should be a check in Sampler::DoSample for
>> Windows to check if the Isolate is locked by the profiled thread. This
>> would require maybe adding another method to Locker like
>> isLockedByThread(ThreadId id) or something like that and then checking that
>> in Sampler::DoSample. When PlatformData is instantiated for Windows it
>> could stash the ThreadId for the current thread so it could be retrieved in
>> Sampler::DoSample. Doesn't seem all that daunting and am willing to put
>> together a PR for this if this seems reasonable and unless someone more
>> conversant with V8 internals doesn't pick it up.
>>
>> However, while this fix would eliminate Windows seg faults when doing
>> profiling in a multi-threaded environment, it would also make me sad
>> because now we would have no way of getting time spent unlocked in my
>> native function. To provide this functionality, it seems like it would be
>> nice to be able to create a provisional TickSample before the Isolate is
>> unlocked in a native function and use that in DoSample if the Isolate is
>> not locked by the profiled thread. Obviously, this would/should only be
>> done if the thread is being profiled.
>>
>> There are a lot of ways this could work but a straw man is that there
>> could be CpuProfiler methods called SaveProvisionalThreadSample and
>> ClearProvisionalThreadSample. The former, at least, would have to be called
>> while one holds the Isolate lock so one would presumably do such a call
>> right before Unlocker instantiation and then do a
>> ClearProvisionalThreadSample call right after the end of the Unlocker
>> context. Then, if DoSample decides that Isolate is not locked by the
>> sampled thread, it could look for a saved provisional sample for the thread
>> and, if available, use that for its sample. Note that a provisional
>> TickSample could be used many times as a thread might be unlocked over many
>> samples.
>>
>> Again, assuming no one else picks this up, happy to take a swing at a PR,
>> myself. While more complicated than the Windows fix, it doesn't seem that
>> daunting.
>>
>> Opinions?
>>
>> Sorry if this would have been more appropriately posted on v8-dev.
>>
>> Thanks
>>
>>
> --
> --
> v8-users mailing list
> v8-users@googlegroups.com
> http://groups.google.com/group/v8-users
> ---
> You received this message because you are subscribed to the Google Groups
> "v8-users" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to v8-users+unsubscr...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/v8-users/1bd503d6-ddec-4dca-9c76-55877b84e283o%40googlegroups.com
> <https://groups.google.com/d/msgid/v8-users/1bd503d6-ddec-4dca-9c76-55877b84e283o%40googlegroups.com?utm_medium=email&utm_source=footer>
> .
>
--
--
v8-users mailing list
v8-users@googlegroups.com
http://groups.google.com/group/v8-users
---
You received this message because you are subscribed to the Google Groups
"v8-users" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to v8-users+unsubscr...@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/v8-users/CAKSzg3RQrhwpCikgS5a9TY%3DcFWksAbzHpOk9wcz04%3DeiK%2BWqCA%40mail.gmail.com.
