RFR (S) 8014262 - more footprint info for PrintStringTableStatistics

[Rickard Bäckman]

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?

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).

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
>> 
>