RFR: 8153858: Clean up needed when obtaining the package name from a fully qualified class name
Rachel Protacio
rachel.protacio at oracle.com
Tue Jun 7 15:00:45 UTC 2016
Thanks for the reviews, Coleen and David! Will make these edits, then
commit.
Rachel
On 6/6/2016 6:46 PM, Coleen Phillimore wrote:
>
> Hi,
>
> http://cr.openjdk.java.net/~rprotacio/8153858.02/src/share/vm/oops/instanceKlass.cpp.udiff.html
>
>
> The comment
>
> +// Used to obtain the package name from a fully qualified class name.
> +// It is the responsibility of the caller to establish ResourceMark.
> +Symbol* InstanceKlass::package_from_name(const Symbol* name, TRAPS) {
>
> Is confusing since you have a ResourceMark a few lines down.
>
> http://cr.openjdk.java.net/~rprotacio/8153858.02/src/share/vm/classfile/systemDictionary.cpp.udiff.html
>
>
> + TempNewSymbol pkg_name =
> InstanceKlass::package_from_name(class_name, CHECK_(nh));
>
>
> Can you change the end to CHECK_NULL? Trying to get rid of these
> CHECK_(nh).
>
> http://cr.openjdk.java.net/~rprotacio/8153858.02/src/share/vm/classfile/classLoader.cpp.udiff.html
>
>
> + TempNewSymbol pkg_name =
> InstanceKlass::package_from_name(class_name, THREAD);
>
>
> The last parameter of this should be CHECK_NULL since the
> SymbolTable::new_symbol() can throw OutOfMemoryError.
>
> These are minor comments, so I don't need to see another webrev.
>
> This is a nice consolidation of tedious character parsing code!
>
> Thanks,
> Coleen
>
>
> On 6/6/16 4:19 PM, Rachel Protacio wrote:
>> Also, I updated systemDictionary.cpp at line 1161 to use
>> StringUtils::replace_no_expand() instead of stepping through the
>> string. http://cr.openjdk.java.net/~rprotacio/8153858.02/
>>
>> Rachel
>>
>>
>> On 6/6/2016 2:52 PM, Rachel Protacio wrote:
>>> Hi David,
>>>
>>> I see what you're suggesting with the test, but since there is no
>>> THREAD to use in the gtest, I can't for example write
>>> TempNewSymbol sym = SymbolTable::new_symbol("", THREAD);
>>> to then pass into InstanceKlass::package_from_name. On the other
>>> hand, creating a new_symbol("", NULL) succeeds I think because it
>>> looks up the symbol with the empty string name which happens to
>>> exist in the table, but in reality it cannot create a new symbol on
>>> a non-existent thread. So even though it would be good to test, I
>>> don't think there's a way to do it. What do you think?
>>>
>>> Thanks,
>>> Rachel
>>>
>>> On 6/5/2016 9:15 PM, David Holmes wrote:
>>>> Hi Rachel,
>>>>
>>>> On 4/06/2016 6:20 AM, Rachel Protacio wrote:
>>>>> Hi,
>>>>>
>>>>> Thanks for the comments. New webrev:
>>>>> http://cr.openjdk.java.net/~rprotacio/8153858.01/
>>>>
>>>> All changes look okay to me.
>>>>
>>>> Regarding the test ... you stated that because gtest can't pass
>>>> THREAD that the InstanceKlass functionality is just tested via JCK
>>>> tests. But in fact you do have one test for
>>>> InstanceKlass::package_from_name where you pass in NULL as THREAD.
>>>> Given the code in package_from_name:
>>>>
>>>> 2187 Symbol* InstanceKlass::package_from_name(const Symbol* name,
>>>> TRAPS) {
>>>> 2188 if (name == NULL) {
>>>> 2189 return NULL;
>>>> 2190 } else {
>>>> 2191 if (name->utf8_length() <= 0) {
>>>> 2192 return NULL;
>>>> 2193 }
>>>> 2194 ResourceMark rm;
>>>> 2195 const char* package_name =
>>>> ClassLoader::package_from_name((const char*) name->as_C_string());
>>>> 2196 if (package_name == NULL) {
>>>> 2197 return NULL;
>>>> 2198 }
>>>> 2199 Symbol* pkg_name = SymbolTable::new_symbol(package_name,
>>>> THREAD);
>>>> 2200 return pkg_name;
>>>> 2201 }
>>>> 2202 }
>>>>
>>>> You are currently testing the return path at L#2189. In theory you
>>>> can test all return paths until the NULL THREAD would cause a
>>>> problem. So you could have a test to return at L#2192 and L#2197.
>>>> But given the NULL returning calls to
>>>> ClassLoader::package_from_name are already being independently
>>>> tested, there is no need to duplicate them. So that just leaves
>>>> adding a test for return at L#2192 - which I'm guessing using "" as
>>>> the name would test.
>>>>
>>>> Thanks,
>>>> David
>>>> -----
>>>>
>>>>> Replies inline.
>>>>>
>>>>> On 6/3/2016 1:48 AM, David Holmes wrote:
>>>>>> Hi Rachel,
>>>>>>
>>>>>> Overall looks good, but a few comments below.
>>>>>>
>>>>>> On 3/06/2016 3:12 AM, Rachel Protacio wrote:
>>>>>>> Hello,
>>>>>>>
>>>>>>> Please review this fix, which consolidates/refactors all the
>>>>>>> instances
>>>>>>> of code parsing fully qualified names and returning the package
>>>>>>> name.
>>>>>>> Includes gtests for the functions. The gtest framework does not
>>>>>>> allow
>>>>>>> for passing in a THREAD, so the InstranceKlass functionality is
>>>>>>> just
>>>>>>> tested by jck tests.
>>>>>>
>>>>>> src/share/vm/classfile/classLoader.hpp
>>>>>>
>>>>>> Can you document what the bad_class_name boolean indicates
>>>>>> please. It
>>>>>> took me a while to figure out that
>>>>>> InstanceKlass::is_same_class_package (but seemingly no other caller)
>>>>>> needs to distinguish between "has no package" and "an error occurred
>>>>>> trying to determine what the package is"
>>>>>>
>>>>> Added.
>>>>>> ---
>>>>>>
>>>>>> src/share/vm/classfile/systemDictionary.cpp
>>>>>>
>>>>>> 1157 TempNewSymbol pkg_name =
>>>>>> InstanceKlass::package_from_name(parsed_name, THREAD);
>>>>>>
>>>>>> Why are you not using a CHECK macro here?
>>>>> My mistake. I've put in a CHECK_NULL.
>>>>>>
>>>>>> ---
>>>>>>
>>>>>> src/share/vm/oops/instanceKlass.cpp
>>>>>>
>>>>>> 2318 bool bad_class_name1 = false;
>>>>>> 2324 bool bad_class_name2 = false;
>>>>>>
>>>>>> You only need a single local variable.
>>>>>>
>>>>>> 2336 // Check that package is identical
>>>>>> 2337 return !strcmp(name1, name2);
>>>>>>
>>>>>> Please avoid implicit booleans: return strcmp(name1, name2) == 0;
>>>>> Ok.
>>>>>>
>>>>>> ---
>>>>>>
>>>>>> test/native/runtime/test_instance_klass.cpp
>>>>>>
>>>>>> Should the naming convention here match the source file that
>>>>>> holds the
>>>>>> code being tested ie test_instanceKlass and test_classLoader?
>>>>> Done.
>>>>>>
>>>>>> It seems that without being able to pass a Thread you can test a
>>>>>> number of inputs, as long as you won't reach the
>>>>>> SymbolTable::new_symbol call. That said the call to
>>>>>> ClassLoader::package_from_name is already being tested
>>>>>> separately, so
>>>>>> it probably suffices to just add a test for the utf8_length() check.
>>>>> Sorry, I'm not sure what you mean here. Can you elaborate?
>>>>>>
>>>>>>> Note that the change in method.hpp was a
>>>>>>> compilation error uncovered by my gtest.
>>>>>>
>>>>>> ??? Your change would seem to me to be introducing a potential
>>>>>> compilation error:
>>>>>>
>>>>>> return (sizeof(Method)/wordSize + is_native()) ? 2 : 0;
>>>>>>
>>>>>> this adds a boolean to an integer. And the result will always be 2!
>>>>> Oops, you're right! I've moved the parentheses around the ternary
>>>>> statement.
>>>>>
>>>>> Rachel
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>>
>>>>>>>
>>>>>>> Passes hotspot gtests, jck lang and api/java_lang tests, JPRT,
>>>>>>> and RBT
>>>>>>> hotspot and non-colo tests.
>>>>>>>
>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8153858
>>>>>>> Open webrev: http://cr.openjdk.java.net/~rprotacio/8153858.00/
>>>>>>>
>>>>>>> Thank you,
>>>>>>> Rachel
>>>>>
>>>
>>
>
More information about the hotspot-runtime-dev
mailing list