Implement proper thread-safety for concurrent ObjectMarker #137
Conversation
Introduce a ConcurrentBitField backed by an AtomicLongArray, and use it to do proper CAS-based state tracking in the concurrent ObjectMarker. This is a potential fix to issue #57.
| import org.junit.Test; | ||
| import org.junit.rules.ExpectedException; | ||
|
|
||
| public class ConcurrentBitFieldTest |
There was a problem hiding this comment.
Please suggest any additional / fixes to tests.
| Bundle-SymbolicName: org.eclipse.mat.report;singleton:=true | ||
| Bundle-Version: 1.17.0.qualifier | ||
| Bundle-RequiredExecutionEnvironment: JavaSE-1.8 | ||
| Bundle-RequiredExecutionEnvironment: JavaSE-17 |
There was a problem hiding this comment.
This was required to support compareAndExchange. If we cannot do xchg, it can be done with compareAndSet, but involves additional CAS operations.
| // Therefore, we need to use full compare and exchange rather than | ||
| // compare and set. | ||
|
|
||
| while (true) |
There was a problem hiding this comment.
Appreciate any extra eyes on this!
There was a problem hiding this comment.
A colleague suggested adding a jcstress, did so in 9499a38.
Using a standalone project and readme since the jcstress modules are not made available for use with tycho or packaged in orbit. Added a README to explain.
|
@jasonk000 Sorry for the terribly long delay. I fell behind on everything for the last 5 months or so and I'm finally starting to dig out. I reviewed the PR and it's quite impressive, especially all of the testing (I think splitting out jcstress is okay), so, overall, it looks good to me. I think it's fine to require later versions of Java (though I notice the pom.xml for jcstress has a javac.target of Java 8?). We can try this out on our support systems. However, are we sure this is a concurrency issue? As I was re-familiarizing myself with this issue, I noticed that the symptoms that we saw showed single-threaded execution: And that's not changed in this latest PR: |
This implementation counts as we go, previously it counted before and after and used the difference as the marked count. If we had a sparse set of bits to count, and a large heap, a lot of calls to mark might result in us using a lot of time just in counting. Hopefully now we do not spend any time in counting.
| try | ||
| { | ||
| markMultiThreaded(1); | ||
| return (int) markMultiThreadedInner(1); |
There was a problem hiding this comment.
This should be convertible to multi-thread-able if this works.
|
@kgibm thanks for the feedback!
Yes, this is something I struggled to follow/understand what was going wrong. With some time and your nudge, I realize now there's a solution to cover both the counting and the marking: count as we go, instead of counting repeatedly. --> see here d094d1d I've done this by switching to a CountedCompleter implementation and the tests pass. I wonder if you can test this out against a slow hprof? |
Introduce a ConcurrentBitField backed by an AtomicLongArray, and use it to do proper CAS-based state tracking in the concurrent ObjectMarker.
This is a potential fix to issue #57.