Hi Alan, Joel,

Thanks to both for taking time to review the patch.

Here's the 3rd revision:

http://cr.openjdk.java.net/~plevart/jdk8-tl/AnnotationType/webrev.03/

Answers inline...

On 07/03/2013 04:51 PM, Alan Bateman wrote:
Sorry for the delay getting back to you on this, I've been busy with other things.

Me too.


I'm not an expert on the annotation code but the updated approach to break the recursion looks good good to me (and a clever approach). Joel has been on vacation but he told me that he plans to look at this too (I'm happy to sponsor it in the mean-time as I think the approach is good and we should get this fix in).

There's another usage of AnnotationParser.parseAnnotation in TypeAnnotationParser that will need to be updated (I only noticed it when I grabbed your patch to try it).

I rather restored the package-private API so that TypeAnnotationParser doesn't have to be changed. It also makes for less changes in internal AnnotationParser code.


A minor comment is that the alignment of the parameter declarations when they go over multiple lines should probably be fixed up to be consistent with the other usages.

I tailored new method to be consistent with the alignment of the other public method. There are 4 different multi-line-parameter alignments used in AnnotationParser though. I also renamed new method to "parseSelectAnnotations" and made it package-private, since it is only used from the AnnotationType.


Thanks for adding tests. One comment on AnnotationTypeDeadlockTest is that the join(500) might not be sufficient on a very busy system (say when running tests concurrently) or on a low-end embedded system. So I think it would be good to bump this up x10.

Done.

An alternative would be to not set the daemon status and just let the test timeout if there is a deadlock. The spin-waits can consume cycles when running tests concurrently but I don't expect it's an issue here.

Right, There can be a substantial delay from Thread.start() to thread actually executing it's run() method. So I added another synchronization latch that makes main thread park and wait until both new threads start-up. Both new threads then spin-wait until main thread wakes up and resets the AtomicInteger value. Experimenting on my Intel i7 box shows that one new thread spins about 80x and the other about 3000x before continuing. This spin-waiting is necessary so that both threads continue coherently and have a better chance to provoke deadlock.


A typo in the @summary of AnnotationTypeRuntimeAssumptionTest ("si" -> "is"). I guess I'd go for slightly shorter lines to make future side-by-side reviews easier.

Corrected.

On 07/03/2013 07:09 PM, Joel Borggrén-Franck wrote:
Hi Peter,

As Alan said, a big thanks for looking into this.

I have been toying with a slightly different approach to breaking the infinite 
recursion by pre-construct AnnotationType instances for Retention and 
Inherited. While cleaner in some places others became uglier. I'm fine with 
this approach.

I'm interested in your approach. How do you handle construction of AnnotationType instances for other annotations apart from @Retention and @Inherited? What if there are two mutually recursive annotations like:


    @Retention(RUNTIME)
    @AnnB
    public @interface AnnA {
    }

    @Retention(RUNTIME)
    @AnnA
    public @interface AnnB {
    }

How do you construct the AnnotationType instance for any one of the above annotations and break recursion?

As Alan said, you need to update the patch with a modification in 
TypeAnnotationParser. After doing that all java/lang tests in the jdk/test 
directory passes.

I missed that, yes. I changed the patch so that no changes are needed now.

Also, can you please explain to me why the update race is safe. I have done 
the/some research myself but I would like to know which angles you have covered.

Well, one thing is that AnnotationType class is now effectively final, meaning that all it's state is referenced using final fields. If a reference to an instance of such class is obtained via data-race, all it's state is observed consistent by the thread that obtained the reference. The other thing is racy caching. If two or more threads are concurrently requesting AnnotationType for the same Class, many of them might construct and use it's own instance and the one published "latest" will finally be the one being cached. Since all such AnnotationType instances are constructed from the same input data, they are equivalent and it doesn't matter which one is used by which thread.

There is a caveat though. What if class is redefined? That's a separate issue and will have to be resolved in a separate patch. I'm planing to prepare a patch after this one gets through. The patch will remove contention from caching of annotations and may also take care of AnnotationType caching at the same time.

Regards, Peter

Given that and that you fix Alan's comments I think this is good to go.

Thanks again!

cheers
/Joel


Reply via email to