Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Incorrect write barrier elimination #40780

Closed
rmacnak-google opened this issue Feb 26, 2020 · 0 comments
Closed

Incorrect write barrier elimination #40780

rmacnak-google opened this issue Feb 26, 2020 · 0 comments
Assignees
@sjindel-google
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends.

Comments

@rmacnak-google
Copy link
Contributor

Using verification code added in https://dart-review.googlesource.com/c/sdk/+/137240, I see that https://dart-review.googlesource.com/c/sdk/+/136221 causes incorrect write barrier elimination in Splay.

$ ./out/ReleaseX64/dart --verify-store-buffer ~/benchmarks/Splay/dart/Splay.dart
./../runtime/vm/heap/scavenger.cc: 512: error: Old object 0x7feaf6403091 references new object 0x7feaf8c79cb9, but it is not in any store buffer. Consider using rr to watch the slot 0x7feaf64030b0 to reverse-continue to find the store with a missing barrier.

rr reveals that last write to 0x7feaf64030b0 was
StoreInstanceField(v213 . right = v245, NoStoreBarrier)

StoreInstanceField(v213 . right = v245, NoStoreBarrier)
v209 <- AllocateObject(Node) T{Node}

v213 <- phi(v209, v255) alive T{Node}
v217 <- phi(v316 T{Node}, v259) alive T{Node?}
v245 <- phi(v237 T{Node}, v217 T{Node}) alive T{Node}
v255 <- phi(v213, v245) alive T{Node}
v255 <- phi(v213, v245) alive T{Node}
v259 <- phi(v271, v247) alive T{Node?}

v237 <- LoadField(v217 T{Node} . right) T{Node?}
v247 <- LoadField(v245 . right) T{Node?}
v271 <- LoadField(v269 . left) T{Node?}
v316 <- LoadField(v8 T{SplayTree} . root) T{Node?}

v213 can come from a field load, so it is not safe to remove the barrier.
@rmacnak-google rmacnak-google added the area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. label Feb 26, 2020
dart-bot pushed a commit that referenced this issue Feb 26, 2020
@rmacnak-google
Revert "[vm] Re-land aggressive write-barrier elimination."
7fd8ad5 
This reverts commit c7d7552.

Reason for revert: #40780

Original change's description:
> [vm] Re-land aggressive write-barrier elimination.
> 
> It incorrectly assumed that all stores in Dart code write to Instances.
> There is actually one exception, Contexts, which do not inherit from Instance.
> 
> I've added asserts to ensure this kind of bug cannot resurface.
> 
> The original change is in patchset 4.
> 
> Change-Id: Ic2d8d05e70a4de738eb9fb5980487b4f27111b8c
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/136221
> Commit-Queue: Samir Jindel <sjindel@google.com>
> Reviewed-by: Martin Kustermann <kustermann@google.com>
> Reviewed-by: Ryan Macnak <rmacnak@google.com>

TBR=kustermann@google.com,rmacnak@google.com,sjindel@google.com

# Not skipping CQ checks because original CL landed > 1 day ago.

Change-Id: I1891677b21560c7fc5a54a8eb800ef5850654402
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/137290
Reviewed-by: Ryan Macnak <rmacnak@google.com>
Commit-Queue: Ryan Macnak <rmacnak@google.com>
dart-bot pushed a commit that referenced this issue Feb 27, 2020
@rmacnak-google
[vm, gc] Verify the store buffer contains all old-space objects that …
2a5d058 
…reference new-space objects.

Speed-up ObjectSets.

Fix pointer verification before and after scavenging to set the correct mark expectation being on concurrent marking state.

Bug: #40780
Change-Id: Ic7ce6d5a829e41634083b0c274b9764da3f4b619
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/137240
Commit-Queue: Ryan Macnak <rmacnak@google.com>
Reviewed-by: Alexander Markov <alexmarkov@google.com>
Reviewed-by: Siva Annamalai <asiva@google.com>
dart-bot pushed a commit that referenced this issue Feb 27, 2020
@rmacnak-google
[vm, gc] Add Splay as a test of various GC modes and verification tools.
275bc63 
Bug: #40780
Change-Id: I2b760c4e2884f400e1dcfbd77f21fa9c4d96f25c
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/137645
Reviewed-by: Alexander Markov <alexmarkov@google.com>
Commit-Queue: Ryan Macnak <rmacnak@google.com>
dart-bot pushed a commit that referenced this issue Mar 10, 2020
@mkustermann
Revert "Re-land "[vm] Aggressive write-barrier elimination.""
30a12a3 
This reverts commit eff1a9f.

Reason for revert:
  Causes flaky hits of RELEASE_ASSERT in marker.cc, see b/151131634.

Original change's description:
> Re-land "[vm] Aggressive write-barrier elimination."
> 
> The original revision is in Patchset 3.
> 
> Four bugs were fixed:
> 
> 1. JoinEntryInstr::SuccessorCount() is not the correct way to get the
>    number of successor blocks from the Join block;
>    JoinEntryInstr::last_instruction()->SuccessorCount() must be used
>    instead.
> 
> 2. BitVector::Equals() was non-deterministically returning 'false'
>    for equal vectors.
> 
> 3. All blocks need to be processed at least once during the Analysis
>    phase (not only in the SaveResults phase).
> 
> 4. We were not removing write barriers from StoreIndexed instructions,
>    even though we had support for it.
> 
> This reverts commit 7fd8ad5.
> 
> Fixes #40780
> 
> Change-Id: I9650ec2c547ec49cf88ca0524e14f6c245621f6a
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/138086
> Commit-Queue: Samir Jindel <sjindel@google.com>
> Reviewed-by: Martin Kustermann <kustermann@google.com>
> Reviewed-by: Ryan Macnak <rmacnak@google.com>

TBR=kustermann@google.com,rmacnak@google.com,sjindel@google.com

Change-Id: If9afd84465175fad2431405a97e9293c8bd5e476
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/138808
Reviewed-by: Martin Kustermann <kustermann@google.com>
Commit-Queue: Martin Kustermann <kustermann@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@sjindel-google sjindel-google

Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@rmacnak-google @sjindel-google