RFR: 8185525: [Event Request] Add Tracing event for DictionarySizes

Fri Apr 5 12:35:04 UTC 2019

Hi Gerard,   This is somewhat of a first pass review.

I like the change a  lot.  I have a couple of suggestions.

http://cr.openjdk.java.net/~gziemski/8185525_rev2/src/hotspot/share/utilities/statistics.hpp.html

Can you rename this file tableStatistics.cpp/hpp because "statistics" is 
too general and the class is called TableStatistics.

http://cr.openjdk.java.net/~gziemski/8185525_rev2/src/hotspot/share/jfr/periodic/jfrPeriodic.cpp.udiff.html

Is there anyway to parameterize these functions and/or add them to 
TableStatistics?

Also, when Stefan is done with the ResolvedMethodTable, you can add that 
too in a separate RFE https://bugs.openjdk.java.net/browse/JDK-8221393

Thanks,
Coleen

On 4/4/19 3:52 PM, gerard ziemski wrote:
> Thank you Erik for clarifications.
>
> I have implemented all your suggestions, which you can find here 
> http://cr.openjdk.java.net/~gziemski/8185525_rev2
>
> I started Mach5 tier1-6 test to test the changes ...
>
>
> cheers
>
> On 4/4/19 1:16 PM, Erik Gahlin wrote:
>> On 2019-04-04 17:39, gerard ziemski wrote:
>>> hi Erik,
>>>
>>>
>>> On 4/3/19 12:44 PM, Erik Gahlin wrote:
>>>> Hi Gerard,
>>>>
>>>> Here are some comments about the metadata (to make it consistent 
>>>> with other events).
>>>>
>>>> The events should not be in the "Java Application" category since 
>>>> they are JVM events. You could perhaps put them in "Java Virtual 
>>>> Machine, Runtime, Tables". Some comments about the names and labels 
>>>> of fields.
>>>>
>>>> - Label: Number of buckets => Bucket Count
>>>> - Label: Number of entries => Entry Count
>>>> - Label: Total footprint => Total Footprint
>>>>
>>>> Could you remove descriptions that are exactly the same as the label.
>>>>
>>>> - Label: Maximum bucket size => Maximum Bucket Size
>>>> - Label: Average bucket size => Average Bucket Size
>>>> - Label: Variance of bucket  size => Bucket Size Variance
>>>> - Name: stdDevOfBucketSize => bucketSizeStandardDeviation
>>>> - Label: Standard deviation of bucket size => Bucket Size Standard 
>>>> Deviation"
>>>>
>>>> Instead of using the word "size", it may make more sense to use the 
>>>> word "count" here as well, i.e "Average Bucket Count", or maybe I'm 
>>>> missing something? Is there a difference?
>>>>
>>>> I wonder how useful standard deviation and variance is? If support 
>>>> engineers are looking at a recording, or JMC adds a rule for the 
>>>> events, what would a good or bad value be? Is it possible to use 
>>>> the information for troubleshooting?
>>>
>>> While I'm working on all the above changes you suggested, we can 
>>> discuss the standard devation and variance.
>>>
>>> I added them because they are part of the jcmd "VM.symboltable 
>>> -verbose" command, so we are consistent.
>> OK
>>>
>>> Now, regarding how useful they are, I always understood them as a 
>>> sign of imbalanced table distribution, and without a proper 
>>> histogram, this is the best description of the histogram shape. In 
>>> reality, however, I think that if they identify an issue, then we 
>>> might have a very curious distribution (some sort of hash table 
>>> attack), or we have an issue with our hash function for the 
>>> particular usage case.
>>>
>>> Still, I'd personally elect to keep them.
>>>
>>> Let me ask you a different question though, Is it expensive to have 
>>> 2 doubles as part of an event (5 events per second)? 
>> Doubles can't be compressed so each value will take 8 bytes. I don't 
>> think the precision of a double is needed, so you could change it 
>> into a float and save a few bytes.
>>
>> Most user will not care about JVM internals and a lower rate than 
>> once per second is probably sufficient for support engineers to spot 
>> that something is wrong.
>>
>> The Thread Context Switch Rate event is emitted once every ten 
>> seconds. I think the same rate could be used here.
>>
>>> And if so, is there currently (or planned) granularity for 
>>> controlling not just which events to record, but also which attributes?
>>>
>> No.
>>
>> If overhead becomes an issues, it's usually better to emit all the 
>> information, but at a lower rate.  That way, users can find out that 
>> the information exists, and increase the rate if a higher resolution 
>> is needed to solve their specific issue.
>>
>>>>
>>>> - Name: addRate => insertionRate
>>>> - Label: Rate of addition =>  Insertation Rate
>>>> - Name: removeRate => removalRate
>>>> - Label: Rate of removal => Removal Rate
>>>
>>> Will do.
>>>
>>>>
>>>> I'm missing unit tests for the events. Could you please add in 
>>>> /test/jdk/jdk/jfr/event/runtime. They can be sanity tests. i.e the 
>>>> average not exceeding max, no negative values etc.
>>>
>>> Working on it, do we need separate test per each event (table), or 
>>> just one table will suffice (ex. StringTable)?
>> They are kind of similar, so I think one test file is sufficient, but 
>> we should sanity check data for all events.
>>
>> Thanks
>> Erik
>>
>>>
>>> Thank you for the feedback!
>>>
>>>
>>> cheers
>>>>
>>>> Thanks!
>>>> Erik
>>>>
>>>>> Hi all,
>>>>>
>>>>> Please review this feature, which adds tracing events for the 
>>>>> internal hash tables.
>>>>>
>>>>> The following attributes are implemented:
>>>>>
>>>>> <Field type="ulong" name="numberOfBuckets" label="Number of 
>>>>> buckets" description="Number of buckets" />
>>>>> <Field type="ulong" name="numberOfEntries" label="Number of 
>>>>> entries" description="Number of all entries" />
>>>>> <Field type="ulong" contentType="bytes" name="totalFootprint" 
>>>>> label="Total footprint" description="Total memory footprint (the 
>>>>> table itself plus all of the entries)" />
>>>>> <Field type="ulong" name="maximumBucketSize" label="Maximum bucket 
>>>>> size" description="The maximum bucket length (entries in a single 
>>>>> bucket)" />
>>>>> <Field type="double" name="averageBucketSize" label="Average 
>>>>> bucket size" description="The average bucket length (entries in a 
>>>>> bucket)" /> <Field type="double" name="varianceOfBucketSize" 
>>>>> label="Variance of bucket sizes" description="How far bucket 
>>>>> lengths are spread out from their average value" />
>>>>> <Field type="double" name="stdDevOfBucketSize" label="Standard 
>>>>> deviation of bucket sizes" description="How far bucket lengths are 
>>>>> spread out from their mean (expected) value" />
>>>>> <Field type="double" name="addRate" label="Rate of addition" 
>>>>> description="How many items were added since last event (per 
>>>>> second)" />
>>>>> <Field type="double" name="removeRate" label="Rate of removal" 
>>>>> description="How many items were removed since last event (per 
>>>>> second)" />
>>>>>
>>>>> This event was implemented for the following system tables:
>>>>>
>>>>> SymbolTable
>>>>> StringTable
>>>>> Placeholder Table
>>>>> LoaderConstraints Table
>>>>> ProtectionDomainCache Table
>>>>>
>>>>> Webrev: http://cr.openjdk.java.net/~gziemski/8185525_rev1/
>>>>> Bug:     https://bugs.openjdk.java.net/browse/JDK-8185525
>>>>> Testing: Mach5 tier1,2,3 (another Mach5 tier1,2,3,4,5,6,7 in 
>>>>> progress…)
>>>>>
>>>>>
>>>>> Cheers