RFR (S) 8014262 - more footprint info for PrintStringTableStatistics
Coleen Phillimore
coleen.phillimore at oracle.com
Thu May 16 04:12:19 PDT 2013
This looks good to me with a couple of minor comments:
+ double bucket_avg = num_buckets <= 0 ? 0 : bucket_bytes / num_buckets;
+ double entry_avg = num_entries <= 0 ? 0 : entry_bytes / num_entries;
+ double literal_avg = num_entries <= 0 ? 0 : literal_bytes / num_entries;
+
Parens would make this easier to read so we don't have to think about
the order of evaluation of ?:. And you might want to add an assert that
num_buckets and num_entries aren't zero.
+ st->print_cr("Total footprint : %9s = %9d bytes", "", total_bytes);
This is a bit odd with the null string. These are minor comments and I
don't need another review if you decide to fix them, the rest looks great.
thanks,
Coleen
On 5/16/2013 12:49 AM, Rickard Bäckman wrote:
> Ioi,
>
> thanks for the explanation. Looks good!
>
> /R
>
> On May 15, 2013, at 11:08 PM, Ioi Lam wrote:
>
>> Hi Rickard,
>>
>> Thanks for the review. I have updated the webrev according to the feedbacks:
>>
>> http://cr.openjdk.java.net/~iklam/8014262/table_stats_002/
>>
>> Please see moew responses below.
>>
>> On 05/14/2013 10:21 PM, Rickard Bäckman wrote:
>>> Ioi,
>>>
>>> I think the change looks good (not a Reviewer) and would be fine with the change as is, just a few comments.
>>>
>>> The Hashtable<T, F> *table argument in the static method Hashtable::dump() seems to be more or less a this *. Convert to a non-static?
>>>
>> I have changed it to a non-static method as you suggested. Also renamed it to Hashtable::dump_table() since g++ has trouble with finding overloaded methods in templates super-classes.
>>
>>>
>>> static int literal_size(ConstantPool *cp) {Unimplemented(); return 0;} // currently
>>> static int literal_size(Klass *k) {Unimplemented(); return 0;} // currently
>>>
>>> should probably be removed until implemented (compile time error vs runtime error).
>>>
>> For Mac and Solaris C++ compilers, Hashtable::dump_table() is instantiated for these two classes (at the bottom of hashtable.cpp) even though we never call the dump_table method for these two classes:
>>
>> // Explicitly instantiate these types
>> template class Hashtable<ConstantPool*, mtClass>;
>> template class Hashtable<Klass*, mtClass>;
>>
>> So I needed to add those two literal_size() methods. Otherwise there would be a linker error. These aren't needed for other platforms. I added comments in hashtable.hpp to reflect this.
>>
>> Thanks
>> - Ioi
>>> Thanks
>>> /R
>>>
>>> On May 14, 2013, at 6:37 PM, Ioi Lam wrote:
>>>
>>>
>>>> Still looking for a reviewer. Advice on C++/template coding style below would be really appreciated!
>>>>
>>>> Thanks
>>>> - Ioi
>>>>
>>>> On 05/09/2013 11:52 AM, Ioi Lam wrote:
>>>>
>>>>> Please review:
>>>>>
>>>>>
>>>>> http://cr.openjdk.java.net/~iklam/8014262/table_stats_001/
>>>>>
>>>>>
>>>>>
>>>>> Bug: PrintStringTableStatistics should include more footprint info
>>>>>
>>>>>
>>>>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8014262
>>>>>
>>>>>
>>>>> https://jbs.oracle.com/bugs/browse/JDK-8014262
>>>>>
>>>>>
>>>>>
>>>>> Summary of fix:
>>>>>
>>>>> StringTable and SymbolTable take up significant footprint (about
>>>>> 4.5% of total footprint of Eclipse). The old output doesn't has very
>>>>> little info
>>>>>
>>>>> ---OLD---
>>>>> Number of buckets : 20011
>>>>> Average bucket size : 8
>>>>> Variance of bucket size : 8
>>>>> Std. dev. of bucket size: 3
>>>>> Maximum bucket size : 25
>>>>> StringTable statistics:
>>>>> Number of buckets : 60013
>>>>> Average bucket size : 0
>>>>> Variance of bucket size : 0
>>>>> Std. dev. of bucket size: 1
>>>>> Maximum bucket size : 6
>>>>>
>>>>>
>>>>> I modified it to print these:
>>>>>
>>>>> ---NEW---
>>>>> SymbolTable statistics:
>>>>> Number of buckets : 20011 = 160088 bytes, avg 8.00
>>>>> Number of entries : 166724 = 4001376 bytes, avg 24.00
>>>>> Number of literals : 166724 = 7943576 bytes, avg 47.65
>>>>> Total footprint : = 12105040 bytes
>>>>> Average bucket size : 8.332
>>>>> Variance of bucket size : 8.308
>>>>> Std. dev. of bucket size: 2.882
>>>>> Maximum bucket size : 25
>>>>> StringTable statistics:
>>>>> Number of buckets : 60013 = 480104 bytes, avg 8.00
>>>>> Number of entries : 25822 = 619728 bytes, avg 24.00
>>>>> Number of literals : 25822 = 2458696 bytes, avg 95.22
>>>>> Total footprint : = 3558528 bytes
>>>>> Average bucket size : 0.430
>>>>> Variance of bucket size : 0.435
>>>>> Std. dev. of bucket size: 0.659
>>>>> Maximum bucket size : 6
>>>>>
>>>>>
>>>>> Templave vs virtual function:
>>>>>
>>>>> Part of this code is kind of ugly, in hashtable.cpp:
>>>>>
>>>>> ...void Hashtable<T, F>::dump( ...) {
>>>>> ...
>>>>> for (HashtableEntry<T, F>* e = table->bucket(i);
>>>>> 264 e != NULL; e = e->next()) {
>>>>> 265 count++;
>>>>> 266 literal_bytes += literal_size(e->literal());
>>>>> 267 }
>>>>>
>>>>>
>>>>> literal_size() is magically matched by the templates to call one of these
>>>>> in hashtable.hpp:
>>>>>
>>>>> template <class T, MEMFLAGS F> class Hashtable : public BasicHashtable<F> {
>>>>> ...
>>>>> 286 static int literal_size(Symbol *symbol);
>>>>> 287 static int literal_size(oop oop);
>>>>> 288 static int literal_size(ConstantPool *cp) {Unimplemented(); return 0;} // currently not used
>>>>> 289 static int literal_size(Klass *k) {Unimplemented(); return 0;} // currently not used
>>>>>
>>>>>
>>>>> I am wondering if I should change this to be a virtual function instead.
>>>>> However, none of the current hashtable code uses virtual functions. So it
>>>>> is considered OK to have a virtual function like this:
>>>>>
>>>>> template <class T, MEMFLAGS F> class Hashtable : public BasicHashtable<F> {
>>>>> public:
>>>>> virtual int literal_size(T) {return 0;}
>>>>> ...
>>>>> }
>>>>>
>>>>> class SymbolTable : public Hashtable<Symbol*, mtSymbol> {
>>>>> public:
>>>>> virtual int literal_size(Symbol* sym) {return sym->size() * HeapWordSize;}
>>>>> ...
>>>>> }
>>>>>
>>>>> Any suggestions?
>>>>>
>>>>> Tests:
>>>>>
>>>>> This code path is not taken in regular JVM executions, so
>>>>> I just ran JPRT to make sure the code builds on all platforms.
>>>>>
>>>>> Tested manually on Linux and Solaris:
>>>>> $ java -XX:+PrintStringTableStatistics -version
>>>>>
>>>>>
>>>>> Thanks
>>>>> - Ioi
>>>>>
>>>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/attachments/20130516/6c2d9465/attachment-0001.html
More information about the hotspot-runtime-dev
mailing list