Fix object layout races #285

daumayr · 2019-01-14T14:24:37Z

This PR fixes issue #85.

The Phaser used in ObjectTransitionSafepoint used a default Phaser, which Terminates when onAdvance(...) returns true. This caused the synchronisation methods to have no effect.
The default onAdvance implementation causes the Phaser to terminate when the number of registered Threads is 0.

By overwriting the onAdvance method and returning false we can prevent Phaser from terminating and preserve the synchronisation.

src/som/interpreter/objectstorage/ObjectTransitionSafepoint.java

src/som/vmobjects/SObject.java

daumayr · 2019-01-15T11:01:42Z

While tests are running for this branch, we still have an issue with some benchmarks. The Vacation Benchmark still deadlocks, so ideally we don't merge this PR until we fix the Join Prim and similar things.

smarr · 2019-01-15T13:01:22Z

Ok, will see what I can do about those. Will probably try to do them in a separate PR to see performance issues independently.

daumayr · 2019-01-15T13:10:41Z

I think we need to exclude those benchmarks (LeeTm and Vacation) until then.

Phaser was able to terminate, an thread registration didn't prevent threads from making progress.

smarr

A quick review to give at least some comments.
Sorry, didn't get to more.

src/som/interpreter/nodes/dispatch/ClassSlotAccessNode.java

src/som/interpreter/objectstorage/ObjectTransitionSafepoint.java

…omPhaser is http://gee.cs.oswego.edu/cgi-bin/viewcvs.cgi/jsr166/src/jdk8/java/util/concurrent/Phaser.java?revision=1.3 This is a simple copy, no modifications yet. Signed-off-by: Stefan Marr <[email protected]>

This change introduces our formatting and names the class SafepointPhaser and sets the package name correctly. No changes of functionality. Signed-off-by: Stefan Marr <[email protected]>

Signed-off-by: Stefan Marr <[email protected]>

src/som/interpreter/objectstorage/ObjectTransitionSafepoint.java

Co-authored-by: Dominik Aumayr <[email protected]> Signed-off-by: Stefan Marr <[email protected]>

The SafepointPhaser should not terminate, even if no parties are registered on it anymore. This can happen with our safepoint design, but it is not safe to terminate the phaser. It needs to continue working and accept threads to join again. Co-authored-by: Dominik Aumayr <[email protected]> Signed-off-by: Stefan Marr <[email protected]>

Signed-off-by: Stefan Marr <[email protected]>

Remove interruptible, timed, and deadline notions. Signed-off-by: Stefan Marr <[email protected]>

SafepointPhaser isn’t a general purpose class, so, let’s provide features only when needed. Signed-off-by: Stefan Marr <[email protected]>

Co-authored-by: Dominik Aumayr <[email protected]> Signed-off-by: Stefan Marr <[email protected]>

This change uses the knowledge of the safepoint consisting of two phases. This allows us to manage registrations without extra race, and it allows us to renew the assumption as part of the arrival routine in the phaser. Signed-off-by: Stefan Marr <[email protected]>

Signed-off-by: Stefan Marr <[email protected]>

smarr · 2019-02-02T14:31:31Z

@daumayr I come up with what I think is a working solution. Could you please review and possible test this?
I haven't yet thought of a good test on how to stress the safepoints.
Not sure I can induce repeated safepoints easily.
Would be good to discuss this in detail, to see whether there are any know races left, or any other issue that may make this approach problematic.

The most important bit to discuss is likely 0274546

daumayr · 2019-02-04T08:30:31Z

(phase & 1) == 0
I was thinking of using something similar when i did some work on the safepoints. When no safepoint is going on and all threads deregister the phaser advances to the next phase. After that the even/odd check would no longer work. You might want to change the doArrive method to not advance phase when there are no threads left.

smarr · 2019-02-04T09:46:54Z

@daumayr hmmmm, ok. Didn't think of that. Looking at the code: https://github.com/smarr/SOMns/pull/285/files#diff-2b3be932b680eb719ab91a02f3b6f064R137

I suppose, I could simply increment the phase in that case by 2, no?

smarr · 2019-02-04T09:48:26Z

hmmm, well, possibly depending on in which concrete state we are. Though, we should never unregister during a safepoint. So, probably increment by 2 plus an assertion that we are never in a phase that indicates a safepoint is going on

daumayr · 2019-02-04T10:28:55Z

Increment by two sounds good, that way we still have a phase change and keep the odd/even.

smarr · 2019-02-04T10:32:16Z

ok, cool.

and one more todo:

add unit tests that test the basic logic for correctness, and have asserts for all the parts not covered

… reregistered Signed-off-by: Stefan Marr <[email protected]>

Signed-off-by: Stefan Marr <[email protected]>

smarr · 2019-02-04T20:25:50Z

@daumayr could you give this a spin with your snapshot tests?
Would also be great if you could review it and look at the added test. Any obvious test we should add?

daumayr · 2019-02-05T11:45:49Z

Snapshot Tests running, didn't see any errors.

tests/java/som/interpreter/objectstorage/SafepointPhaserTest.java

daumayr force-pushed the FixObjectLayoutRaces branch from c84b334 to 5f611db Compare January 14, 2019 15:57

smarr reviewed Jan 14, 2019

View reviewed changes

src/som/interpreter/objectstorage/ObjectTransitionSafepoint.java Outdated Show resolved Hide resolved

src/som/vmobjects/SObject.java Show resolved Hide resolved

smarr mentioned this pull request Jan 15, 2019

All Blocking Primitives need to interact with Safepoint Logic #286

Closed

Repository owner deleted a comment Jan 16, 2019

