RFR (S) 8014262 - more footprint info for PrintStringTableStatistics
Ioi Lam
ioi.lam at oracle.com
Thu May 16 15:51:24 PDT 2013
On 05/16/2013 04:12 AM, Coleen Phillimore wrote:
>
> 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.
I will add in the parens. I'll probably skip the assert just in case the
VM may quit before loading any classes (e.g., with bad rt.jar file) and
your string table might be empty.
>
> + 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.
>
I did this so that the C source code lines up nicely with the %9d in the
previous line :-)
Thanks
- Ioi
> 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/9b35db1c/attachment.html
More information about the hotspot-runtime-dev
mailing list