Re: [8u] RFR 8225797: [backport] OldObjectSample event creates unexpected amount of checkpoint data

[Denghui Dong]

Hi Mario and Andrew,

Sorry for the late reply, and thank you very much for the comments.

I will file a new bug to clean the useless code about PackageEntry and ModuleEntry first,
and then refine this patch.

Thanks,
Denghui Dong


------------------------------------------------------------------
From:Andrew Dinn <adinn at redhat.com>
Send Time:2020年5月28日(星期四) 18:06
To:Mario Torre <neugens at redhat.com>; 董登辉(卓昂) <denghui.ddh at alibaba-inc.com>; jdk8u-dev <jdk8u-dev at openjdk.java.net>
Subject:Re: [8u] RFR 8225797: [backport] OldObjectSample event creates unexpected amount of checkpoint data

Hi Denghui,

On 13/05/2020 12:07, Mario Torre wrote:
> On Wed, 2020-05-13 at 09:56 +0800, Denghui Dong wrote:
>> Gentle ping?
>>
> 
> Hi Denghui,
> 
> Sorry, the patch is really big so it's taking some time, but I asked
> some help ;)

This is a very complex patch which has been very hard to review. The
difficulty is not just the obvious problem of the complexity of the code
under modification. It is compounded by the fact that the diff algorithm
used to construct the patch files has selected different 'match lines'
when distinguishing the start and extent of jdk8u differences from those
used to establish the start and extent of jdk11u differences.

So, although the two patches mostly contain the same additions in the
same order and the same deletions in the same order these additions and
deletions are interleaved differently from one patch file to the other.
That has made it very hard to subtract apparent changes which in reality
turn out to be no-ops from the changes actually needed to make the
jdk11u fix work on jdk8u.

Also, retention/update of all the commented out code relating to modules
and packages has not helped. I agree with Mario that this commented out
code is best deleted (and arguably should have been in the initial
backport). Retaining commented out code in a downstream repo in order to
mark omissions from the upstream version is always going to be a losing
game. It simply introduces noise and code/comment-rot into the process.
It is much clearer and simpler to have no module-related code in the
jdk8u repo and not have to review edits to commented out module code in
backport patches.

Arguably this problem could be handled as a separate issue -- there may
well be commented out code blocks that are not updated by this patch.
However, I'd like to see the process started here by ensuring that any
commented out blocks that are updated or replaced with new commented
code by the current patch are instead just replaced with nothing. Any
remaining cleanup can happen later.

Finally, while the set of .rej files linked in the original post does
help to clarify locations where application the jdk11u patch to jdk8u
was not straightforward that isn't really enough to explain 1) why the
patch failed to apply and 2) what needed to be done to remedy the
situation. I would very much prefer an account of both of those before
accepting this patch. For example, was the failure down to a minor
difference in layout, a failure to match because an upstream change was
not present or a difference arising because of a disparity in design or
features between the jdk8u and jdk11u JVMs. At the very least a few
lines of explanation  per .rej file would be helpful. That would allow
me to make a much more confident assessment of the correctness of the
backport and will also stand as a record of what was needed for later
reference in case any problems arise when this goes into production use.

On the plus side this change is pretty much self-contained, only really
affecting the JFR code and, in particular, the leak analysis part of
that code. So, whatever risk the lack of clarity over precisely what has
been changed implies is /probably/ fairly tightly localized to users of
JFR who enable leak analysis. Since that function is effectively broken
anyway I'm not extremely concerned about allowing this patch to go into
the release. I'd still like to see the rationale for the changes before
agreeing that though.

So, to sum up before this can be pushed I'd like to see 1) a revised
patch with the commented out code deleted and 2) for each .rej file a
summary of what was changed between jdk11u and jdk8u and why. I'll take
another look once those are provided.

regards,


Andrew Dinn
-----------
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill