Refactor IOBuffer code #57570

jakobnissen · 2025-02-28T09:42:04Z

I've been a little frustrated with the IOBuffer code. It contains a whole bunch of implicit invariants, and is poorly commented. It also has several bugs that ultimately stems from the code being unclear about its own assumptions.

This is a refactoring of IOBuffer. The primary goals are:

Comment the code more heavily
Test the code more thoroughly

The secondary goals are

Fix a few outstanding bugs
Add some minor performance improvements

This is a purely internal refactoring with be no change in behaviour of IOBuffer, except straight up bugfixes. However, note that previous code may have relied on buggy behaviour. Fixing bugs may therefore cause breakage.

Current changes

BEHAVIOUR CHANGES

The following code used to not throw an error, but now does: IOBuffer(b"abc"; maxsize=2). I consider this a bugfix. It should not be possible to construct an IOBuffer with a buffersize larger than maxsize.
It used to be possible to write to indices higher than maxindex, which could trigger a bug causing data loss. The bug has been fixed, but as a result, some IOBuffers may reach full capacity faster (really: reach it at the correct point), changing writing behaviour.

Bugfixes

Allow reading from an IOBuffer backed by non-dense memory.
Invalidate non-dense memory buffer after take!
Do not corrupt data on copyline on a non-appending buffer
Respect maxsize even after take! (fix IOBuffer does not respect maxsize after the first take! #57549)
Fix bug when copying from an appending iobuffer to itself
Fix bug where re-allocating the buffer may cause it to shrink, discarding data.
Fix bug where truncate may throw a wrong BoundsError
Fix a bug where truncating a buffer may not correctly removed mark at position that has been deleted
Fix a bug where initializing an IOBuffer without an explicit buffer and with truncate=false makes it contain the full buffer

Changes to brittle code

Removed some tests that explicitly tested internal code and internal behaviour. Some of that behaviour has changed.
Changed some internal PipeBuffer behaviour, which did not respect writable IOBuffer's ownership of their buffer and therefore failed spuriously

Performance improvements

Writing to dense IOBuffer now uses memmove and is up to 10x faster for long writes.
Minor optimisations (about ten percent) for writing to IOBuffers in general

Closes #57549

vtjnash · 2025-02-28T14:21:51Z

base/iobuffer.jl

@@ -329,6 +404,7 @@ function _resize!(io::GenericIOBuffer, sz::Int)
    a = io.data
    offset = io.offset
    if applicable(resize!, a, sz)
+        # TODO: This is buggy: The buffer should never touch data before the offset.


This seems like you're claiming something as a bug which was never part of the contract. When using an IOBuffer with writing (especially if a resize is needed), it is allowed to destroy the underlying object to make more space

That is good to know - adding the comments is precisely to learn what is part of the contract! Okay, so the IOBuffer takes ownership of its underlying array - and, in the case of being passed a Vector, it then transitively takes control of the underlying Memory.

vtjnash · 2025-02-28T14:30:38Z

base/iobuffer.jl

+
+    # Data is read/written from/to ptr, except in situations where append is true, in which case
+    # data is still read from ptr, but written to size+1.
+    # This value is alwaus in offset+1 : size+1


This is actually a bug in the current implementation. The ptr is not supposed to be restricted to be inside the file size, and any calls to write that happen should then automatically zero-pad the file from size:ptr (https://pubs.opengroup.org/onlinepubs/009695399/functions/lseek.html)

Hmm how does that work? Would the user have to seek first to beyond size? That doesn't correspond to how seek normally works in Julia: Normally, you can't seek beyond the filesize. Should that be allowed? Then what happens if the user seeks beyond lastindex(buffer.data)?

How is it supposed to work with IOStream? What file should the following script create?

f = open("/tmp/foo", "w+") write(f, "abcd") flush(f) seek(f, 10) write(f, 'a') flush(f) close(f)

jakobnissen · 2025-03-03T08:42:36Z

Let's stop the PR here - tests pass, the most pressing issues has been addressed, and it's already a fairly large (too large?) PR. I'll make a followup PR with some more tests and a few more optimisations and bugfixes after this is merged and I get code coverage.

jakobnissen · 2025-03-03T09:12:06Z

Adding @nhz2 because you've been a thorough reviewer on previous IO related PRs. Feel free to unassign yourself.

base/iobuffer.jl

nhz2 · 2025-03-03T19:20:22Z

base/iobuffer.jl

-    buf.size = length(data) + offset
-    buf.offset = offset
+    if !iszero(offset)
+        unsafe_copyto!(mem, 1, mem, offset+1, len)


I think it is safe for src and dst to alias here, but from what I can tell the alias behavior of unsafe_copyto! is not documented.

It's safe, yes. Not documented, but I think that may be beyond the scope of this PR. Except if you suggest handling it another way?

base/iobuffer.jl

oscardssmith · 2025-03-03T20:40:29Z

base/iobuffer.jl

+    # Data is read/written from/to ptr, except in situations where append is true, in which case
+    # data is still read from ptr, but written to size+1.
+    # This value is always in 1 : size+1
+    ptr::Int


I wonder if we could make this a MemoryRef If possible, doing so might save a few cycles.

That can't be done, because a MemoryRef can't point to one past the end, which ptr needs to do to signal EOF

Oscar seems probably right that we could replace the Memory+offset fields with a single MemoryRef field, since that may express almost the same concept

That would require a new struct which is not generic over the array type. Otherwise I can try to give it a go.

vtjnash · 2025-03-03T22:31:18Z

The removal of offset is a breaking change and performance regression. That probably shouldn't be mixed in with a PR that claims to be just bug fixes and refactoring for clarity

jakobnissen · 2025-03-04T06:02:43Z

I would argue against it being a performance regression, but surely, it's not a breaking change. How is it breaking to remove a non-documented field of a struct? One which was added only in the current release? Which documented behaviour does that break?

Edit: I see it now. The documentation says that the buffer only controls any input array if write is true.

caleb-allen · 2025-03-04T15:45:08Z

base/iobuffer.jl

+
+# Position is zero-indexed, but ptr is one-indexed, hence the -1
+# TODO: Document that position for an unseekable stream is invalid, or
+# make it error
 position(io::GenericIOBuffer) = io.ptr - io.offset - 1


My vote here is for an error

mark returns the "marked position" of the stream, so erroring will also make mark error.

jakobnissen · 2025-03-04T18:52:40Z

Thanks for the review. All comments has now been addressed (except the one about endianness which I don't understand)

jakobnissen · 2025-03-05T07:04:59Z

The build failure (segfault) on 32-bit linux looks unrelated.

base/iobuffer.jl

jakobnissen · 2025-03-17T19:04:49Z

Bump. This is done now - a few people have reviewed it, but none have approved. It would be nice to get this in early in 1.13's cycle, so any bugs are detected early. Once this PR has landed, and #57668 has been fixed, I can make a followup PR to get test coverage closer to 100%

This allows e.g. reading from an IOBuffer backed by a UnitRange or whatever.

* For non-memory backed buffers, do not use invalid inbounds. * For memory-backed buffers, use unsafe_copyto!, which is much faster

Use the existing implementation of in directly with the buffer. This should already be fast

This fixes a bug introduced somewhere earlier in this chain of commits. However, the previous behaviour wasn't great either. In the changed code, correctness relied on the buffer having very specific resizing behaviour, i.e. a mercurial implementation detail. The current solution is more robust. However: 1. This workaround copies the data one extra time, which is not great 2. It's possible that the bug surfaced here due to another bug in the resizing behaviour. However, if so, I haven't been able to find it.

* Outline slow path in ensureoom. * Do not uses memcpy when writing short arrays

This is due to some sketchy behaviour by some other code which relies on IOBuffer internals, but shouldn't.

Before Julia 1.11, observing uninit bitstypes was UB. Now, it's no longer UB, so no need.

Also test the behaviour that depends on this field

Only copy the used bytes, not unreachable bytes.

* Remove unsafe trait objects, to avoid adding private methods to public functions * Remove a superfluous style change

Since it needs to write all its content, this would cause an infinite loop, exhausing all memory. Throw an error instead.

This makes the method write(::IOBuffer, ::IOBuffer) work like the generic fallback.

jakobnissen force-pushed the iobuffer branch from 34340d4 to 033e2d4 Compare February 28, 2025 11:39

vtjnash reviewed Feb 28, 2025

View reviewed changes

jakobnissen force-pushed the iobuffer branch 2 times, most recently from 21008ca to 7274c10 Compare February 28, 2025 20:25

nsajko added the io Involving the I/O subsystem: libuv, read, write, etc. label Mar 2, 2025

jakobnissen marked this pull request as ready for review March 3, 2025 08:41

jakobnissen added the status: waiting for PR reviewer PR is complete and seems ready to merge. Has tests and news/compat if needed. CI failures unrelated. label Mar 3, 2025

jakobnissen mentioned this pull request Mar 3, 2025

Semantics of seek #57618

Open

jakobnissen changed the title ~~WIP: Refactor IOBuffer code~~ Refactor IOBuffer code Mar 3, 2025

jakobnissen assigned nhz2 Mar 3, 2025

jakobnissen requested a review from nhz2 March 3, 2025 09:12

jakobnissen unassigned nhz2 Mar 3, 2025

jakobnissen force-pushed the iobuffer branch from 582dbca to 099d196 Compare March 3, 2025 13:53

KristofferC reviewed Mar 3, 2025

View reviewed changes

base/iobuffer.jl Outdated Show resolved Hide resolved

nhz2 reviewed Mar 3, 2025

View reviewed changes

oscardssmith reviewed Mar 3, 2025

View reviewed changes

jakobnissen force-pushed the iobuffer branch from 0e5f547 to 478afb5 Compare March 4, 2025 12:50

caleb-allen reviewed Mar 4, 2025

View reviewed changes

jakobnissen force-pushed the iobuffer branch from 1813cfa to 835a76b Compare March 10, 2025 12:07

nsajko reviewed Mar 11, 2025

View reviewed changes

base/iobuffer.jl Outdated Show resolved Hide resolved

nsajko reviewed Mar 11, 2025

View reviewed changes

base/iobuffer.jl Outdated Show resolved Hide resolved

nsajko reviewed Mar 11, 2025

View reviewed changes

base/iobuffer.jl Outdated Show resolved Hide resolved

jakobnissen added 23 commits March 21, 2025 09:12

Allow read and peek from non-dense IOBuffer

9ea4489

This allows e.g. reading from an IOBuffer backed by a UnitRange or whatever.

Also reinit used-up non-Memory buffer after take!

45f4ca1

Use memcpy to write to IOBuffer when applicable

8820dd2

* For non-memory backed buffers, do not use invalid inbounds. * For memory-backed buffers, use unsafe_copyto!, which is much faster

Simplify and speed-up occursin implementation

5d3d5e8

Use the existing implementation of in directly with the buffer. This should already be fast

Add more tests

09a11ea

Bugfix: Fix copyline for non-appending IOBuffer

caa3375

Minor optimisation of write(::IOBuffer, mem)

cf0a30b

* Outline slow path in ensureoom. * Do not uses memcpy when writing short arrays

Enforce invariants in IOBuffer constructors

e3a9c20

Tweak compacting heuristics

f0ca5fe

Bugfix: Copying to self with appending buffer

3673d4e

Rename Unsafe to UnsafeMethod

0d1c310

Fix errors during build

40fe2b3

This is due to some sketchy behaviour by some other code which relies on IOBuffer internals, but shouldn't.

Fix typo in test

ab6ccfc

Skip initializing IOBuffer data

c4f6a05

Before Julia 1.11, observing uninit bitstypes was UB. Now, it's no longer UB, so no need.

Reinstate offset field

ccb1db0

Also test the behaviour that depends on this field

Address some reviewer comments

5fbf4e0

Try to make inferrible for trimming CI

df1c215

Minor change in comments

df029e8

DECISION: Do not fill buffer past maxsize

164513e

More efficient copy of IOBuffer

3ce4bce

Only copy the used bytes, not unreachable bytes.

Address reviewer comments

16ff6ed

* Remove unsafe trait objects, to avoid adding private methods to public functions * Remove a superfluous style change

Fixup

52f60cd

jakobnissen added status: waiting for author Anybody home? and removed status: waiting for PR reviewer PR is complete and seems ready to merge. Has tests and news/compat if needed. CI failures unrelated. labels Mar 21, 2025

WIP: Address some reviewer comments

840c3de

jakobnissen force-pushed the iobuffer branch from d7cdd67 to 840c3de Compare March 21, 2025 08:55

jakobnissen added 3 commits March 21, 2025 10:00

Error when calling write(x::IOBuffer, x)

fcc83c1

Since it needs to write all its content, this would cause an infinite loop, exhausing all memory. Throw an error instead.

Make writing between IOBuffer always exhaust the source

b341967

This makes the method write(::IOBuffer, ::IOBuffer) work like the generic fallback.

Address some more reviewer comments

57a82a6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor IOBuffer code #57570

Refactor IOBuffer code #57570

jakobnissen commented Feb 28, 2025 •

edited

Loading

vtjnash Feb 28, 2025

jakobnissen Feb 28, 2025

vtjnash Feb 28, 2025

jakobnissen Feb 28, 2025

jakobnissen Feb 28, 2025 •

edited

Loading

jakobnissen commented Mar 3, 2025 •

edited

Loading

jakobnissen commented Mar 3, 2025

nhz2 Mar 3, 2025

jakobnissen Mar 4, 2025

oscardssmith Mar 3, 2025

jakobnissen Mar 4, 2025

vtjnash Mar 4, 2025

jakobnissen Mar 4, 2025

vtjnash commented Mar 3, 2025

jakobnissen commented Mar 4, 2025 •

edited

Loading

caleb-allen Mar 4, 2025

nhz2 Mar 4, 2025

jakobnissen commented Mar 4, 2025

jakobnissen commented Mar 5, 2025

jakobnissen commented Mar 17, 2025

Refactor IOBuffer code #57570

Are you sure you want to change the base?

Refactor IOBuffer code #57570

Conversation

jakobnissen commented Feb 28, 2025 • edited Loading

Current changes

BEHAVIOUR CHANGES

Bugfixes

Changes to brittle code

Performance improvements

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakobnissen Feb 28, 2025 • edited Loading

Choose a reason for hiding this comment

jakobnissen commented Mar 3, 2025 • edited Loading

jakobnissen commented Mar 3, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vtjnash commented Mar 3, 2025

jakobnissen commented Mar 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakobnissen commented Mar 4, 2025

jakobnissen commented Mar 5, 2025

jakobnissen commented Mar 17, 2025

jakobnissen commented Feb 28, 2025 •

edited

Loading

jakobnissen Feb 28, 2025 •

edited

Loading

jakobnissen commented Mar 3, 2025 •

edited

Loading

jakobnissen commented Mar 4, 2025 •

edited

Loading