daumayr added 4 commits January 16, 2019 11:54

Fix ObjectTransitionSafepoint

9aa73e1

Phaser was able to terminate, an thread registration didn't prevent threads from making progress.

Fix ClassSlot initialisation Deadlock

1e7e1d0

Replace safepoint assumption at end

595a46e

Fix registration loophole

7c562ab

daumayr force-pushed the FixObjectLayoutRaces branch from f4f6d42 to 1ad49a2 Compare January 16, 2019 10:55

smarr reviewed Jan 24, 2019

View reviewed changes

src/som/interpreter/nodes/dispatch/ClassSlotAccessNode.java Outdated Show resolved Hide resolved

src/som/interpreter/objectstorage/ObjectTransitionSafepoint.java Outdated Show resolved Hide resolved

smarr added 3 commits January 31, 2019 23:31

Added SafepointPhaser based on JRE-166 Java 8. The source of the Cust…

06b8a4c

…omPhaser is http://gee.cs.oswego.edu/cgi-bin/viewcvs.cgi/jsr166/src/jdk8/java/util/concurrent/Phaser.java?revision=1.3 This is a simple copy, no modifications yet. Signed-off-by: Stefan Marr <[email protected]>

Adapt to be SafepointPhaser

e307fd4

This change introduces our formatting and names the class SafepointPhaser and sets the package name correctly. No changes of functionality. Signed-off-by: Stefan Marr <[email protected]>

Fix checkstyle issues in SafepointPhaser

0ac798b

Signed-off-by: Stefan Marr <[email protected]>

smarr force-pushed the FixObjectLayoutRaces branch from 1ad49a2 to 439313e Compare February 1, 2019 00:12

Repository owner deleted a comment Feb 1, 2019

smarr reviewed Feb 1, 2019

View reviewed changes

src/som/interpreter/objectstorage/ObjectTransitionSafepoint.java Outdated Show resolved Hide resolved

smarr and others added 5 commits February 2, 2019 13:57

Use SafepointPhaser

8028edf

Co-authored-by: Dominik Aumayr <[email protected]> Signed-off-by: Stefan Marr <[email protected]>

Remove further unused elements from SafepointPhaser

f60331f

Signed-off-by: Stefan Marr <[email protected]>

Simplify SafepointPhaser QNode to needed features

d31657d

Remove interruptible, timed, and deadline notions. Signed-off-by: Stefan Marr <[email protected]>

Make SafepointPhaser final and restrict visibility

0c6ad0c

SafepointPhaser isn’t a general purpose class, so, let’s provide features only when needed. Signed-off-by: Stefan Marr <[email protected]>

daumayr and others added 4 commits February 2, 2019 13:58

Add Missing Unsafe Method

f12176e

Use available Unsafe object from TraceBuffer

702a5e9

Co-authored-by: Dominik Aumayr <[email protected]> Signed-off-by: Stefan Marr <[email protected]>

Reduce SafepointPhaser documentation to the accurate bits

2c0f9ba

Signed-off-by: Stefan Marr <[email protected]>

smarr force-pushed the FixObjectLayoutRaces branch from 13c4a97 to 2c0f9ba Compare February 2, 2019 14:27

smarr added this to the v0.7.0 milestone Feb 2, 2019

smarr added the bug Fixes an issue, incorrect implementation label Feb 2, 2019

smarr assigned smarr and daumayr Feb 2, 2019

Repository owner deleted a comment Feb 2, 2019

SafepointPhase: ensure odd/even for phases also after all threads are…

303a74b

… reregistered Signed-off-by: Stefan Marr <[email protected]>

smarr force-pushed the FixObjectLayoutRaces branch from 3b34006 to 743481b Compare February 4, 2019 20:01

Added basic SafepointPhaser unit test

56e54f9

Signed-off-by: Stefan Marr <[email protected]>

smarr force-pushed the FixObjectLayoutRaces branch from 743481b to 56e54f9 Compare February 4, 2019 20:10

Repository owner deleted a comment Feb 4, 2019

daumayr commented Feb 5, 2019

View reviewed changes

tests/java/som/interpreter/objectstorage/SafepointPhaserTest.java Show resolved Hide resolved

smarr merged commit 29a2c9f into smarr:dev Feb 5, 2019

daumayr deleted the FixObjectLayoutRaces branch February 5, 2019 15:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix object layout races #285

Fix object layout races #285

daumayr commented Jan 14, 2019

daumayr commented Jan 15, 2019

smarr commented Jan 15, 2019

daumayr commented Jan 15, 2019 •

edited

Loading

smarr left a comment

smarr commented Feb 2, 2019

daumayr commented Feb 4, 2019

smarr commented Feb 4, 2019

smarr commented Feb 4, 2019

daumayr commented Feb 4, 2019

smarr commented Feb 4, 2019 •

edited

Loading

smarr commented Feb 4, 2019

daumayr commented Feb 5, 2019

Fix object layout races #285

Fix object layout races #285

Conversation

daumayr commented Jan 14, 2019

daumayr commented Jan 15, 2019

smarr commented Jan 15, 2019

daumayr commented Jan 15, 2019 • edited Loading

smarr left a comment

Choose a reason for hiding this comment

smarr commented Feb 2, 2019

daumayr commented Feb 4, 2019

smarr commented Feb 4, 2019

smarr commented Feb 4, 2019

daumayr commented Feb 4, 2019

smarr commented Feb 4, 2019 • edited Loading

smarr commented Feb 4, 2019

daumayr commented Feb 5, 2019

daumayr commented Jan 15, 2019 •

edited

Loading

smarr commented Feb 4, 2019 •

edited

Loading