RFR (S) 8014262 - more footprint info for PrintStringTableStatistics

[Rickard Bäckman]

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