On 11/05/2013 03:21 AM, Mandy Chung wrote:
On 11/4/2013 6:03 PM, David Holmes wrote:
Hi Mandy,
This all seems quite reasonable to me.
Two minor nits:
1. The "delay ntil" typo in Finalizer.java is still present.
Missed that :(
2. In VM.java. booted need not be volatile now that it is only
accessed within a locked region. Also awaitBooted might as well be
void as it can only ever return true.
Fixed. Revised webrev at:
http://cr.openjdk.java.net/~mchung/jdk8/webrevs/8027351/webrev.03/
Hi Mandy,
Just a nit. You are assigning SharedSecrets.getJavaLangAccess() to a
local 'jla' variable declared outside Runnables, which makes it
effectively a final field. You could declare and assign the 'jla' inside
run() methods of both Runnables, just before the loop - like it is done
in the FinalizerThread...
Regards, Peter
P.S. I'm curious about the strange seemingly unneeded code fragments in
FinalizerThread and both Runnables. For example:
169 forkSecondaryFinalizer(new Runnable() {
170*private volatile boolean running;*
171 public void run() {
172*if (running)*
173*return;*
174*running = true;*
The FinalizerThread and each Runnable instance is only executed once.
Why these checks then? Does anybody know?
Regards, Peter
Thanks for the review.
Mandy
Thanks,
David
On 5/11/2013 6:04 AM, Mandy Chung wrote:
Thank you all for the suggestions.
Before the VM initialization is completed, any agent ought to be
careful
in what things it can do and what shouldn't do. I think it's
reasonable
to ignore System.runFinalization if it's called at early startup phase.
I'm unclear if there is any use case that we really require
finalization
to happen before VM is booted (I can imagine other problems may
happen).
I change the runFinalization and runAllFinalizers methods to return if
it's not booted and also change runFinalizer method to take
JavaLangAccess to simplify the synchronization instead of caching it in
the static field.
http://cr.openjdk.java.net/~mchung/jdk8/webrevs/8027351/webrev.02/
David - I decided to leave VM.awaitBooted as it is. Having the
callsite
to call awaitBooted makes it explicit and clear that it's blocking and
we will keep SharedSecrets.getJavaLangAccess method consistent with the
other SharedSecrets methods that they are all getter methods. If any
future change calls SharedSecrets.getJavaLangAccess before it's booted,
it's a little easier to diagnose (NPE vs hangs).
Peter - thanks for the ObjectAccess idea. I don't think supporting
finalization to happen before VM is booted buys us anything and I would
rather keep this change simple.
Mandy
