RFR: 8241371: Refactor and consolidate package_from_name

[Ioi Lam]

On 3/22/20 4:02 PM, Claes Redestad wrote:
> Hi Ioi,
>
> On 2020-03-22 23:45, Ioi Lam wrote:
>> Hi Claes,
>>
>> This is a good optimization. Looks good to me. Here are some minor nits:
>>
>> [1] ClassLoader::search_module_entries may be called twice by the 
>> same caller,
>>      I think it's better to compute the mod_entry in the caller, so 
>> you can avoid
>>      the pkg_entry lookup twice.
>
> ClassLoader::search_module_entries would only be called twice if
> running with --patch-module on an exploded build, which doesn't seem
> like a case we should bother to optimize for.
>
>
>>
>> [2] For tracking the code changes, I think it's better to move your 
>> new version
>>      of InstanceKlass::package_from_name() to 
>> ClassLoader::package_from_name().
>>
>>      I did a diff of the new version of 
>> InstanceKlass::package_from_name() and
>>      the old version of ClassLoader::package_from_name(), and I can 
>> see that the
>>      logic has not changed (the only changes are how we allocate the 
>> buffers).
>>
>>      With the current patch, it's hard to see what has changed and 
>> difficult to
>>      validate its correctness.
>
> Ok, I just felt that as a static utility it's more coupled with a 
> class/InstanceKlass than its class loader. If you insist I'll move it
> to ClassLoader instead.
>
classLoader.cpp also has other methods related to packages, so I think 
it's a good place for this method, too. Plus, you don't need to move the 
test cases, so there's less code churn.

Thanks
- Ioi

>>
>>      Also, it's better to rename the function to
>>      ClassLoader::package_from_class_name()
>
> Sure, that sounds better!
>
> /Claes
>
>>
>> Thanks
>> - Ioi
>>
>>
>>
>> On 3/22/20 2:22 PM, Claes Redestad wrote:
>>> Correct webrev: http://cr.openjdk.java.net/~redestad/8241371/open.00/
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8241371
>>>
>>> On 2020-03-22 17:15, Claes Redestad wrote:
>>>> Hi,
>>>>
>>>> JDK-8153858 partially consolidated the logic shared between
>>>> InstanceKlass::package_from_name and ClassLoader::package_from_name.
>>>> Unfortunately it also caused an undetected regression in some paths 
>>>> due
>>>> need to copy from Symbol -> resource arrays and back again.
>>>>
>>>> By refactoring package_from_name into a single implementation which
>>>> operates on a Symbol* rather than a char* we can both simplify a bit
>>>> and regain some lost performance:
>>>>
>>>> Testing: tier1-5, verified a ~0.5% improvement on startup
>>>>
>>>> Thanks!
>>>>
>>>> /Claes
>>