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

[Erik Gahlin]

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?

I also missed changes to the file EventNames.java

(I haven't actually tried the code, but you get the idea)

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