Performance patch for AIX system api support of sun.nio.ch.FileChannelImpl.transferTo0()

[Neil Richards]

On Thu, 2011-10-20 at 09:07 +0100, Alan Bateman wrote:
> Charles Lee wrote: 
> > Thanks Alan. Any requirements for performance patches to contribute?
> > Test case? Test result?
> Speaking generally, when working on a performance fix (or any fix for
> that matter) it's always good to understand the test coverage for the
> area that you are changing. The math area is a good example where
> performance work in 7 required extending the test coverage to give
> confidence that performance work didn't cause any correctness issues.
> There are examples in other areas too. When fixing bugs then the rule
> is that tests should be added (or existing tests updated) to cover the
> scenario that is being fixed. However there are cases, including
> performance, where it's not feasible or doesn't make sense to write an
> automated test. Tests need to run quickly so we wouldn't check in a
> test that takes an hour to demonstrate a bug for example. Also it's
> worth keeping existing tests in mind to avoid duplicating existing
> tests. With performance tests then it's as I was saying in the first
> mail, the jdk repository isn't really the best place for them in my
> view. It's fine to push tests that print results that we can run
> manually in a controlled environment (we have several examples of
> this) but we can't have tests that fail if they think that the
> performance isn't as expected. At some point it would it great if
> OpenJDK could get a repository and framework for micro benchmarks and
> maybe larger performance tests but that's a bigger discussion and not
> really something for this list.
> 
> -Alan.
> 
> 

Hi Charles,
As Alan says, it can be quite valuable to provide functional tests which
enhance the coverage of the existing tests to include the area whose
performance is being improved, as this helps to demonstrate that the
suggested change does not affect the function.

I think providing micro benchmark tests (where possible) can also be
valuable to demonstrate the performance benefit of the change (with
figures gathered from them given as part of the contribution).

I don't think such benchmark tests should have a "pass/fail" criteria -
they should merely report the result of their measurements (eg.
transactions per second, or whatever it happens to be).
This also means it makes no sense to have them run as part of the jtreg
functional tests, so they shouldn't use the '@test' tag. 

Whilst it would be fabulous to have a repository and/or framework for
such micro benchmarks to use, as things currently stand, it seems to me
that storing such tests somewhere under /test in the jdk repository (for
jdk code changes, that is) is about the best that can be done under the
existing conventions.

(It would be a shame to miss the opportunity to capture the criteria by
which a performance change was made merely because an overarching micro
benchmark framework and/or repository was not yet available).

----

As to the specific change being proposed, for ease of review I've
uploaded a webrev [1], marginally modified to address some spacing /
alignment issues, and to follow the '#elif defined(<PLATFORM>)'
convention that protects the code for the other platforms.

In review, the comment at lines 225-226 seem incomplete to me (under
what circumstances does send_file() return -1 ?).

Also, I wonder if it's really intended that the method returns
IOS_UNSUPPORTED_CASE if 'return != -1' and 'sf_iobuf.bytes_sent <= 0' ?

(This behaviour is more apparent with the change from #ifdef/#endif to
#elif/#else. It looked a little like it might be accidental, rather than
considered behaviour).

Also, what's the semantic difference between returning
'IOS_UNSUPPORTED' (line 210) and 'IOS_UNSUPPORTED_CASE' (lines 214, 231,
241) ?
(I don't see 'IOS_UNSUPPORTED' being used by the other platforms, so
wonder if this is a typo).

----

For the testcase code given, I'd be tempted to have the client and
server code run as two threads within the same process (unless there's
some good reason why this wouldn't drive the code properly that I've
missed).

----

Hope this helps,
Regards, Neil

[1] http://cr.openjdk.java.net/~ngmr/ojdk-199/webrev.00/index.html

-- 
Unless stated above:
IBM email: neil_richards at uk.ibm.com
IBM United Kingdom Limited - Registered in England and Wales with number 741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU