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

Fri Apr 5 15:30:23 UTC 2019

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.

>
> 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.

>
> 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
>
>
> 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
>
>