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
