[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Re: RFR (s) 8143269: Refactor code in universe_post_init that sets up methods to upcall
From:       Coleen Phillimore <coleen.phillimore () oracle ! com>
Date:       2016-03-23 11:49:05
Message-ID: 56F282B1.9020200 () oracle ! com
[Download RAW message or body]



On 3/23/16 7:40 AM, David Holmes wrote:
> Hi Coleen,
>
> Thank goodness for inflight wi-fi, I nearly missed this when my 
> battery died at the airport :)

Have a good vacation!   I was going to list you as a reviewer if you 
didn't answer anyway.  I think the vm initialization cleanup work is 
something that will be around when you get back ;)

Thanks for closing the loop on this.
Coleen
>
> Thumbs up! More inline ...
>
> On 22/03/2016 10:18 PM, Coleen Phillimore wrote:
>>
>> Hi David,
>> Thank you for reviewing.
>>
>> On 3/21/16 10:07 PM, David Holmes wrote:
>>> On 22/03/2016 12:05 AM, Coleen Phillimore wrote:
>>>> On 3/21/16 1:12 AM, David Holmes wrote:
>>>>> Hi Coleen,
>>>>>
>>>>> On 19/03/2016 10:57 PM, Coleen Phillimore wrote:
>>>>>> Summary: Deferred code review cleanups
>>>>>>
>>>>>> This was a review comment I had when the StackWalk API was 
>>>>>> integrated.
>>>>>> There were too many of this same code pattern in the middle of
>>>>>> universe_post_init.  The only real change here is that if the method
>>>>>> isn't found and doesn't have the right linkage because of a
>>>>>> mismatch in
>>>>>> JDK version, we can vm_exit_during_initialization() rather than 
>>>>>> try to
>>>>>> return JNI_ERR.  This avoids ugly code to check
>>>>>> initialize_known_method's boolean result for each call. We can't
>>>>>> throw
>>>>>> an exception here.  Because of the EXCEPTION_MARK in
>>>>>> universe_post_init,
>>>>>> any exception gets turned into vm_exit_during_initialization
>>>>>> anyway. But
>>>>>> this wouldn't be a user error, so vm_exit_during_initialization 
>>>>>> seemed
>>>>>> like the cleanest choice.
>>>>>
>>>>> Our initialization code is truly awful. Until the VM is initialized
>>>>> any/all exceptions have to convert back to a different form of error
>>>>> because there will be no thread (or JVM for that matter) on which the
>>>>> user code could ask if an exception occurred. It then comes down to
>>>>> whether an error should be reported to the caller of JNI_CreateJavaVM
>>>>> so the host program doesn't get terminated, or we abort everything
>>>>> with vm_exit_during_initialization. And we're sliding further and
>>>>> further into always doing the latter.
>>>>>
>>>>> So in an absolute sense the new code is incompatible with the old
>>>>> because it will terminate host applications that wouldn't previously
>>>>> have been terminated - but in a practical sense this is unlikely to
>>>>> make any real difference to anything.
>>>>>
>>>>> That said something about all this is really bugging me. If we can 
>>>>> do:
>>>>>
>>>>> ! SystemDictionary::Finalizer_klass()->link_class(CHECK_false);
>>>>>
>>>>> which purports to allow an exception to have been posted then how is
>>>>> it the immediately following code:
>>>>>
>>>>> !   Method* m = SystemDictionary::Finalizer_klass()->find_method(
>>>>> ! vmSymbols::register_method_name(),
>>>>> ! vmSymbols::register_method_signature());
>>>>> !   if (m == NULL || !m->is_static()) {
>>>>> !     tty->print_cr("Unable to link/verify Finalizer.register 
>>>>> method");
>>>>> !     return false; // initialization failed (cannot throw exception
>>>>> yet)
>>>>> !   }
>>>>>
>>>>> can not throw an exception yet? Either both can throw or neither can
>>>>> throw AFAICS. So maybe the refactored link() call should not be using
>>>>> CHECK_ but also exit immediately?
>>>>
>>>> I don't think we should refactor InstanceKlass::link_class() for this.
>>>
>>> I wasn't suggesting that. As I said either both bits of code can throw
>>> exceptions or neither can, and I would have to assume it is the
>>> latter. link_class can be replaced by link_class_or_fail ie this:
>>>
>>>    ik->link_class(CHECK);
>>>    TempNewSymbol name = SymbolTable::new_symbol(method, CHECK);
>>>    Method* m = ik->find_method(name, signature);
>>>    if (m == NULL || is_static != m->is_static()) {
>>>      ResourceMark rm(THREAD);
>>>      vm_exit_during_initialization(err_msg("Unable to link/verify
>>> %s.%s method",
>>> ik->name()->as_C_string(), method));
>>>    }
>>>
>>> becomes (hope I get this right):
>>>
>>> TempNewSymbol name = SymbolTable::new_symbol(method, CHECK);
>>> Method* m = NULL;
>>> if (!ik->link_class_or_fail(THREAD) ||
>>>     ((m = ik->find_method(name, signature)) == NULL) ||
>>>     is_static != m->is_static()) {
>>>   ResourceMark rm(THREAD);
>>>   vm_exit_during_initialization(err_msg("Unable to link/verify %s.%s
>>> method",
>>> ik->name()->as_C_string(), method));
>>> }
>>>
>>> The CHECK on new_symbol is still a pretense but ...
>>>
>>
>> Yes, the CHECK in link_class is also a pretense.  I added some dummy
>> code to throw a VerifyError and it got the assertion:
>>
>> #  assert(resolved_method->method_holder()->is_linked()) failed: must be
>> linked
>>
>>  From trying to call the Throwable.<clinit>.  I thought we had fixed the
>> jvm to handle this better.
>
> I think we moved the failure point but ultimately there has to be some 
> point before which you can't throw an exception as it depends on 
> initialization of classes that have failed for some reason. But as I 
> said it would be interesting to see the actual order of linking for 
> this particular chunk of code.
>
>> I think the code I had is easier to read, but in appreciation for your
>> code reviews, I've changed it to your if statement and retested.
>>
>> http://cr.openjdk.java.net/~coleenp/8143269.02/webrev
>
> Much appreciated. It really makes little sense to me to pretend to 
> throw on one line and then vm-exit because we know we can't throw.
>
>>
>>>> I can rewrite so that every initialize_known_method call does:
>>>>
>>>> bool ok;
>>>> ok = initialize_known_method(<params>, CHECK);
>>>> if  (!ok) return ok;
>>>>
>>>> ok = initialize_known_method(<params>, CHECK);
>>>> if (!ok) return ok;
>>>>
>>>> ...
>>>>
>>>> return true;
>>>>
>>>> Which I thought was really ugly and meaningless, since as I said, this
>>>> case is really an assert that the library doesn't match.
>>>
>>> I'm not sure why you are suggesting that. It would only make sense if
>>> if there were no vm_exit_during_initialization. ??
>>
>> Right, which I thought was the objection.
>
> I was commenting on the potential change of behaviour but as I noted 
> it is not a practical matter and as you said these failures really 
> indicate a basic error in terms of the jdk+hotspot combination. So all 
> good there. (I should make my commentary and my "review"  more separate.)
>
> Thanks,
> David
> -----
>
>>>
>>> It would be interesting to see exactly when java_lang_Class is linked
>>> in relation to this code executing, so we could see when it would be
>>> possible to just throw the exceptions in all cases. Pity there seems
>>> to be no logging for class linking.
>>>
>>>> I could change it to an assert.
>>>>
>>>> Which is your preference?  This is not my preference.
>>>
>>> I'm not sure what you would assert and where ??
>>>
>>
>> I meant a fatal() call instead of vm_exit_during_initialization() since
>> the case that I'm calling it is an error in the JVM, not really from a
>> users perspective.
>>
>> And YES, initialization is a messy business.  Kim filed this bug a while
>> ago for one aspect of initialization.  Throwing exceptions and/or
>> returning JNI_ERR is another aspect that makes it messy. Returning
>> JNI_ERR to some process embedding the JVM means that we've cleaned up
>> memory that we've allocated, of course, we haven't done that.  I think
>> we have other bugs like that too.
>>
>> https://bugs.openjdk.java.net/browse/JDK-8068392
>>
>> Thanks,
>> Coleen
>>
>>> Thanks,
>>> David
>>>
>>>> Thanks,
>>>> Coleen
>>>>>
>>>>>> Also, I want to get the code to not have to include method names 
>>>>>> (and
>>>>>> field names) in vmSymbols.cpp.
>>>>>>
>>>>>> I verified that this code won't merge conflict with jigsaw. There 
>>>>>> are
>>>>>> many additional cleanups that can be done here. universe_post_init
>>>>>> should really be a method belonging to Universe since it sets many
>>>>>> static fields in Universe, but to avoid merge conflicts, I didn't do
>>>>>> that.
>>>>>
>>>>> If you do that cleanup some time there are some very inaccurate
>>>>> comments in relation to the initialization code in init.cpp that 
>>>>> could
>>>>> do with correcting. And I also spotted two paths to
>>>>> Interpreter::initialize.
>>>>>
>>>>> Thanks,
>>>>> David
>>>>> -----
>>>>>
>>>>>>
>>>>>> Tested with JPRT and colocated tests.
>>>>>>
>>>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8143269.01/webrev
>>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8143269
>>>>>>
>>>>>> Thanks,
>>>>>> Coleen
>>>>
>>

[prev in list] [next in list] [prev in thread] [next in thread]