Hi Serguei,
Thank you for the review.
On Jan 29, 2015, at 11:53 PM, serguei.spit...@oracle.com wrote:
> Jiangli,
>
> The fix looks good.
> One minor comment/question though.
>
> It looks like the comment needs an update as it does not return null anymore
> if the string has not been found in the symbol table.
I’ll update the comments to reflect the latest code.
>
> agent/src/share/classes/sun/jvm/hotspot/memory/SymbolTable.java
> @@ -74,20 +81,23 @@
> /** Clone of VM's "temporary" probe routine, as the SA currently
> does not support mutation so lookup() would have no effect
> anyway. Returns null if the given string is not in the symbol
> table. */
> public Symbol probe(byte[] name) {
> + Symbol sym;
> long hashValue = hashSymbol(name);
> for (HashtableEntry e = (HashtableEntry) bucket(hashToIndex(hashValue));
> e != null; e = (HashtableEntry) e.next()) {
> if (e.hash() == hashValue) {
> - Symbol sym = Symbol.create(e.literalValue());
> + sym = Symbol.create(e.literalValue());
> if (sym.equals(name)) {
> return sym;
> }
> }
> }
> - return null;
> +
> + sym = sharedTable.probe(name, hashValue);
> + return sym;
> }
>
>
> You may also want to update the copyright comments.
Yep, will do. David also sent me an offline reminder for the copyright headers.
Thanks!
Jiangli
>
>
> Thanks,
> Serguei
>
>
> On 1/29/15 5:46 PM, Jiangli Zhou wrote:
>> Hi Serguei,
>>
>> Thanks for noticing that. It’s actually a duplicated bug ID for the same
>> issue. I just fixed the web rev:
>> http://cr.openjdk.java.net/~jiangli/8071962/webrev.00/.
>>
>> Thanks,
>> Jiangli
>>
>> On Jan 29, 2015, at 5:40 PM, serguei.spit...@oracle.com wrote:
>>
>>> On 1/29/15 5:13 PM, Jiangli Zhou wrote:
>>>> Hi,
>>>>
>>>> Please review the change for JDK-8071962, "The SA code needs to be updated
>>>> to support Symbol lookup from the shared archive”.
>>>>
>>>> In JDK-8059510, we introduced a compact form of hash table for the symbols
>>>> in shared archive. The shared symbols are stored separately from the
>>>> regular symbol table. The VM looks up both tables when searching for
>>>> existing symbol at runtime. The SA code needs to do the same when looking
>>>> up symbols.
>>>>
>>>> Webrev: http://cr.openjdk.java.net/~jiangli/8067977/webrev.00/
>>>
>>> Jiangli,
>>>
>>> I'm not reviewing, just wanted to make sure there is no typo here ...
>>> The webrev bug number is 8067977, but the RFR bug number is 8071962 which
>>> is strange.
>>>
>>> Thanks,
>>> Serguei
>>>
>>>>
>>>> Tested on both 32-bit and 64-bit platforms:
>>>> bin/java sun.jvm.hotspot.tools.DumpJFR <pid>
>>>>
>>>> JPRT tests in progress.
>>>>
>>>> Thanks,
>>>> Jiangli
>>>>
>>>>
>>>
>>
>
