tracing: Fix some race conditions#22812
Closed
Uh oh!
There was an error while loading. Please reload this page.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Original commit message: [tracing] do not add traces when disabled nodejs#21038 Change-Id: Ic4c9f403b5e54a97d3170b2311dd5aab8c8357c8 Reviewed-on: https://chromium-review.googlesource.com/1217726 Commit-Queue: Ali Ijaz Sheikh <[email protected]> Reviewed-by: Yang Guo <[email protected]> Cr-Commit-Position: refs/heads/master@{nodejs#55809} Refs: v8/v8@2363cdf
nodejs-github-bot added build 
refack approved these changes Sep 12, 2018
refack reviewed Sep 12, 2018
| auto& chunk = chunks_[i]; | ||
| for (size_t j = 0; j < chunk->size(); ++j){ | ||
| agent_->AppendTraceEvent(chunk->GetEventAt(j)); | ||
| TraceObject* trace_event = chunk->GetEventAt(j); |
Member
There was a problem hiding this comment.
We tend to avoid auto, except for cases where the type is hard to spell out (e.g. STL iterator types/dependent on templating), or long and easy to infer (e.g. auto foo = new VeryLongClassName();). Using auto means spending more time writing the code, but more time reading it, and Node.js code is typically read a lot more than it is written.
Contributor
There was a problem hiding this comment.
IMHO it's time for that to change (ES.11).
Beside using the CCG to not bikeshed, for me as a reader TraceObject* is as opaque as auto, that why we have IDEs.
jasnell approved these changes Sep 12, 2018
Member
jasnell left a comment
jasnell left a comment There was a problem hiding this comment.
I think this actually fixes a problem I've been seeing in some of the other dev work I've been doing. LGTM!
addaleax approved these changes Sep 12, 2018
eugeneo approved these changes Sep 12, 2018
ContributorAuthor
ofrobots commented Sep 12, 2018
Upstream change got flagged by UBSAN. I need to investigate those before this lands. |
ContributorAuthor
ofrobots commented Sep 13, 2018
UBSAN failures were unrelated to my change 🎉. |
ContributorAuthor
ofrobots commented Sep 13, 2018
CI looks green. I will land this later today. |
ContributorAuthor
Landed as 5cccc55...90c9368. |
ofrobots added a commit that referenced this pull request Sep 14, 2018
Original commit message: [tracing] do not add traces when disabled #21038 Change-Id: Ic4c9f403b5e54a97d3170b2311dd5aab8c8357c8 Reviewed-on: https://chromium-review.googlesource.com/1217726 Commit-Queue: Ali Ijaz Sheikh <[email protected]> Reviewed-by: Yang Guo <[email protected]> Cr-Commit-Position: refs/heads/master@{#55809} Refs: v8/v8@2363cdf PR-URL: #22812 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]>
ofrobots added a commit that referenced this pull request Sep 14, 2018
PR-URL: #22812 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]>
2 tasks
targos pushed a commit that referenced this pull request Sep 14, 2018
Original commit message: [tracing] do not add traces when disabled #21038 Change-Id: Ic4c9f403b5e54a97d3170b2311dd5aab8c8357c8 Reviewed-on: https://chromium-review.googlesource.com/1217726 Commit-Queue: Ali Ijaz Sheikh <[email protected]> Reviewed-by: Yang Guo <[email protected]> Cr-Commit-Position: refs/heads/master@{#55809} Refs: v8/v8@2363cdf PR-URL: #22812 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]>
targos pushed a commit that referenced this pull request Sep 14, 2018
PR-URL: #22812 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]>
targos pushed a commit that referenced this pull request Sep 19, 2018
Original commit message: [tracing] do not add traces when disabled #21038 Change-Id: Ic4c9f403b5e54a97d3170b2311dd5aab8c8357c8 Reviewed-on: https://chromium-review.googlesource.com/1217726 Commit-Queue: Ali Ijaz Sheikh <[email protected]> Reviewed-by: Yang Guo <[email protected]> Cr-Commit-Position: refs/heads/master@{#55809} Refs: v8/v8@2363cdf PR-URL: #22812 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]>
targos pushed a commit that referenced this pull request Sep 19, 2018
PR-URL: #22812 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]>
targos pushed a commit that referenced this pull request Sep 20, 2018
Original commit message: [tracing] do not add traces when disabled #21038 Change-Id: Ic4c9f403b5e54a97d3170b2311dd5aab8c8357c8 Reviewed-on: https://chromium-review.googlesource.com/1217726 Commit-Queue: Ali Ijaz Sheikh <[email protected]> Reviewed-by: Yang Guo <[email protected]> Cr-Commit-Position: refs/heads/master@{#55809} Refs: v8/v8@2363cdf PR-URL: #22812 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]>
targos pushed a commit that referenced this pull request Sep 20, 2018
PR-URL: #22812 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]>
Trott added a commit to Trott/io.js that referenced this pull request Oct 6, 2018
Unreliability for test-trace-events-fs-sync is believed to have been fixed. Remove flaky designation. Ref: nodejs#22812 Ref: nodejs#21038 (comment)
Trott added a commit to Trott/io.js that referenced this pull request Oct 7, 2018
Unreliability for test-trace-events-fs-sync is believed to have been fixed. Remove flaky designation. Ref: nodejs#22812 Ref: nodejs#21038 (comment) PR-URL: nodejs#22856 Refs: nodejs#22812 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Weijia Wang <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
targos pushed a commit that referenced this pull request Oct 7, 2018
Unreliability for test-trace-events-fs-sync is believed to have been fixed. Remove flaky designation. Ref: #22812 Ref: #21038 (comment) PR-URL: #22856 Refs: #22812 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Weijia Wang <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
targos pushed a commit that referenced this pull request Oct 7, 2018
Unreliability for test-trace-events-fs-sync is believed to have been fixed. Remove flaky designation. Ref: #22812 Ref: #21038 (comment) PR-URL: #22856 Refs: #22812 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Weijia Wang <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
jasnell pushed a commit that referenced this pull request Oct 17, 2018
Unreliability for test-trace-events-fs-sync is believed to have been fixed. Remove flaky designation. Ref: #22812 Ref: #21038 (comment) PR-URL: #22856 Refs: #22812 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Weijia Wang <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR contains two commits that fix some race conditions that were causing flakiness on #21038. There may yet be more races, but this is what I got right now.
One of the commits is an upstream V8 fix. I didn't bother requesting an upstream back-port as it is not particular useful for Chrome and better to float here.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes