Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1858143003:
Get rid of UnsafeCurrent in Sampler (Closed)
DescriptionGet rid of UnsafeCurrent in Sampler
Currently we are using UnsafeCurrent in async signal handler to acquire the
isolate of VM thread, but we want to get rid of that since it prevents V8 from
being thread agnostic.
This patch replaces UnsafeCurrent with a static map, where we store a map of
samplers for threads, and makes it accessible by signal handler.
BUG=v8:4889
LOG=n
Committed: https://crrev.com/62fb4775fea0d56d8a175baf1d902213f6752168
Cr-Commit-Position: refs/heads/master@{#35541}
Patch Set 1 #
Total comments: 1
Patch Set 2 : #Patch Set 3 : Use atomic-utils.h #Patch Set 4 : Update check-static-initializers.sh #
Total comments: 6
Patch Set 5 : Address fadi's comments #
Total comments: 28
Patch Set 6 : Address alph's comment #
Total comments: 9
Patch Set 7 : #
Total comments: 8
Patch Set 8 : Address aplh's comments. #Patch Set 9 : #
Total comments: 5
Patch Set 10 : #
Total comments: 4
Patch Set 11 : Address Yang's comments #Patch Set 12 : rebase #
Messages
Total messages: 36 (10 generated)
fmeawad@chromium.org changed reviewers: + fmeawad@chromium.org
https://codereview.chromium.org/1858143003/diff/1/src/profiler/sampler.cc File src/profiler/sampler.cc (right): https://codereview.chromium.org/1858143003/diff/1/src/profiler/sampler.cc#new... src/profiler/sampler.cc:266: bool is_block) Try to the the v8 base version at https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/atomic-util...
https://codereview.chromium.org/1858143003/diff/60001/src/profiler/sampler.cc File src/profiler/sampler.cc (right): https://codereview.chromium.org/1858143003/diff/60001/src/profiler/sampler.cc... src/profiler/sampler.cc:263: template <typename T> I do not think you need this class, I think you should use TrySetValue at point of call immediately instead. https://codereview.chromium.org/1858143003/diff/60001/src/profiler/sampler.h File src/profiler/sampler.h (right): https://codereview.chromium.org/1858143003/diff/60001/src/profiler/sampler.h#... src/profiler/sampler.h:96: // since CpuProfiler won't start the sampler but need to collect samples. I would rephrase this as // CpuProfiler collects samples without calling Start, to avoid deleting the sampler while in use by the CpuProfiler we register it. (or something similar)
https://codereview.chromium.org/1858143003/diff/60001/src/profiler/sampler.cc File src/profiler/sampler.cc (right): https://codereview.chromium.org/1858143003/diff/60001/src/profiler/sampler.cc... src/profiler/sampler.cc:263: template <typename T> On 2016/04/06 20:07:56, fmeawad wrote: > I do not think you need this class, I think you should use TrySetValue at point > of call immediately instead. lpy@ explained offline that it is a scoped context, Guard did not convey that meaning to me. https://codereview.chromium.org/1858143003/diff/60001/src/profiler/sampler.cc... src/profiler/sampler.cc:282: atomic_ = NULL; If you failed to set it, you should not reset it.
lpy@chromium.org changed reviewers: + alph@chromium.org
Thanks, I updated the CL. +alph@ for review. PTAL. https://codereview.chromium.org/1858143003/diff/60001/src/profiler/sampler.cc File src/profiler/sampler.cc (right): https://codereview.chromium.org/1858143003/diff/60001/src/profiler/sampler.cc... src/profiler/sampler.cc:282: atomic_ = NULL; On 2016/04/06 20:13:34, fmeawad wrote: > If you failed to set it, you should not reset it. Done. https://codereview.chromium.org/1858143003/diff/60001/src/profiler/sampler.h File src/profiler/sampler.h (right): https://codereview.chromium.org/1858143003/diff/60001/src/profiler/sampler.h#... src/profiler/sampler.h:96: // since CpuProfiler won't start the sampler but need to collect samples. On 2016/04/06 20:07:56, fmeawad wrote: > I would rephrase this as > // CpuProfiler collects samples without calling Start, to avoid deleting the > sampler while in use by the CpuProfiler we register it. > > (or something similar) Done.
https://codereview.chromium.org/1858143003/diff/80001/src/profiler/sampler.cc File src/profiler/sampler.cc (right): https://codereview.chromium.org/1858143003/diff/80001/src/profiler/sampler.cc... src/profiler/sampler.cc:243: class ThreadSamplersPair { Is it possible to use std::pair? https://codereview.chromium.org/1858143003/diff/80001/src/profiler/sampler.cc... src/profiler/sampler.cc:263: template <typename T> Why a template? Do you plan to use it elsewhere with a different type? https://codereview.chromium.org/1858143003/diff/80001/src/profiler/sampler.cc... src/profiler/sampler.cc:266: explicit AtomicGuard(AtomicValue<T>* atomic, T expected, T desired, Drop explicit Also, can you just hardcode expected=0, desired=1? https://codereview.chromium.org/1858143003/diff/80001/src/profiler/sampler.cc... src/profiler/sampler.cc:275: } while (is_block); while (is_block && !is_success_) https://codereview.chromium.org/1858143003/diff/80001/src/profiler/sampler.cc... src/profiler/sampler.cc:282: atomic_->SetValue(expected_); v8 style requires block statement here https://codereview.chromium.org/1858143003/diff/80001/src/profiler/sampler.cc... src/profiler/sampler.cc:289: T desired_; not used https://codereview.chromium.org/1858143003/diff/80001/src/profiler/sampler.cc... src/profiler/sampler.cc:792: samplers->Add(sampler); use {} https://codereview.chromium.org/1858143003/diff/80001/src/profiler/sampler.cc... src/profiler/sampler.cc:815: #if defined(USE_SIGNALS) you can have a single #if - #endif block for USE_SIGNALS https://codereview.chromium.org/1858143003/diff/80001/src/profiler/sampler.cc... src/profiler/sampler.cc:826: for (int i = 0; i < SamplerThread::thread_id_to_samplers_.length(); ++i) { Looks like thread_id_to_samplers_ should be a map, rather than list. https://codereview.chromium.org/1858143003/diff/80001/src/profiler/sampler.cc... src/profiler/sampler.cc:833: DCHECK(samplers != NULL); This is not true. The samplers could get deleted while the signal was in flight. https://codereview.chromium.org/1858143003/diff/80001/src/profiler/sampler.cc... src/profiler/sampler.cc:836: SignalHandler::CollectSample(context, sampler); drop SignalHandler:: https://codereview.chromium.org/1858143003/diff/80001/src/profiler/sampler.cc... src/profiler/sampler.cc:971: SamplerThread::RemoveSampler(this); {} https://codereview.chromium.org/1858143003/diff/80001/tools/check-static-init... File tools/check-static-initializers.sh (right): https://codereview.chromium.org/1858143003/diff/80001/tools/check-static-init... tools/check-static-initializers.sh:35: expected_static_init_count=3 Is it true on Windows?
Thanks, I updated the CL. PTAL. https://codereview.chromium.org/1858143003/diff/80001/src/profiler/sampler.cc File src/profiler/sampler.cc (right): https://codereview.chromium.org/1858143003/diff/80001/src/profiler/sampler.cc... src/profiler/sampler.cc:243: class ThreadSamplersPair { On 2016/04/06 22:21:26, alph wrote: > Is it possible to use std::pair? Done. https://codereview.chromium.org/1858143003/diff/80001/src/profiler/sampler.cc... src/profiler/sampler.cc:263: template <typename T> On 2016/04/06 22:21:26, alph wrote: > Why a template? Do you plan to use it elsewhere with a different type? Done. https://codereview.chromium.org/1858143003/diff/80001/src/profiler/sampler.cc... src/profiler/sampler.cc:266: explicit AtomicGuard(AtomicValue<T>* atomic, T expected, T desired, On 2016/04/06 22:21:26, alph wrote: > Drop explicit > Also, can you just hardcode expected=0, desired=1? I don't understand, what's the benefit to hard code? https://codereview.chromium.org/1858143003/diff/80001/src/profiler/sampler.cc... src/profiler/sampler.cc:275: } while (is_block); On 2016/04/06 22:21:26, alph wrote: > while (is_block && !is_success_) if is_block is set to false, then we only want to try to set the value of the atomic once. https://codereview.chromium.org/1858143003/diff/80001/src/profiler/sampler.cc... src/profiler/sampler.cc:282: atomic_->SetValue(expected_); On 2016/04/06 22:21:25, alph wrote: > v8 style requires block statement here Done. https://codereview.chromium.org/1858143003/diff/80001/src/profiler/sampler.cc... src/profiler/sampler.cc:289: T desired_; On 2016/04/06 22:21:25, alph wrote: > not used Done. https://codereview.chromium.org/1858143003/diff/80001/src/profiler/sampler.cc... src/profiler/sampler.cc:792: samplers->Add(sampler); On 2016/04/06 22:21:26, alph wrote: > use {} Done. https://codereview.chromium.org/1858143003/diff/80001/src/profiler/sampler.cc... src/profiler/sampler.cc:815: #if defined(USE_SIGNALS) On 2016/04/06 22:21:25, alph wrote: > you can have a single #if - #endif block for USE_SIGNALS Done. https://codereview.chromium.org/1858143003/diff/80001/src/profiler/sampler.cc... src/profiler/sampler.cc:826: for (int i = 0; i < SamplerThread::thread_id_to_samplers_.length(); ++i) { On 2016/04/06 22:21:26, alph wrote: > Looks like thread_id_to_samplers_ should be a map, rather than list. Done. https://codereview.chromium.org/1858143003/diff/80001/src/profiler/sampler.cc... src/profiler/sampler.cc:833: DCHECK(samplers != NULL); On 2016/04/06 22:21:26, alph wrote: > This is not true. The samplers could get deleted while the signal was in flight. Done. https://codereview.chromium.org/1858143003/diff/80001/src/profiler/sampler.cc... src/profiler/sampler.cc:836: SignalHandler::CollectSample(context, sampler); On 2016/04/06 22:21:26, alph wrote: > drop SignalHandler:: Done. https://codereview.chromium.org/1858143003/diff/80001/src/profiler/sampler.cc... src/profiler/sampler.cc:971: SamplerThread::RemoveSampler(this); On 2016/04/06 22:21:26, alph wrote: > {} Done. https://codereview.chromium.org/1858143003/diff/80001/tools/check-static-init... File tools/check-static-initializers.sh (right): https://codereview.chromium.org/1858143003/diff/80001/tools/check-static-init... tools/check-static-initializers.sh:35: expected_static_init_count=3 On 2016/04/06 22:21:26, alph wrote: > Is it true on Windows? Are we using check-static-initializers.sh on Windows? I only see this check on Linux build.
Description was changed from ========== Get rid of UnsafeCurrent in Sampler Currently we are using UnsafeCurrent in async signal handler to acquire the isolate of VM thread, but we want to get rid of that since it prevents V8 from being thread agnostic. This patch replace UnsafeCurrent with a static list, where we store a list of samplers for threads, and make it accessible by signal handler. BUG=v8:4889 LOG=n ========== to ========== Get rid of UnsafeCurrent in Sampler Currently we are using UnsafeCurrent in async signal handler to acquire the isolate of VM thread, but we want to get rid of that since it prevents V8 from being thread agnostic. This patch replace UnsafeCurrent with a static map, where we store a map of samplers for threads, and make it accessible by signal handler. BUG=v8:4889 LOG=n ==========
Some more comments. Thanks for extracting this part from the large patch! https://codereview.chromium.org/1858143003/diff/80001/src/profiler/sampler.cc File src/profiler/sampler.cc (right): https://codereview.chromium.org/1858143003/diff/80001/src/profiler/sampler.cc... src/profiler/sampler.cc:266: explicit AtomicGuard(AtomicValue<T>* atomic, T expected, T desired, On 2016/04/06 23:51:37, lpy wrote: > On 2016/04/06 22:21:26, alph wrote: > > Drop explicit > > Also, can you just hardcode expected=0, desired=1? > > I don't understand, what's the benefit to hard code? It is always 0,1 at the call sites. So you use it as a classic mutex everywhere. Why would you expose this complication to the class usages? https://codereview.chromium.org/1858143003/diff/80001/src/profiler/sampler.cc... src/profiler/sampler.cc:275: } while (is_block); On 2016/04/06 23:51:37, lpy wrote: > On 2016/04/06 22:21:26, alph wrote: > > while (is_block && !is_success_) > > if is_block is set to false, then we only want to try to set the value of the > atomic once. the code I suggest is semantically equivalent to your code. It just more compact and straightforward. https://codereview.chromium.org/1858143003/diff/100001/src/profiler/sampler.cc File src/profiler/sampler.cc (right): https://codereview.chromium.org/1858143003/diff/100001/src/profiler/sampler.c... src/profiler/sampler.cc:245: explicit AtomicGuard(AtomicValue<int>* atomic, int expected, int desired, Still please drop the explicit keyword. https://codereview.chromium.org/1858143003/diff/100001/src/profiler/sampler.c... src/profiler/sampler.cc:672: if (entry->value != NULL) { Is it possible to get NULL value? https://codereview.chromium.org/1858143003/diff/100001/src/profiler/sampler.c... src/profiler/sampler.cc:798: for (HashMap::Entry *p = SamplerThread::thread_id_to_samplers_.Start(); The point of switching to map was that you wouldn't need to iterate over it. Wouldn't lookup work here? https://codereview.chromium.org/1858143003/diff/100001/src/profiler/sampler.c... src/profiler/sampler.cc:805: if (entry == NULL || entry->value == NULL) { is it possible to have entry->value NULL here?
Thanks, I updated the CL. PTAL. https://codereview.chromium.org/1858143003/diff/100001/src/profiler/sampler.cc File src/profiler/sampler.cc (right): https://codereview.chromium.org/1858143003/diff/100001/src/profiler/sampler.c... src/profiler/sampler.cc:245: explicit AtomicGuard(AtomicValue<int>* atomic, int expected, int desired, On 2016/04/07 00:40:10, alph wrote: > Still please drop the explicit keyword. Done. https://codereview.chromium.org/1858143003/diff/100001/src/profiler/sampler.c... src/profiler/sampler.cc:672: if (entry->value != NULL) { On 2016/04/07 00:40:10, alph wrote: > Is it possible to get NULL value? Done. https://codereview.chromium.org/1858143003/diff/100001/src/profiler/sampler.c... src/profiler/sampler.cc:798: for (HashMap::Entry *p = SamplerThread::thread_id_to_samplers_.Start(); On 2016/04/07 00:40:10, alph wrote: > The point of switching to map was that you wouldn't need to iterate over it. > Wouldn't lookup work here? We are using profiled_thread_id as hash in map, in general we can use ThreadId::Current() to get thread id, but it's using thread local storage, while means we can't safely use it in signal handler, see here: https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/isolate.cc&... So I iterate the map here. https://codereview.chromium.org/1858143003/diff/100001/src/profiler/sampler.c... src/profiler/sampler.cc:805: if (entry == NULL || entry->value == NULL) { On 2016/04/07 00:40:10, alph wrote: > is it possible to have entry->value NULL here? Done.
https://codereview.chromium.org/1858143003/diff/100001/src/profiler/sampler.cc File src/profiler/sampler.cc (right): https://codereview.chromium.org/1858143003/diff/100001/src/profiler/sampler.c... src/profiler/sampler.cc:798: for (HashMap::Entry *p = SamplerThread::thread_id_to_samplers_.Start(); On 2016/04/07 15:13:54, lpy wrote: > On 2016/04/07 00:40:10, alph wrote: > > The point of switching to map was that you wouldn't need to iterate over it. > > Wouldn't lookup work here? > > We are using profiled_thread_id as hash in map, in general we can use > ThreadId::Current() to get thread id, but it's using thread local storage, while > means we can't safely use it in signal handler, see here: > https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/isolate.cc&... > > > So I iterate the map here. Can't you just lookup with thread_id used as a key? https://codereview.chromium.org/1858143003/diff/120001/src/profiler/sampler.cc File src/profiler/sampler.cc (right): https://codereview.chromium.org/1858143003/diff/120001/src/profiler/sampler.c... src/profiler/sampler.cc:245: AtomicGuard(bool is_block, AtomicValue<int>* atomic, nit: why did you swap the arguments? is_blocking should go second. also you can make it default to true. https://codereview.chromium.org/1858143003/diff/120001/src/profiler/sampler.c... src/profiler/sampler.cc:246: int expected = 0, int desired = 1) just drop these. if at some point in the future they will be needed (what I doubt), we can reintroduce them. https://codereview.chromium.org/1858143003/diff/120001/src/profiler/sampler.c... src/profiler/sampler.cc:253: } while (is_block); still, why don't you want to change it to: do { } while (is_block && !is_success_); https://codereview.chromium.org/1858143003/diff/120001/src/profiler/sampler.c... src/profiler/sampler.cc:802: if (entry == NULL) { nit: drop {}
Thanks for the feedback, I made some updates. PTAL. https://codereview.chromium.org/1858143003/diff/120001/src/profiler/sampler.cc File src/profiler/sampler.cc (right): https://codereview.chromium.org/1858143003/diff/120001/src/profiler/sampler.c... src/profiler/sampler.cc:245: AtomicGuard(bool is_block, AtomicValue<int>* atomic, On 2016/04/07 22:38:16, alph wrote: > nit: why did you swap the arguments? > is_blocking should go second. also you can make it default to true. Done. https://codereview.chromium.org/1858143003/diff/120001/src/profiler/sampler.c... src/profiler/sampler.cc:246: int expected = 0, int desired = 1) On 2016/04/07 22:38:17, alph wrote: > just drop these. if at some point in the future they will be needed (what I > doubt), we can reintroduce them. Done. https://codereview.chromium.org/1858143003/diff/120001/src/profiler/sampler.c... src/profiler/sampler.cc:253: } while (is_block); On 2016/04/07 22:38:17, alph wrote: > still, why don't you want to change it to: > do { } while (is_block && !is_success_); Done. https://codereview.chromium.org/1858143003/diff/120001/src/profiler/sampler.c... src/profiler/sampler.cc:802: if (entry == NULL) { On 2016/04/07 22:38:16, alph wrote: > nit: drop {} Done.
Description was changed from ========== Get rid of UnsafeCurrent in Sampler Currently we are using UnsafeCurrent in async signal handler to acquire the isolate of VM thread, but we want to get rid of that since it prevents V8 from being thread agnostic. This patch replace UnsafeCurrent with a static map, where we store a map of samplers for threads, and make it accessible by signal handler. BUG=v8:4889 LOG=n ========== to ========== Get rid of UnsafeCurrent in Sampler Currently we are using UnsafeCurrent in async signal handler to acquire the isolate of VM thread, but we want to get rid of that since it prevents V8 from being thread agnostic. This patch replaces UnsafeCurrent with a static map, where we store a map of samplers for threads, and makes it accessible by signal handler. BUG=v8:4889 LOG=n ==========
https://codereview.chromium.org/1858143003/diff/160001/src/profiler/sampler.cc File src/profiler/sampler.cc (right): https://codereview.chromium.org/1858143003/diff/160001/src/profiler/sampler.c... src/profiler/sampler.cc:276: #if V8_OS_MACOSX The reason I used this macro, is because on MACOSX, pthread_t is a.k.a. _opaque_pthread_t *, which is a pointer.
https://codereview.chromium.org/1858143003/diff/160001/src/profiler/sampler.cc File src/profiler/sampler.cc (right): https://codereview.chromium.org/1858143003/diff/160001/src/profiler/sampler.c... src/profiler/sampler.cc:276: #if V8_OS_MACOSX On 2016/04/08 00:24:39, lpy wrote: > The reason I used this macro, is because on MACOSX, pthread_t is a.k.a. > _opaque_pthread_t *, which is a pointer. Why not use the MacOS version everywhere instead?
https://codereview.chromium.org/1858143003/diff/160001/src/profiler/sampler.cc File src/profiler/sampler.cc (right): https://codereview.chromium.org/1858143003/diff/160001/src/profiler/sampler.c... src/profiler/sampler.cc:276: #if V8_OS_MACOSX On 2016/04/08 16:08:48, fmeawad wrote: > On 2016/04/08 00:24:39, lpy wrote: > > The reason I used this macro, is because on MACOSX, pthread_t is a.k.a. > > _opaque_pthread_t *, which is a pointer. > > Why not use the MacOS version everywhere instead? Similar to here: https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/address-map...
On 2016/04/08 16:10:16, fmeawad wrote: > https://codereview.chromium.org/1858143003/diff/160001/src/profiler/sampler.cc > File src/profiler/sampler.cc (right): > > https://codereview.chromium.org/1858143003/diff/160001/src/profiler/sampler.c... > src/profiler/sampler.cc:276: #if V8_OS_MACOSX > On 2016/04/08 16:08:48, fmeawad wrote: > > On 2016/04/08 00:24:39, lpy wrote: > > > The reason I used this macro, is because on MACOSX, pthread_t is a.k.a. > > > _opaque_pthread_t *, which is a pointer. > > > > Why not use the MacOS version everywhere instead? > > Similar to here: > https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/address-map... It would be a compilation error: 'reinterpret_cast from 'pthread_t' (aka 'unsigned long') to 'intptr_t' (aka 'long') is not allowed'.
lgtm https://codereview.chromium.org/1858143003/diff/160001/src/profiler/sampler.cc File src/profiler/sampler.cc (right): https://codereview.chromium.org/1858143003/diff/160001/src/profiler/sampler.c... src/profiler/sampler.cc:471: if (sampler == NULL || (!sampler->IsProfiling() && !sampler->IsRegistered())) shouldn't it be || instead of && here?
lpy@chromium.org changed reviewers: + jochen@chromium.org
Thanks for the review. +jochen@ for review. https://codereview.chromium.org/1858143003/diff/160001/src/profiler/sampler.cc File src/profiler/sampler.cc (right): https://codereview.chromium.org/1858143003/diff/160001/src/profiler/sampler.c... src/profiler/sampler.cc:471: if (sampler == NULL || (!sampler->IsProfiling() && !sampler->IsRegistered())) On 2016/04/12 21:27:45, alph wrote: > shouldn't it be || instead of && here? If a sample is not profiling and not registered, then we should return. If it's changed to ||, then the logic would be if a sample is not profiling or not registered, then we should return, which is not correct. Let's say when the CpuProfiler is using it, then IsRegistered() will return true and IsProfiling() will return false, in this case we shouldn't return.
jochen@chromium.org changed reviewers: + yangguo@chromium.org
Yang, could you have a look please?
can you please use a lazy initializer instead of adding a static initializer? and can you also delete Isolate::UnsafeCurrent()?
On 2016/04/14 13:58:11, jochen - slow wrote: > can you please use a lazy initializer instead of adding a static initializer? > > and can you also delete Isolate::UnsafeCurrent()? Thanks. I updated the CL. Yang, PTAL.
lgtm https://codereview.chromium.org/1858143003/diff/180001/src/profiler/sampler.cc File src/profiler/sampler.cc (right): https://codereview.chromium.org/1858143003/diff/180001/src/profiler/sampler.c... src/profiler/sampler.cc:471: if (sampler == NULL || (!sampler->IsProfiling() && !sampler->IsRegistered())) please use {} brackets on line break. https://codereview.chromium.org/1858143003/diff/180001/src/profiler/sampler.h File src/profiler/sampler.h (right): https://codereview.chromium.org/1858143003/diff/180001/src/profiler/sampler.h... src/profiler/sampler.h:97: // sampler is in use by the CpuProfiler. Comment is a bit weird. How about "CpuProfiler collects samples by calling DoSample directly without calling Start. To keep it working, we register the sampler with the CpuProfiler.
Thank you for the review. I updated the CL. Anyone else I should ask for review? https://codereview.chromium.org/1858143003/diff/180001/src/profiler/sampler.cc File src/profiler/sampler.cc (right): https://codereview.chromium.org/1858143003/diff/180001/src/profiler/sampler.c... src/profiler/sampler.cc:471: if (sampler == NULL || (!sampler->IsProfiling() && !sampler->IsRegistered())) On 2016/04/15 05:35:53, Yang wrote: > please use {} brackets on line break. Done. https://codereview.chromium.org/1858143003/diff/180001/src/profiler/sampler.h File src/profiler/sampler.h (right): https://codereview.chromium.org/1858143003/diff/180001/src/profiler/sampler.h... src/profiler/sampler.h:97: // sampler is in use by the CpuProfiler. On 2016/04/15 05:35:53, Yang wrote: > Comment is a bit weird. How about > "CpuProfiler collects samples by calling DoSample directly > without calling Start. To keep it working, we register the sampler > with the CpuProfiler. Done.
lgtm
The CQ bit was checked by lpy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alph@chromium.org, yangguo@chromium.org Link to the patchset: https://codereview.chromium.org/1858143003/#ps220001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1858143003/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1858143003/220001
Message was sent while issue was closed.
Description was changed from ========== Get rid of UnsafeCurrent in Sampler Currently we are using UnsafeCurrent in async signal handler to acquire the isolate of VM thread, but we want to get rid of that since it prevents V8 from being thread agnostic. This patch replaces UnsafeCurrent with a static map, where we store a map of samplers for threads, and makes it accessible by signal handler. BUG=v8:4889 LOG=n ========== to ========== Get rid of UnsafeCurrent in Sampler Currently we are using UnsafeCurrent in async signal handler to acquire the isolate of VM thread, but we want to get rid of that since it prevents V8 from being thread agnostic. This patch replaces UnsafeCurrent with a static map, where we store a map of samplers for threads, and makes it accessible by signal handler. BUG=v8:4889 LOG=n ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Get rid of UnsafeCurrent in Sampler Currently we are using UnsafeCurrent in async signal handler to acquire the isolate of VM thread, but we want to get rid of that since it prevents V8 from being thread agnostic. This patch replaces UnsafeCurrent with a static map, where we store a map of samplers for threads, and makes it accessible by signal handler. BUG=v8:4889 LOG=n ========== to ========== Get rid of UnsafeCurrent in Sampler Currently we are using UnsafeCurrent in async signal handler to acquire the isolate of VM thread, but we want to get rid of that since it prevents V8 from being thread agnostic. This patch replaces UnsafeCurrent with a static map, where we store a map of samplers for threads, and makes it accessible by signal handler. BUG=v8:4889 LOG=n Committed: https://crrev.com/62fb4775fea0d56d8a175baf1d902213f6752168 Cr-Commit-Position: refs/heads/master@{#35541} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/62fb4775fea0d56d8a175baf1d902213f6752168 Cr-Commit-Position: refs/heads/master@{#35541}
Message was sent while issue was closed.
A revert of this CL (patchset #12 id:220001) has been created in https://codereview.chromium.org/1897673002/ by machenbach@chromium.org. The reason for reverting is: [Sheriff] Breaks tsan: https://build.chromium.org/p/client.v8/builders/V8%20Linux64%20TSAN/builds/8999. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
