RFR (S) 8211239: Build fails without JFR: empty JFR events signatures mismatch

[David Holmes]

Trying to gather my thoughts ...

On 1/10/2018 7:45 PM, David Holmes wrote:
> On 1/10/2018 6:05 PM, Aleksey Shipilev wrote:
>> On 10/01/2018 09:49 AM, David Holmes wrote:
>>>> Any other Reviewers? Would like to unbreak jdk/jdk :)
>>>
>>> Isn't printEmptyType redundant with this change?
>>
>> Right! Removed here:
>>    http://cr.openjdk.java.net/~shade/8211239/webrev.03/
>>
>>> 480             out.write("template <typename T>");
>>>
>>> Why is this now a template class? I don't see T being used.
>>
>> "Empty" JfrEvent now matches "full" JfrEvent from jfrEvent.hpp:
>>
>> template <typename T>
>> class JfrEvent {
>>   private:
>>    jlong _start_time;
>>    jlong _end_time;
>>    bool _started;
>>    ...
> 
> I'm confused. You stated in the RFR:
> 
> "JFR events generator produces two versions for each event: "full" and 
> "empty", and the version is then selected by INCLUDE_JFR macro during 
> build time."
> 
> but really it only generates one version and #includes the .hpp with the 
> "real version".

Or is it that the top-level JfrEvent exists in the .hpp but otherwise 
all the subclasses are generated. If so I'd probably find it much 
clearer if the actual jfrEvent.hpp file contained both versions and the 
appropriate ifdefs. At least that way the two versions are in the same 
file and there is less likelihood of them getting out of sync.

David

> I'm at a loss to understand why it is done this way as the two versions 
> are not connected in any way and so rely on manual intervention to be 
> kept in sync AFAICS.
> 
>> The "empty" subclasses use the template parameter here:
>>
>>    out.write("class Event" + event.name + " : public JfrEvent<Event" + 
>> event.name + ">");
>>
>> ...and here:
>>
>>    out.write("  using JfrEvent<Event" + event.name + ">::commit; // 
>> else commit() is hidden by
>> overloaded versions in this class");
> 
> I see. This is very difficult to follow.
> 
>>
>>> The refactoring to handle the "empty" case within the same logic 
>>> seems reasonable, though it's not
>>> immediately obvious to me where the signature mismatch was introduced 
>>> previously or how the refactor
>>> prevents that ?
>>
>> I think the mismatch between full and empty events was always there, I 
>> see it up to the original
>> commit of JFR event generator. But it was not observed until we 
>> started to use the multi-parameter
>> commit() methods in JDK-8196341: 
>> http://hg.openjdk.java.net/jdk/jdk/rev/5f931e3e7a63 -- when it
>> broke the non-JFR build. The surprises like that is why I think we are 
>> better generating the same
>> methods with/without body on full/empty event paths.
> 
> This seems error prone to me. I'm guessing we keep far more "stubbed 
> out" JFR code in a non-JFR build than might naively be assumed.
> 
> Thanks,
> David
> 
>> Thanks,
>> -Aleksey
>>