On Sat, 16 Dec 2023 15:40:50 GMT, Vladimir Sitnikov <vsitni...@openjdk.org> wrote:
>> @vlsi This is a very interesting solution 🤩, but it opens a number of >> questions! 🤔 As a start, here are mine: >> * You propose a new public method (`OutputStream.write(ByteBuffer)`) as part >> of your solution. We need to discuss whether this is worth the effort to >> change all implementations (and to perceive acceptable performance, all >> custom authors need to optimize their custom classes, too). We also need to >> discuss whether we like the design choice that de facto the public API (not >> just the implementation) of the legacy IO domain (`OutputStream`) is now >> linked to the NIO domain (`ByteBuffer`) (which, IMHO, feels some kind of >> scary beyond `ChannelOutputStream`). >> * Just thinking loudly: I wonder if we could simplify the solution, or if we >> could turn parts of it into some generic `byte[] readOnly(byte[])` utility. >> * I would like to know how much faster the solution with `ByteBuffer` is >> compared to `Arrays.copyOfRange()`: Is it really so huge that it is worth >> the additional trouble? >> * I would like to know how much slower the solution with `ByteBuffer` is >> compared to *skipping* `Arrays.copyOfRange()` for trusted cases (as you >> removed the skipping). >> * I would like to know the performance of custom streams, because your >> default implementation is filling a temporary byte array with a >> Java-implemented byte-by-byte loop, which IMHO would be rather painful >> compared to hardware-supported `copyOrRange()`, and it does that even in >> case of *trusted* targets. >> * @briangoetz Wouldn't we rather teach the Java language some day to provide >> native read-only arrays? That would be much faster, much better to read and >> implies less complex code for the end user. > > @mkarg , thank you for the review, I replied in JIRA to avoid mixing comments > across different issues: > https://bugs.openjdk.org/browse/JDK-8321271?focusedId=14634794&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14634794 While frozen arrays are on our radar, this is a significant lift, so unlikely to be coming all that soon. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/16879#discussion_r1428846145
