Skip to content
New issue

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

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

Already on GitHub? # to your account

Reduce race-related stack overflows, and handled invalid layouts in ClassSlotAccessNode #244

Merged
merged 3 commits into from
Mar 25, 2018

Conversation

smarr
Copy link
Owner

@smarr smarr commented Mar 24, 2018

If multiple threads mutate objects from the same class, it can happen that the object layout gets invalidated by another thread while we try to access it.

In the worst case, this can lead to stack overflows, because our interpreter tries repeatedly to find a valid object layout while creating dispatch chains, or interacting with the object layout in other nodes, which use recursive fallbacks.

A bad case I saw during debugging was that class loading occurred while initializing a new object layout. This delayed the availability of a valid layout long enough to see stack overflows.

To reduce the chance of this happening, I added a busy loop to wait that our lookup getLayoutForInstancesToUpdateObject() actually returns a valid layout. It might still be invalidate before use again, but hopefully this happens now less often, and prevents stack overflows.

This is likely related to #85

@daumayr probably relevant for you, you might still have seen such races in your benchmarks

This change introduces a busy loop, for which we might want to add Thread.onSpinWait() once we can use JDK 9.

If multiple threads mutate objects from the same class, it can happen that the object layout gets invalidated while we do an update in another thread.

In the worst case, this can lead to stack overflows, because our interpreter tries repeatedly to find a valid object layout while creating dispatch chains, or interacting with the object layout in other nodes, which use recursive fallbacks.

To reduce the chance of this happening, I added a busy loop to wait that our lookup getLayoutForInstancesToUpdateObject() actually returns a valid layout. It might still be invalidate before use again, but hopefully this happens now less often, and prevents stack overflows.

Signed-off-by: Stefan Marr <git@stefan-marr.de>
@smarr smarr added the bug Fixes an issue, incorrect implementation label Mar 24, 2018
@smarr smarr added this to the v0.6.0 - Black Diamonds milestone Mar 24, 2018
@smarr smarr mentioned this pull request Mar 24, 2018
3 tasks
@daumayr
Copy link
Contributor

daumayr commented Mar 24, 2018

Good to hear there is progress on this, surprisingly i haven't had issues with this for a while.

@coveralls
Copy link

coveralls commented Mar 24, 2018

Coverage Status

Coverage remained the same at 77.312% when pulling d3ce30b on so-on-race into adfb8c4 on dev.

@smarr
Copy link
Owner Author

smarr commented Mar 25, 2018

The latest changes also solve an issue with not handling invalid layouts in the ClassSlotAccessNode.
This could cause data been written to the wrong memory address. While the object layout of the object potentially already has been update.

It is hard to test, but at least I observed an A being read where an R should be read (both are inner classes in the Parser.ns BasicInterpreterTest). The debugging code is available here: https://gist.github.com/smarr/c7b626dbc8d8c33251d952d6653c2ef3
The following BasicInterpreterTests where of good help:

{"Parser", "testOuterInKeyword", 32 * 32 * 32, Long.class, null},
{"Parser", "testOuterWithKeyword", 3 * 4, Long.class, null},

By checking whether the read/write operations are valid, we can avoid the issue.
In case they are invalid, we will fall back to the standard accessors on SObject, which do the right thing.

@smarr smarr changed the title Avoid StackOverflow and Increase chance that objects are update with valid layouts Reduce race-related stack overflows, and handled invalid layouts in ClassSlotAccessNode Mar 25, 2018
smarr added 2 commits March 25, 2018 12:21
Signed-off-by: Stefan Marr <git@stefan-marr.de>
Signed-off-by: Stefan Marr <git@stefan-marr.de>
@smarr smarr merged commit aa508e2 into dev Mar 25, 2018
@smarr smarr deleted the so-on-race branch March 25, 2018 11:28
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Fixes an issue, incorrect implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants