RFR: JDK-8319307: DCmds should not assert on truncation and should report truncation [v2]

[Thomas Stuefe]

On Fri, 10 Nov 2023 07:44:12 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

> > @tstuefe thanks for the history lesson. So it was assumed writes can't fail and messages can't be truncated, which was true until we hit actual system limits. So we imposed a cap that we didn't expect to reach in practice (hence assert) and an indicator that the message had been truncated (which is a good thing). So we were still pretending writes can't fail.
> 
> Yes
> 
> > Now you are saying, lets drop the pretense - the buffer is limited in size and callers have to (or can if they want to) deal with that. The question then is how? The usual way would be for writes to fail, report an error and caller then does something to resize the buffer or abort the writing. That would be very disruptive to the existing code. Instead you've added a way for a failing write to record that it failed in a side-channel: is_truncated(). The callers can then query this side-channel and decide what to do. Now that I write that all out this isn't as bad as I initially thought, but I still find it somewhat grating from an API perspective.
> 
> Yes. Minus the grating, I think having an observable "buffer full" signal would make sense at least for stringStream, which can be run over an existing static buffer. I agree that bufferedStream feels more like it should not limit output but flush occasionally. IMHO the real error is not honoring the flush() request.
> 
> > An alternative, for these problem cases, would be to go back to a buffer that attempts to grow as needed until it hits system limits. Are the actual buffers involved isolated so that we can choose which one(s) to change to be unlimited? If so are these commands ones that would likely lead to the OOM issues that caused the limit to be imposed in the first place?
> 
> Well, yes. Its the one buffer in the attach listener. We could just crank up the buffer maximum; we could also do that on a per-command base, for commands that we know may print a lot of output.

BTW, one possible improvement for bufferedStream could be to switch to mmap if backing buffer grows beyond a certain threshold. The benefit would be that upon release, this memory is really gone and does not lurk around increasing libc memory retention.

Another improvement could be to then reallocate by attempting to map adjacent pages. That removes the need for copying. One could even just use a VirtualSpace from the start, with a larger reserved size and commit on growth.

-------------

PR Comment: https://git.openjdk.org/jdk/pull/16474#issuecomment-1805314829