Thank you, Yumin. I incorporated your following suggestions with slight modification for the comment change.
Thanks again, Jiangli On Jan 30, 2015, at 2:55 PM, Yumin Qi <yumin...@oracle.com> wrote: > Jiangli, > > Two minor suggestions (did not pay attention in first round, anyway, no > harm): > > 1) CompactHashtable.java: > > 58 private static long uintxSize; > > I did not find the usage of this var, please remove it. > > 2) > + anyway. Searches the regular symbol table and the shared symbol > + table. Null is returned if the given string is not in the symbol > + tables. */ > > Maybe better with: "Return null if the given name is not found in both > tables." ? > > No need to send review again if you change. > > Thanks > Yumin > > > On 1/30/2015 2:31 PM, Jiangli Zhou wrote: >> Here is the updated webrev that incorporates all feedbacks: >> >> http://cr.openjdk.java.net/~jiangli/8071962/webrev.01/ >> >> Thanks, >> Jiangli >> >> On Jan 30, 2015, at 10:05 AM, Jiangli Zhou <jiangli.z...@oracle.com> wrote: >> >>> Hi Ioi, >>> >>> Thanks for the review. >>> >>> On Jan 30, 2015, at 9:49 AM, Ioi Lam <ioi....@oracle.com> wrote: >>> >>>> Hi Jiangli, >>>> >>>> The code looks good. I am wondering if you need a more specific JTREG test >>>> for the new sun/jvm/hotspot/utilities/CompactHashTable.java. There's a lot >>>> of code in CompactHashTable.java. Will the existing test case in 8071962 >>>> provide enough coverage? >>> My thinking would be yes. The symbol lookup is very foundational operations >>> in sun.jvm.hotspot.tools.DumpJFR. If it fails, it would get exception >>> immediately. We also have JTREG tests that uncovers the SA symbol lookup >>> issue. >>> >>> Thanks, >>> Jiangli >>> >>>> Thanks >>>> - Ioi >>>> >>>> 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 >>>>>>> >>>>>>> >
