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

gerard ziemski gerard.ziemski at oracle.com
Fri Apr 5 15:42:27 UTC 2019 

Thank you Erik for more suggestions.

I have implemented them, which you can find here 
http://cr.openjdk.java.net/~gziemski/8185525_rev3

I will start Mach5 tier1-6 test to test the changes shortly


On 4/4/19 4:11 PM, Erik Gahlin wrote:
> Thanks for fixing.
>
> A quick comments about the test.
>
> I think it can be simplified by using some of the test library 
> functionality, i.e
>
>   public static void main(String[] args) throws Throwable {
>     try (Recording recording = new Recording()) {
>       recording.enable(EventNames.SymbolTableStatistics);
>       recording.enable(EventNames.StringTableStatistics);
>       recording.enable(EventNames.PlaceholderTableStatistics);
> recording.enable(EventNames.LoaderConstraintsTableStatistics);
> recording.enable(EventNames.ProtectionDomainCacheTableStatistics);
>       recording.start();
>       recording.stop();
>
>       List<RecordedEvent> events = Events.fromRecording(recording);
>       verifyTable(events, EventNames.SymbolTableStatistics);
>       verifyTable(events, EventNames.StringTableStatistics);
>       verifyTable(events, EventNames.PlaceholderTableStatistics);
>       verifyTable(events, EventNames.LoaderConstraintsTableStatistics);
>       verifyTable(events, 
> EventNames.ProtectionDomainCacheTableStatistics);
>     }
>   }
>
>   private static void verifyTable(List<RecordedEvent> allEvents, 
> String eventName) throws Exception {
>     List<RecordedEvent> eventsForTable = allEvents.stream()
>                                                   .filter(e -> 
> e.getEventType().getName().equals(eventName))
> .collect(Collectors.toList());
>      if (eventsForTable.isEmpty()) {
>        throw new Exception("No events for " + eventName);
>      }
>      for (RecordedEvent event : eventsForTable) {
>        Events.assertField(event, "bucketCount").atLeast(0L);
>        long entryCount = Events.assertField(event, 
> "entryCount").atLeast(0L).getValue();
>        Events.assertField(event, "totalFootprint").atLeast(0L);
>        long averageBucketCount = Events.assertField(event, 
> "averageBucketCount").atLeast(0L).getValue();
>        Events.assertField(event, 
> "maximumBucketCount").atLeast(averageBucketCount);
>        Events.assertField(event, "bucketCountVariance").atLeast(0.0f);
>        Events.assertField(event, 
> "bucketCountStandardDeviation").atLeast(0.0f);
>        float insertionRate = Events.assertField(event, 
> "insertionRate").atLeast(0.0f).getValue();
>        float removalRate = Events.assertField(event, 
> "removalRate").atLeast(0.0f).getValue();
>        if ((insertionRate > 0.0f) && (insertionRate > removalRate)) {
>          Asserts.assertGreaterThan(entryCount, 0L, "Entries marked as 
> added, but no entries found for " + eventName);
>        }
>     }
>   }
>
> - It's nice to have the main method on top so you can easily see what 
> the test is supposed to do.
> - Changed (some) field names that used the previous naming style.
> - Reduced the number of methods to make it easier to read
> - Reduced number of calls to Events.fromRecording(...) as will 
> repeatedly dump a file to disk.
> - Used Events.assertField() which will provide better error message if 
> an assertion fails,
> - Used EventType::getName instead of event.toString() contains
> - Added sanity checks for standard deviation and variance fields
> - Wrapped Recording creation in try-with-resource to avoid warning 
> about resource leak
> - Removed threshold as the events are periodic and don't use a threshold
> - Removed "Thread.sleep"
> - The test now relies on events having period "everyChunk" which means 
> at least two events per recording are guaranteed
>
> Could you explain how the string table test work, and why it needs 
> special handling?
Some tables might have zero entries, so I just wanted to be clear that 
for StringTable (which will always have some entries) we do something 
that will make it grow. I was planning on doing more elaborate tests, 
but I don't think it's necessary.


>
> I also missed changes to the file EventNames.java
>
> (I haven't actually tried the code, but you get the idea)

I fixed it up just a bit, and it works nice, thanks!


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

More information about the jmc-dev mailing list