Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1735733002:
Fix iterator (std::vector) invalidation during sampling heap profile retrieval (Closed)
|
Created:
4 years, 10 months ago by mattloring Modified:
4 years, 10 months ago CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionFix iterator (std::vector) invalidation during sampling heap profile retrieval
It is possible for JS objects to be allocated while we are retrieving the
profile. These JS objects can in turn end up getting sampled by the profiler.
Adding these to the profile data structures invalidates the iterators that
are presently in flight. This change prevents such concurrent modifications
from affecting the retrieve operation.
BUG=
Committed: https://crrev.com/7bc1577a0bb33a1e8a4c3b4b099d923a24f528b0
Cr-Commit-Position: refs/heads/master@{#34298}
Patch Set 1 #Patch Set 2 : Remove unncessary preallocation of nodes (deque) #Patch Set 3 : Add Reviewers #
Total comments: 1
Patch Set 4 : Clarifying comment #Patch Set 5 : Fix windows build (signed/unsigned compare) #
Messages
Total messages: 26 (15 generated)
Description was changed from ========== Block self samples in the sampling heap profiler The sampling heap profiler allocates JS objects when an allocation profile is retrieved. These allocations mutate the underlying structure as it is being copied. This change prevents such modifications from leaving the allocation profile in an inconsistent state. Also, the vector containing all the nodes in the profile is now preallocated to ensure that it is not resized (and thus moved) allowing us to hold references to its elements. BUG= ========== to ========== Block self samples in the sampling heap profiler The sampling heap profiler allocates JS objects when an allocation profile is retrieved. These allocations mutate the underlying structure as it is being copied. This change prevents such modifications from leaving the allocation profile in an inconsistent state. BUG= ==========
mattloring@google.com changed reviewers: + ofrobots@google.com
mattloring@google.com changed reviewers: + hpayer@chromium.org, ulan@chromium.org
Description was changed from ========== Block self samples in the sampling heap profiler The sampling heap profiler allocates JS objects when an allocation profile is retrieved. These allocations mutate the underlying structure as it is being copied. This change prevents such modifications from leaving the allocation profile in an inconsistent state. BUG= ========== to ========== Fix iterator (std::deque) invalidation during sampling heap profile retrieval It is possible for JS objects to be allocated while we are retrieving the profile. These JS objects can in turn end up getting sampled by the profiler. Adding these to the profile data structures invalidates the iterators that are presently in flight. This change prevents such concurrent modifications from affecting the retrieve operation. BUG= ==========
ofrobots@google.com changed reviewers: + machenbach@chromium.org - hpayer@chromium.org
On 2016/02/24 22:45:27, ofrobots wrote: > mailto:ofrobots@google.com changed reviewers: > + mailto:machenbach@chromium.org > - mailto:hpayer@chromium.org eek.. concurrent modification of reviewers.
Description was changed from ========== Fix iterator (std::deque) invalidation during sampling heap profile retrieval It is possible for JS objects to be allocated while we are retrieving the profile. These JS objects can in turn end up getting sampled by the profiler. Adding these to the profile data structures invalidates the iterators that are presently in flight. This change prevents such concurrent modifications from affecting the retrieve operation. BUG= ========== to ========== Fix iterator (std::vector) invalidation during sampling heap profile retrieval It is possible for JS objects to be allocated while we are retrieving the profile. These JS objects can in turn end up getting sampled by the profiler. Adding these to the profile data structures invalidates the iterators that are presently in flight. This change prevents such concurrent modifications from affecting the retrieve operation. BUG= ==========
ofrobots@google.com changed reviewers: + alph@chromium.org
On 2016/02/24 22:50:52, ofrobots wrote: > mailto:ofrobots@google.com changed reviewers: > + mailto:alph@chromium.org lgtm.
lgtm https://codereview.chromium.org/1735733002/diff/40001/src/profiler/sampling-h... File src/profiler/sampling-heap-profiler.cc (right): https://codereview.chromium.org/1735733002/diff/40001/src/profiler/sampling-h... src/profiler/sampling-heap-profiler.cc:227: for (int i = 0; i < child_len; i++) { I'd put a comment here about concurrent modification.
lgtm, good catch
The CQ bit was checked by mattloring@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1735733002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1735733002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/3342)
The CQ bit was checked by mattloring@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1735733002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1735733002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mattloring@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from ofrobots@google.com, alph@chromium.org, ulan@chromium.org Link to the patchset: https://codereview.chromium.org/1735733002/#ps80001 (title: "Fix windows build (signed/unsigned compare)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1735733002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1735733002/80001
Message was sent while issue was closed.
Description was changed from ========== Fix iterator (std::vector) invalidation during sampling heap profile retrieval It is possible for JS objects to be allocated while we are retrieving the profile. These JS objects can in turn end up getting sampled by the profiler. Adding these to the profile data structures invalidates the iterators that are presently in flight. This change prevents such concurrent modifications from affecting the retrieve operation. BUG= ========== to ========== Fix iterator (std::vector) invalidation during sampling heap profile retrieval It is possible for JS objects to be allocated while we are retrieving the profile. These JS objects can in turn end up getting sampled by the profiler. Adding these to the profile data structures invalidates the iterators that are presently in flight. This change prevents such concurrent modifications from affecting the retrieve operation. BUG= ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Fix iterator (std::vector) invalidation during sampling heap profile retrieval It is possible for JS objects to be allocated while we are retrieving the profile. These JS objects can in turn end up getting sampled by the profiler. Adding these to the profile data structures invalidates the iterators that are presently in flight. This change prevents such concurrent modifications from affecting the retrieve operation. BUG= ========== to ========== Fix iterator (std::vector) invalidation during sampling heap profile retrieval It is possible for JS objects to be allocated while we are retrieving the profile. These JS objects can in turn end up getting sampled by the profiler. Adding these to the profile data structures invalidates the iterators that are presently in flight. This change prevents such concurrent modifications from affecting the retrieve operation. BUG= Committed: https://crrev.com/7bc1577a0bb33a1e8a4c3b4b099d923a24f528b0 Cr-Commit-Position: refs/heads/master@{#34298} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/7bc1577a0bb33a1e8a4c3b4b099d923a24f528b0 Cr-Commit-Position: refs/heads/master@{#34298} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
