• Peter Marshall's avatar
    Revert "[cpu-profiler] Ensure sampled thread has Isolate lock under Windows" · 32435062
    Peter Marshall authored
    This reverts commit dfb3f7da.
    
    Reason for revert: Breaks LSAN & ASAN flakily: https://bugs.chromium.org/p/v8/issues/detail?id=10861
    
    Original change's description:
    > [cpu-profiler] Ensure sampled thread has Isolate lock under Windows
    > 
    > While the sampler checked if the sampled thread had the Isolate locked
    > (if locks are being used) under Linux, the check was not done under
    > Windows (or Fuchsia) which meant that in a multi-threading application
    > under Windows, thread locking was not checked making it prone to seg
    > faults and the like as the profiler would be extracting info from a
    > heap in motion. The fix was to move the lock check into CpuSampler
    > and Ticker (--prof) so all OSes would do the correct check.
    > 
    > The basic concept is that on all operating systems a CpuProfiler, and
    > so its corresponding CpuCampler, the profiler is tied to a thread.
    > This is not based on first principles or anything, it's simply the
    > way it works in V8, though it is a useful conceit as it makes
    > visualization and interpretation of profile data much easier.
    > 
    > To collect a sample on a thread associated with a profiler the thread
    > must be stopped for obvious reasons -- walking the stack of a running
    > thread is a formula for disaster. The mechanism for stopping a thread
    > is OS-specific and is done in sample.cc. There are currently three
    > basic approaches, one for Linux/Unix variants, one for Windows and one
    > for Fuchsia. The approaches vary as to which thread actually collects
    > the sample -- under Linux the sample is actually collected on the
    > (interrupted) sampled thread whereas under Fuchsia/Windows it's on
    > a separate thread.
    > 
    > However, in a multi-threaded environment (where Locker is used), it's
    > not sufficient for the sampled thread to be stopped. Because the stack
    > walk involves looking in the Isolate heap, no other thread can be
    > messing with the heap while the sample is collected. The only ways to
    > ensure this would be to either stop all threads whenever collecting a
    > sample, or to ensure that the thread being sampled holds the Isolate
    > lock so prevents other threads from messing with the heap. While there
    > might be something to be said for the "stop all threads" approach, the
    > current approach in V8 is to only stop the sampled thread so, if in a
    > multi-threaded environment, the profiler must check if the thread being
    > sampled holds the Isolate lock.
    > 
    > Since this check must be done, independent of which thread the sample
    > is being collected on (since it varies from OS to OS), the approach is
    > to save the thread id of the thread to be profiled/sampled when the
    > CpuSampler is instantiated (on all OSes it is instantiated on the
    > sampled thread) and then check that thread id against the Isolate lock
    > holder thread id before collecting a sample. If it matches, we know
    > sample.cc has stop the sampled thread, one way or another, and we know
    > that no other thread can mess with the heap (since the stopped thread
    > holds the Isolate lock) so it's safe to walk the stack and collect data
    > from the heap so the sample can be taken. It it doesn't match, we can't
    > safely collect the sample so we don't.
    > 
    > Bug: v8:10850
    > Change-Id: Iab2493130b9328430d7e5f5d3cf90ad6d10b1892
    > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2377108
    > Reviewed-by: Peter Marshall <petermarshall@chromium.org>
    > Commit-Queue: Peter Marshall <petermarshall@chromium.org>
    > Cr-Commit-Position: refs/heads/master@{#69623}
    
    TBR=akodat@rocketsoftware.com,petermarshall@chromium.org,petermarshall@google.com
    
    Change-Id: Ib6b6dc4ce109d5aa4e504fa7c9769f5cd95ddd0c
    No-Presubmit: true
    No-Tree-Checks: true
    No-Try: true
    Bug: v8:10850
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2387570Reviewed-by: 's avatarPeter Marshall <petermarshall@chromium.org>
    Commit-Queue: Peter Marshall <petermarshall@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#69638}
    32435062
cctest.h 26.5 KB