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

[gerard ziemski]

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