Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1900473002:
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/ff7e6defffe4cd2feb1336a333b5220f0d3cdcf6
Cr-Commit-Position: refs/heads/master@{#35722}
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : #Patch Set 4 : #
Total comments: 3
Patch Set 5 : #
Total comments: 3
Patch Set 6 : #
Messages
Total messages: 37 (13 generated)
lpy@chromium.org changed reviewers: + alph@chromium.org, fmeawad@chromium.org, jochen@chromium.org, yangguo@chromium.org
alph, fmeawad, jochen and yang. PTAL. https://codereview.chromium.org/1900473002/diff/60001/src/profiler/sampler.cc File src/profiler/sampler.cc (right): https://codereview.chromium.org/1900473002/diff/60001/src/profiler/sampler.cc... src/profiler/sampler.cc:250: USE(atomic_->Value()); This is the only place I changed since the revert. The reason I added this line is to make sure the mutual exclusion is recognized by TSAN, and since AtomicValue::Value() is using Acquire_Load, I think we can achieve the goal by just calling it.
https://codereview.chromium.org/1900473002/diff/60001/src/profiler/sampler.cc File src/profiler/sampler.cc (right): https://codereview.chromium.org/1900473002/diff/60001/src/profiler/sampler.cc... src/profiler/sampler.cc:249: // Use Acquire_Load to make TSAN happy. Instead of saying make TSAN happy, can you explain in the comment what goes on.
I updated the comment. PTAL. https://codereview.chromium.org/1900473002/diff/60001/src/profiler/sampler.cc File src/profiler/sampler.cc (right): https://codereview.chromium.org/1900473002/diff/60001/src/profiler/sampler.cc... src/profiler/sampler.cc:249: // Use Acquire_Load to make TSAN happy. On 2016/04/20 23:17:12, fmeawad wrote: > Instead of saying make TSAN happy, can you explain in the comment what goes on. Done.
lgtm with updated comment https://codereview.chromium.org/1900473002/diff/80001/src/profiler/sampler.cc File src/profiler/sampler.cc (right): https://codereview.chromium.org/1900473002/diff/80001/src/profiler/sampler.cc... src/profiler/sampler.cc:250: // TSAN recognizes the atomic variable acting as guard. i think the point is that it doesn't act as a guard unless you do this.
https://codereview.chromium.org/1900473002/diff/80001/src/profiler/sampler.cc File src/profiler/sampler.cc (right): https://codereview.chromium.org/1900473002/diff/80001/src/profiler/sampler.cc... src/profiler/sampler.cc:250: // TSAN recognizes the atomic variable acting as guard. On 2016/04/21 14:44:24, jochen wrote: > i think the point is that it doesn't act as a guard unless you do this. Why? To me it looks more like TSAN bug. Isn't TrySetValue imply use because it has to read the value to make the comparison.
mlippautz@chromium.org changed reviewers: + mlippautz@chromium.org
https://codereview.chromium.org/1900473002/diff/80001/src/profiler/sampler.cc File src/profiler/sampler.cc (right): https://codereview.chromium.org/1900473002/diff/80001/src/profiler/sampler.cc... src/profiler/sampler.cc:250: // TSAN recognizes the atomic variable acting as guard. On 2016/04/21 16:21:28, alph wrote: > On 2016/04/21 14:44:24, jochen wrote: > > i think the point is that it doesn't act as a guard unless you do this. > > Why? To me it looks more like TSAN bug. Isn't TrySetValue imply use because it > has to read the value to make the comparison. dbc (basically the comment from the other CL): TrySetValue currently *only* implies a Release barrier (not acquire). This is far from ideal. To support stand-alone use cases (like the one above without the load) and load/cas scenarios (like it's modeled now) we should have a full barrier in the CAS.
On 2016/04/21 16:34:18, Michael Lippautz wrote: > https://codereview.chromium.org/1900473002/diff/80001/src/profiler/sampler.cc > File src/profiler/sampler.cc (right): > > https://codereview.chromium.org/1900473002/diff/80001/src/profiler/sampler.cc... > src/profiler/sampler.cc:250: // TSAN recognizes the atomic variable acting as > guard. > On 2016/04/21 16:21:28, alph wrote: > > On 2016/04/21 14:44:24, jochen wrote: > > > i think the point is that it doesn't act as a guard unless you do this. > > > > Why? To me it looks more like TSAN bug. Isn't TrySetValue imply use because it > > has to read the value to make the comparison. > > dbc (basically the comment from the other CL): > TrySetValue currently *only* implies a Release barrier (not acquire). This is > far from ideal. To support stand-alone use cases (like the one above without the > load) and load/cas scenarios (like it's modeled now) we should have a full > barrier in the CAS. The reason why this CL failed to pass TSAN was because TrySetValue is using the wrong memory ordering that doesn't provide mutual exclusion. we can either change it to use Acquire, or use a full barrier like I did in the CL. And the reason Michael suggested us using a full barrier is that the purpose of the utils is to have a set of operations where you don't need to worry about ordering semantics.
lgtm
lgtm
The CQ bit was checked by lpy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org Link to the patchset: https://codereview.chromium.org/1900473002/#ps100001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1900473002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1900473002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_linux_nodcheck_rel_ng on tryserver.v8 (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
The CQ bit was checked by fmeawad@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1900473002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1900473002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_linux_arm64_rel_ng on tryserver.v8 (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...) v8_mac_rel_ng on tryserver.v8 (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/563)
The CQ bit was checked by lpy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1900473002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1900473002/100001
The CQ bit was unchecked by machenbach@chromium.org
The CQ bit was checked by machenbach@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1900473002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1900473002/100001
On 2016/04/22 06:50:24, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/1900473002/100001 > View timeline at > https://chromium-cq-status.appspot.com/patch-timeline/1900473002/100001 Looks like it timed out but build passed, any idea? Thanks.
Filed http://crbug.com/605846 Might take a few hours until folks are around for fixing this. Is it urgent to land?
On 2016/04/22 06:54:50, Michael Achenbach wrote: > Filed http://crbug.com/605846 > > Might take a few hours until folks are around for fixing this. Is it urgent to > land? No rush. Thanks.
Please upload a new (empty) patchset and queue that one (see http://crbug.com/605846). Sorry for the inconvenience.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_mac_rel_ng on tryserver.v8 (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/570)
The CQ bit was checked by machenbach@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1900473002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1900473002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
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/ff7e6defffe4cd2feb1336a333b5220f0d3cdcf6 Cr-Commit-Position: refs/heads/master@{#35722} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/ff7e6defffe4cd2feb1336a333b5220f0d3cdcf6 Cr-Commit-Position: refs/heads/master@{#35722} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
