CFV: New JDK Reviewer: Dmitrij Pochepko

[Andrew Dinn]

Vote: veto

I have amended my vote to use the required term. Apologies for not
following correct form in my previous post and thanks to Dean Long and
David Holmes for correcting me. The comments below are as per the
previous note.

regards,


Andrew Dinn
-----------

On 26/07/2019 16: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.
> 
> 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, Eric Shander
> 

-- 
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, Eric Shander