Re: RFR: 8284638: store skip buffers in InputStream Object [v7]
On Wed, 20 Apr 2022 16:36:16 GMT, XenoAmess wrote: >>> no, we use other way, but yes for we take care of thread safety. >> >> Fair enough. >> >>> Don't think it necessary... I think making it cannot touched by other >>> object (with security manager on) is enough. >> >> I was thinking more for heapdumps due to extending the life of the skip >> buffer in this patch. At least we don't have to waste more cycles. >> >> Oh I forgot last time but, it looks like MIN_SKIP_BUFFER_SIZE is not used. >> Unless I'm missing something I assume that is a bug. > > @jmehrens comments about not making it static added. > @XenoAmess This pull request has been inactive for more than 4 weeks and will > be automatically closed if another 4 weeks passes without any activity. To > avoid this, simply add a new comment to the pull request. Feel free to ask > for assistance if you need help with progressing this pull request towards > integration! comment. - PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8284638: store skip buffers in InputStream Object [v7]
On Tue, 19 Apr 2022 22:37:21 GMT, jmehrens wrote: >>> > @jmehrens what about this then? I think it safe now(actually this >>> > mechanism is learned from Reader) >>> >>> Reader uses a lock object and it appears that InputStream locks on this >>> (per make/reset) I would assume now that you have some object member fields >>> you need to hold some lock while accessing those. Even though single thread >>> access would be the expected case. >> >> no, we use other way, but yes for we take care of thread safety. >> >>> Not related but, it looks like the static buffer issue has come up a few >>> times. Maybe time to add a test to: >>> https://github.com/openjdk/jdk/blob/master/test/jdk/java/io/InputStream/Skip.java >>> that would fail if the skip buffer is static? >> >> I think it is worthy to add some comments, but a test for it sound a little >> bit extreme. >> >>> Not really the primary issue but, it seems questionable to me that we don't >>> have to fill the skip buffer with zeros after the last read. That way any >>> sensitive information that was skipped would also be forgotten. Matching >>> with Reader seems more important for now. >> >> Don't think it necessary... I think making it cannot touched by other object >> (with security manager on) is enough. > >> no, we use other way, but yes for we take care of thread safety. > > Fair enough. > >> Don't think it necessary... I think making it cannot touched by other object >> (with security manager on) is enough. > > I was thinking more for heapdumps due to extending the life of the skip > buffer in this patch. At least we don't have to waste more cycles. > > Oh I forgot last time but, it looks like MIN_SKIP_BUFFER_SIZE is not used. > Unless I'm missing something I assume that is a bug. @jmehrens comments about not making it static added. - PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8284638: store skip buffers in InputStream Object [v7]
On Tue, 19 Apr 2022 22:47:42 GMT, liach wrote: >> XenoAmess has updated the pull request incrementally with one additional >> commit since the last revision: >> >> change to liach operation. > > src/java.base/share/classes/java/io/InputStream.java line 62: > >> 60: >> 61: /** Skip buffer, null until allocated */ >> 62: private byte[] skipBuffer = null; > > This can be removed too as you have the soft reference already. @liach done. - PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8284638: store skip buffers in InputStream Object [v7]
On Fri, 15 Apr 2022 18:56:37 GMT, XenoAmess wrote: >> @jmehrens what about this then? >> I think it safe now(actually this mechanism is learned from Reader) > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > change to liach operation. src/java.base/share/classes/java/io/InputStream.java line 62: > 60: > 61: /** Skip buffer, null until allocated */ > 62: private byte[] skipBuffer = null; This can be removed too as you have the soft reference already. - PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8284638: store skip buffers in InputStream Object [v7]
On Tue, 19 Apr 2022 18:04:03 GMT, XenoAmess wrote: > no, we use other way, but yes for we take care of thread safety. Fair enough. > Don't think it necessary... I think making it cannot touched by other object > (with security manager on) is enough. I was thinking more for heapdumps due to extending the life of the skip buffer in this patch. At least we don't have to waste more cycles. Oh I forgot last time but, it looks like MIN_SKIP_BUFFER_SIZE is not used. Unless I'm missing something I assume that is a bug. - PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8284638: store skip buffers in InputStream Object [v7]
On Tue, 19 Apr 2022 18:04:03 GMT, XenoAmess wrote: > Reader uses a lock object and it appears that InputStream locks on this (per > make/reset) I would assume now that you have some object member fields you > need to hold some lock while accessing those. Even though single thread > access would be the expected case. Locks are expensive. We just use buffers that may or may not be shared by different threads (for the java memory model), because locking objects is too costly performance-wise. - PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8284638: store skip buffers in InputStream Object [v7]
On Mon, 18 Apr 2022 20:08:23 GMT, jmehrens wrote: > > @jmehrens what about this then? I think it safe now(actually this mechanism > > is learned from Reader) > > Reader uses a lock object and it appears that InputStream locks on this (per > make/reset) I would assume now that you have some object member fields you > need to hold some lock while accessing those. Even though single thread > access would be the expected case. no, we use other way, but yes for we take care of thread safety. > Not related but, it looks like the static buffer issue has come up a few > times. Maybe time to add a test to: > https://github.com/openjdk/jdk/blob/master/test/jdk/java/io/InputStream/Skip.java > that would fail if the skip buffer is static? I think it is worthy to add some comments, but a test for it sound a little bit extreme. > Not really the primary issue but, it seems questionable to me that we don't > have to fill the skip buffer with zeros after the last read. That way any > sensitive information that was skipped would also be forgotten. Matching with > Reader seems more important for now. Don't think it necessary... I think making it cannot touched by other object (with security manager on) is enough. - PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8284638: store skip buffers in InputStream Object [v7]
On Fri, 15 Apr 2022 18:56:37 GMT, XenoAmess wrote: >> @jmehrens what about this then? >> I think it safe now(actually this mechanism is learned from Reader) > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > change to liach operation. > @jmehrens what about this then? I think it safe now(actually this mechanism > is learned from Reader) Reader uses a lock object and it appears that InputStream locks on this (per make/reset) I would assume now that you have some object member fields you need to hold some lock while accessing those. Even though single thread access would be the expected case. Not related but, it looks like the static buffer issue has come up a few times. Maybe time to add a test to: https://github.com/openjdk/jdk/blob/master/test/jdk/java/io/InputStream/Skip.java that would fail if the skip buffer is static? Not really the primary issue but, it seems questionable to me that we don't have to fill the skip buffer with zeros after the last read. That way any sensitive information that was skipped would also be forgotten. Matching with Reader seems more important for now. - PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8284638: store skip buffers in InputStream Object [v7]
> @jmehrens what about this then? > I think it safe now(actually this mechanism is learned from Reader) XenoAmess has updated the pull request incrementally with one additional commit since the last revision: change to liach operation. - Changes: - all: https://git.openjdk.java.net/jdk/pull/5872/files - new: https://git.openjdk.java.net/jdk/pull/5872/files/1f6e0eb7..9854f523 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5872=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5872=05-06 Stats: 22 lines in 2 files changed: 14 ins; 5 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/5872.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5872/head:pull/5872 PR: https://git.openjdk.java.net/jdk/pull/5872
