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

Tue Apr 9 14:43:33 UTC 2019

On 4/5/19 11:30 AM, gerard ziemski wrote:
> hi Coleen,
>
> Thank you for the review. My comments are inline below:
>
> On 4/5/19 7:35 AM, coleen.phillimore at oracle.com wrote:
>>
>> 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.
> I deliberately named the file "statistics.hpp", because I assume we 
> will be adding more JFR events in the future, and this file could hold 
> all the related code, which for now just comprises of table statistics 
> as you pointed out.

Hi I don't agree with that.  I think if you want more different JFR 
statistics you could add files where they belong as differentStatistics.hpp

>
>
>>
>> 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?
> I didn't want to add JFR dependency to TableStatistics. I'm unsure 
> what I can do more here, and whether it deserves the effort - 
> TableStatistics basically serves as a struct for passing event 
> attributes around, but I'm open to suggestions.
>

I didn't think this should be moved from jfrPeriodic.cpp.  I thought it 
could be something like an X macro.

Or just make this bit a function that they all call with event as parameter.

+ event.set_numberOfBuckets(statistics._number_of_buckets);
+ event.set_numberOfEntries(statistics._number_of_entries);
+ event.set_totalFootprint(statistics._total_footprint);
+ event.set_maximumBucketCount(statistics._maximum_bucket_size);
+ event.set_averageBucketCount(statistics._average_bucket_size);
+ event.set_varianceOfBucketCount(statistics._variance_of_bucket_size);
+ event.set_stdDevOfBucketCount(statistics._stddev_of_bucket_size);
+ event.set_insertionRate(statistics._add_rate);
+ event.set_removalRate(statistics._remove_rate);
+ event.commit();

>
>>
>> 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
> Thank you, I linked them.

Thanks!
Coleen
>
>
>>
>> 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
>>
>>
>