RFR (S) 8014262 - more footprint info for PrintStringTableStatistics

[Ioi Lam]

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/ 
<http://cr.openjdk.java.net/%7Eiklam/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/20130515/c4536701/attachment-0001.html