RFR: 8241371: Refactor and consolidate package_from_name

[Lois Foltan]

On 3/23/2020 10:07 AM, Claes Redestad wrote:
>
>
> On 2020-03-23 14:41, Lois Foltan wrote:
>>>
>>>
>>> http://cr.openjdk.java.net/~redestad/8241371/open.01/
>>>
>>> /Claes
>>
>> Hi Claes,
>>
>> This looks great, thank you for looking into this improvement! 
>
> Thanks for reviewing, Lois!
>
>> Few review comments:
>>
>> - classfile/classLoader.cpp
>> line #181: ClassLoader::package_from_class_name(), could we remove 
>> all the checks for "if (bad_class_name != NULL)" in favor of an 
>> assert ahead of line #182 that just "assert(bad_class_name != NULL, 
>> "caller did not set bad_class_name correctly");"
>
> Hmm, passing NULL for bad_class_name is the common case, since only one
> caller is testing for malformed class names.
>
> As a future simplification/micro-optimization I think we could get rid
> of the first test (if (name == NULL)) and replace that and a few other
> NULL-checks with asserts since they seem to be impossible. I've gotten
> that through tier1-3, but decided I did not want to overburden this RFE.
Ideally I wish we could get rid of the parameter all together, but that 
is for another time.

>
>> line #197: I still wish we could get rid of that do while loop. Would 
>> searching for the last '[' be better performance wise?  Like 
>> UTF8::strrchr(start, utf_len, JVM_SIGNATURE_ARRAY)?
>
> I think that could have some cost since we'll scan the entire string.
>
> Perhaps an UTF8::mismatch(start, end, JVM_SIGNATURE_ARRAY) could be
> added, but the if-do-while construct is nice here since the check for
> JVM_SIGNATURE_CLASS is moved off the critical path. Either way I think
> this is out of scope for this RFE, since the do-while loop is pre-
> existing?
Agree, thanks for considering it.

>
>>
>> - gtest/runtime/test_classLoader.cpp
>> please include a test that has a multi-dimensional array
>
> Adding:
>
> TEST_VM(ClassLoader, class_multiarray) {
>   bool bad_class_name = false;
>   TempNewSymbol name = SymbolTable::new_symbol("[[package/class");
>   TempNewSymbol retval = ClassLoader::package_from_class_name(name, 
> &bad_class_name);
>   ASSERT_FALSE(bad_class_name) << "Function set bad_class_name with 
> class array";
>   ASSERT_TRUE(retval->equals("package")) << "Wrong package for class 
> with leading bracket";
> }
Thank you!

>
> OK, or do you need a new webrev?
No, thanks!

>
> /Claes