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

[gerard ziemski]

Sorry Erik that it took me a while to respond, but the thread got so 
long, that I missed your reply initially.


On 4/16/19 10:05 PM, Erik Gahlin wrote:
>>>>>>
>>>>>> If the lock is taken, then it means that someone is scanning 
>>>>>> through the entire table, or the table is being resized. Either 
>>>>>> way, we're not loosing data, but are just temporarily blind - I 
>>>>>> don't see a problem here for a long running apps, they will start 
>>>>>> receiving events eventually (which happen every 10 sec by default)
>>> A user can set period "everyChunk" which means events are guaranteed 
>>> to be in the recording.
>>>
>>> I think we should try to avoid breaking that contract. When event 
>>> streaming is in place, we can implement requestable events where a 
>>> user can demand an event programmatically from Java. If they 
>>> sometimes don't get an event, it will break their code in a subtle way.
>>
>> No problem, I removed the resize_lock around the JFR table 
>> statistics, so we might get a slightly incorrect stats every now and 
>> then, but we will be emitting the events on schedule: 
>> http://cr.openjdk.java.net/~gziemski/8185525_rev7
> Is it sufficient to just remove the lock to make it "work"?

Yes, the event statistics might be slightly off though.


>
> I think it could be OK to use stale data, or perhaps count a value 
> twice, but are there other issues that needs to be fixed as well? 
> Robbin may have more information on this.

Yes, we might end up miscounting some items, as Coleen pointed out 
before. I already noted that emitting the event in such situation might 
result is slightly wrong data, and used that as an argument that I'd 
prefer not to emit the event at all, but you said that you preferred 
slightly wrong data as long as we emit the event.

I don't want to speak for Robbin here, but I want to note that he 
already expressed his opinion, by saying that we might as well skip the 
event, when we can't grab the lock, which would me my personal choice 
here as well.


>
> An alternative approach would be to use the last known data, if we are 
> not able to take the lock. It would be old, but not out of whack.

This is not really much better than not emitting the event at all, as we 
had in previous implementation. Any client reading the events might as 
well assume that the missing event would be same as the last for this 
particular event and synthesize one as needed. I don't see this as much 
improvement.

>
> That said, it would be interesting to have some numbers on what the 
> cost would be to wait for the lock.

Robbin's hash table is concurrent and I personally would hate to 
introduce, even in JFR code base, a mechanism that blocks and waits 
around for the table to be locked (however infrequent such situation 
might be called for).

I'm not saying that it can not be done, but I personally would not want 
to do this. If you want to follow up on this yourself, however, you can 
always do that.


>
>>
>> Last question: what is the recommended way to programatically tell if 
>> JFR is ON? I'm wondering whether I should collect the add/remove 
>> rates for the tables only if JRF is ON. As it is right now, we 
>> collect them always. It's just an atomic increment, but still, it's 
>> work only JFR events need.
>
> You can use the JFR_ONLY macro, if it's not built with JFR. If you 
> want to check if a recording is running, you can use 
> Jfr::is_recording(), but perhaps Jfr::is_enabled() is more 
> accurate/correct if a recording is started/stopped repeatedly?'

Thanks! I used some of these APIs, but I think that we currently don't 
have enough granularity here, so I filed JDK-8222736 expressing my 
concerns and the logic behind it.


>
> I looked at jfrPeriodic.cpp, and it seems to me that things could be 
> simplified, i.e.
>
> template<typename T>
> static void emit_table_statistics(TableStatistics& statistics) {
>    T event;
>    event.set_bucketCount(statistics._number_of_buckets);
>    ...
>    event.commit();
> }

Very nice suggestion, thank you.

As much as I'd like to move on from this issue, I'm having second 
thoughts about the "insertion rate" and "deletion rate" attributes for 
these events. They can be synthesized by clients, and I wonder whether 
or not we should include them. I'd rather see only the core attributes 
be part of the event, and anything else that can be synthesized by 
clients, not "clutter" the event.


Uploaded update revision here 
http://cr.openjdk.java.net/~gziemski/8185525_rev8


cheers