RFR (s) 8143269: Refactor code in universe_post_init that sets up methods to upcall

Tue Mar 22 21:50:39 UTC 2016

Thank you, Harold!
Coleen

On 3/22/16 1:24 PM, harold seigel wrote:
> Hi Coleen,
>
> Your latest changes look good.
>
> Thanks, Harold
>
> On 3/22/2016 8:18 AM, 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 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
>>
>>
>>>> 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.
>>>
>>> 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
>>>>
>>
>