CFV: New JDK Reviewer: Dmitrij Pochepko
dms at samersoff.net
Tue Jul 30 13:44:55 UTC 2019
Thank you for sharing your view, we appreciate your candid feedback.
Needless to say, that we greatly appreciate the value that you
constantly bring to OpenJDK.
However, our perception of the situation is different. We believe that
you might have missed some facts, for example that, Dmitrij did his best
to address your concerns promptly and the 5 RFRs from Dmitrij are
waiting for review.
I hope that all the issues can be resolved if we just speak to each
other. We would like to have an opportunity to clarify your remaining
concerns and work out a solution plan to address it.
Therefore, as the first step to
"work together to reach a mutually-agreeable resolution" could we sync
with you, Andrew Haley and Dmitrij on CW32 by zoom call?
I’d like to finish by saying that we are very grateful to you for the
time you have spent looking into that complex code and hope to have a
constructive dialogue going forward.
On 26.07.2019 18:05, Andrew Dinn wrote:
> Vote: No
> I am not at all happy to see this proposal accepted. Specifically, my
> concern relates to the experience of reviewing Dmitrij's proposed
> changes for the AArch64 math and String intrinsics (mentioned by
> Vladimir in his proposal). The problem was not just the poor quality of
> the changes initially proposed (specifically, extremely poor
> documentation of complex code, a woefully inadequate test plan and, as a
> result of the latter, very serious errors in the code). I was especially
> unhappy with some of Dmitrij's responses to the reviews:
> When I asked whether he could show that one of the maths intrinsic
> code optimizations he was proposing was valid he claimed to have a proof
> that this was so but declined to provide it. The excuse offered was that
> it was too difficult to do so in email. That does not manifest respect
> for the review process. It also shows a cavalier attitude to correctness
> (correct by say so is a dangerous way to operate, especially with
> complex mathematical code where proof is vital). Neither of these
> recommend him as a potential reviewer.
> During the first review I undertook I went to great lengths to explain
> why and how Dmitrij needed to comment the algorithms he was using at a
> very detailed level in order to ensure that his patch would not pose a
> serious maintenance problem. In the first instance he claimed not to
> understand what I was asking for nor why it was needed. So, I produced a
> complete description of his original, very complex algorithm and asked
> him to work that description into the code along with the revisions
> required to fix the errors I found.
> He failed to do this task as requested and simply inserted most of the
> original commentary unchanged i.e. not properly folded into the
> generated code and out of sync with revisions to his patch. Out of date
> commentary is pretty much useless, especially for complex mathematical
> code. In subsequent change requests for the String intrinsics Dmitrij
> continued to produce inadequately documented code. He did not seem to
> understand the value of a separate statement of what the code does from
> the code itself nor to appreciate that there is a trade-off between code
> clarity and simplicity vs squeezing every last cycle out of an
> implementation i.e. between minor performance gains and maintenance
> costs. I don't think someone who is either i) incapable of writing clear
> and clearly commented code or ii) unwilling to make the effort to ensure
> it it clear and clearly commented is an appropriate person to review
> other people's code.
> I found one of two serious errors in Dmitrij's implementation of the
> maths intrinsics. I don't doubt that the only reason I caught the bug
> before commit was because I spent /several weeks/ carefully identifying
> and then writing up the specifics of his chosen algorithm and relating
> them back to the underlying mathematics at which point I spotted a large
> hole in the implementation. Andrew Haley, by contrast, was more generous
> in his review of the other (trig) intrinsic, accepting more minimal
> comments and taking it on trust that the correct tests had been
> executed. As a result, a bug in the trig code was committed and almost
> made it into a release.
> Now these errors are not in themselves the problem. We all make mistakes
> which is why we have a review process. However, in this case the errors
> happened in telling circumstances. My wariness and consequent motivation
> to comb through Dmitrij's code came down to the fact that he described
> (but only after prompting) what appeared to be a very minimal and ad hoc
> test process. At that point alarm bells rang. Having reviewed the
> mathematics behind the implementation it became very quickly obvious
> that no serious thought had been put into testing -- a vital test case
> (denormal inputs) had been omitted. An equally egregious lapse turned
> out to have occurred with the test plan for the trig code Andrew
> reviewed -- inputs outside the range [-PI, PI] were omitted. I don't
> think this approach to implementation and testing signals someone who is
> ready to review other people's code.
> 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, Eric Shander
More information about the jdk-dev