RFR JDK-8059510 Compact symbol table layout inside shared archive

[Jiangli Zhou]

Hi Gerard,

On 10/10/2014 01:44 PM, Gerard Ziemski wrote:
> hi Jiangli,
>
> On 10/10/2014 3:10 PM, Jiangli Zhou wrote:
>> Hi Gerard,
>>
>> On 10/10/2014 08:12 AM, Gerard Ziemski wrote:
>>> hi Jiangli,
>>>
>>> On 10/9/2014 2:11 PM, Jiangli Zhou wrote:
>>>> Hi Gerard,
>>>>
>>>> Thank you very much for the review. Please see my comments below.
>>>>
>>>> On 10/09/2014 08:04 AM, Gerard Ziemski wrote:
>>>>> hi Jiangli,
>>>>>
>>>>> I'm a reviewer with small "r" and I'm still going through your 
>>>>> code and learning as I go, but so far I have 2 items as my 
>>>>> feedback/questions:
>>>>>
>>>>> #1 Re: "SymbolTable::lookup”
>>>>>
>>>>>  Symbol* SymbolTable::lookup(int index, const char* name,
>>>>>                                int len, unsigned int hash) {
>>>>> +  Symbol* s = _shared_table.lookup(name, hash, len);
>>>>> +  if (s != NULL) {
>>>>> +    return s;
>>>>> +  }
>>>>> +
>>>>>    int count = 0;
>>>>>    for (HashtableEntry<Symbol*, mtSymbol>* e = bucket(index); e != 
>>>>> NULL; e = e->next()) {
>>>>>      count++;  // count all entries in this bucket, not just ones 
>>>>> with same hash
>>>>>      if (e->hash() == hash) {
>>>>>        Symbol* sym = e->literal();
>>>>>
>>>>> a) Do we need to evaluate the lookup time performance, now that 
>>>>> some entries will have to be looked up in 2 separate tables in 
>>>>> "SymbolTable::lookup"?
>>>>>
>>>>> b) Shared table is being looked at 1st, is this the case we expect?
>>>>
>>>> Those are very good questions. The shared symbol table lookup are 
>>>> fast since we can very efficiently locate the specific bucket with 
>>>> pre-calculated bucket sizes. The shared table is searched first 
>>>> because the symbols contained in that are from archived classes, 
>>>> which are the ones used during bootstrap (by default). Separating 
>>>> the symbols into two sets do introducing some overhead. In this 
>>>> case, I think the effect is negligible.  The data from Aleksey's 
>>>> benchmark for classloading showed very small difference between the 
>>>> patched and non-patched version.
>>>
>>> You might be very well right that the performance hit is negligible, 
>>> but my point is that you haven't shown that this issue isn't a 
>>> problem by backing it up with actual performance data. You use 
>>> Aleksey's own benchmark to prove your point, which only came up 
>>> during the review and which actually shows the opposite (though only 
>>> a slight regression). I would think that we need real performance 
>>> data that will prove your assumptions without any doubt.
>>
>> You have a very good point. I apologize for not providing my 
>> first-hand benchmark data. Here are some classloading benchmark 
>> results on linux-i586 and linux-arm (soft-float vfp) platforms. 17436 
>> classes were loaded from bootclasspath. For both before and after, 
>> the shared archive were used. 10 samples were collected for both 
>> before and after.
>>
>> *Linux ARMv7 tegra board*
>> Before(average): 7.9505s
>> After(average)   :  7.8601s
>>
>> *Linux Intel i5*
>> Before(average): 1.2162s
>> After(average)   : 1.1457s
>
> This looks promising, but it also looks like a specialized benchmark 
> designed to test shared archive behavior. Do we have performance 
> regressions numbers from standard benchmarks (ie. refworkload) that do 
> not use shared archive path?

The test used was designed for benchmarking classloading speed, not 
specifically for testing shared archive behavior. Shared archive was 
used for both before and after because the shared symbol table would 
only be used in that case. The potential performance impact of looking 
up the shared symbol table would only manifest in that case. When class 
data sharing is not enabled, the shared symbol table is not used at all.

I'll run specjvm with reworkload.

Thanks,
Jiangli

>
> cheers
>