review (M) for 6965184: possible races in make_not_entrant_or_zombie
Vladimir Kozlov
vladimir.kozlov at oracle.com
Fri Jul 2 17:18:49 PDT 2010
Tom,
Nice VTune cleanup :)
Thank you for consolidating sweep logging code.
In nmethod.cpp why you don't use the_method() in your new code (// Remove nmethod from method) ?
In sweeper.cpp why you need second (_invocations > 0) check? Only current thread will modify _invocations now after your changes.
Why you replaced next code:
! // We want to visit all nmethods after NmethodSweepFraction invocations.
! // If invocation is 1 we do the rest
! int todo = CodeCache::nof_blobs();
! if (_invocations > 1) {
! todo = (CodeCache::nof_blobs() - _seen) / _invocations;
! }
...
! for(int i = 0; i < todo && _current != NULL; i++) {
whith this:
! int todo = (CodeCache::nof_nmethods() - _seen) / _invocations;
...
! // The last invocation iterates to the end of the code cache
! for (int i = 0; (i < todo || _invocations == 0) && _current != NULL; i++) {
First, _invocations can't be 0 according to the code in possibly_sweep()
Second, if it was 0 then you will get division by 0 in todo expression.
Next assert also looks strange:
+ assert(_invocations > 0 || _current == NULL, "must have scanned the whole cache");
Vladimir
Tom Rodriguez wrote:
> http://cr.openjdk.java.net/~never/6965184
>
> 6965184: possible races in make_not_entrant_or_zombie
> Reviewed-by:
>
> While investigating the sweeper concurrency problems I identified a
> bunch of minor issues that could lead to races. NMethod groups a
> bunch of flags into a struct using bitfield which was okay in the past
> because they were either written by the constructor, at a safepoint or
> with a single lock held. Now some of those fields are written under
> one lock and some are written under another so they should be
> separated. I've done away with nmFlags all together and moved a bunch
> of common initialization into a new method. I also deleted a other of
> unused or dead things.
>
> In make_not_entrant_or_zombie, all the zombie logic used to be
> performed at a safepoint but now it can happen concurrently so the
> logic that is executed outside of the Patching_lock has to be more
> careful. All the not_entrant logic has been moved into the region
> protected by the Patching_lock. The link between the nmethod and the
> methodOop is also broken ealier though a strong reference to the
> methodOop is maintained to make sure it stays alive.
>
> There were problems with the maintenance of _sweep_started and
> _invocations that could mistakenly allow another thread into
> possibly_sweep but only if there was no work to be done. Also threads
> besides the sweeping thread could clear _sweep_started.
>
> The sweeper divides the cache based on the number of blobs even though
> it only works on nmethod, so I added a nof_nmethods count along with a
> nof_adapters count just for informational purposes.
>
> As a cleanup I also deleted the dead and unused VTune logic since it
> doesn't work and VTune uses JVMTI agents these days.
>
> Tested with NSK jvmti,jdi,jdb,hprof,jit,regression and JDI_REGRESSION
> tests.
More information about the hotspot-compiler-dev
mailing list