RFR: 8241371: Refactor and consolidate package_from_name

[Claes Redestad]

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.

> 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?

> 
> - 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";
}

OK, or do you need a new webrev?

/Claes