CFV: New JDK Reviewer: Dmitrij Pochepko

[Andrew Dinn]

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