3x slow down in string benchmarks due to Fix annotated join with non-concrete eltype iters (#54919)

[Zentrik]

The ["string", "readuntil", "target length 1"] and ["string", "readuntil", "target length 2"] benchmarks got 3x slower recently. I bisected to 462d7f4, @tecosaur.

See https://github.com/JuliaCI/NanosoldierReports/blob/master/benchmark/by_date/2024-07/26/report.md or https://tealquaternion.camdvr.org/compare.html?start=6b08e80bd1217c5c2eb6c89604c21faa5b2a156f&end=42a8efff5a6b96394a36af541cf8a7a4725632fb&stat=min-wall-time&name=string for the benchmark results

[tecosaur]

Thanks for investigating this @Zentrik.

Regarding the eyebrow-raising regressions you highlighted (string/readuntil/target length {1,2}), I can reproduce them comparing 1.10 to nightly:

# 1.10
julia> using Chairmarks

julia> buffer = IOBuffer("A" ^ 50000)
IOBuffer(data=UInt8[...], readable=true, writable=false, seekable=true, append=false, size=50000, maxsize=Inf, ptr=1, mark=-1)

julia> @b readuntil(seekstart($buffer), "AA")
13.762 ns (1 allocs: 64 bytes)

# Nightly
julia> @b readuntil(seekstart($buffer), "AA")
92.853 ns (4 allocs: 208 bytes)

From a quick look though, it's hard for me to see how the commit you linked (changing some of the join code) affects them. It also seems like most of the additional time is from the seekstart(...) call, which I find strange? I tested using BenchmarkTools's @btime as well, and that also gave ~90ns for that expression, and it also recorded ~70ns for seek($buffer, 0).

I'll dig further into this, but if anybody else would like to take a look at this, that would be appreciated.

[nsajko]

julia/base/strings/io.jl

Lines 357 to 371 in 197295c

	if isconcretetype(eltype(iterator)) && !_isannotated(eltype(iterator)) && !any(_isannotated, args)
	sprint(join, iterator, args...)
	else
	io = AnnotatedIOBuffer()
	join(io, iterator, args...)
	# If we know (from compile time information, or dynamically in the case
	# of iterators with a non-concrete eltype), that the result is annotated
	# in nature, we extract an `AnnotatedString`, otherwise we just extract
	# a plain `String` from `io`.
	if isconcretetype(eltype(iterator)) || !isempty(io.annotations)
	read(seekstart(io), AnnotatedString{String})
	else
	String(take!(io.io))
	end
	end

Probably not relevant for performance, but it'd be nice to deduplicate the code here, e.g., eltype(iterator) is in triplicate:

[tecosaur]

Mmm, the to me the crux of it is that the logic I added in the code you link @nsajko should be fully resolved using compile-time information, except in the case of type-unstable iterators.

It may be the case that @code_warntype lied to me about what would be resolved at compile-time and what wouldn't (that's happened a few times now), but regardless I have a hard time seeing how _join_preserve_annotations is called in the path of the ["string", "readuntil", "target length 2"] benchmark, which seems to just be:

buffer = IOBuffer("A" ^ 50000) # setup
readuntil(seekstart($buffer), "AA") # readuntil 2 benchmark

[KristofferC]

I have a hard time seeing

The easiest might be to just revert locally and check if it fixes things. In that case, you just get the correct answer, no matter if it is hard to see or not.

[vtjnash]

Don't know if significant here, but usually all(!f, c) would be preferred to !any(f, c), I think, given that any and all are shortcircuiting.

Since they are both short circuiting, neither is preferred, and so it isn't something that 'should' be changed (absent other considerations such as profiling)

[KristofferC]

My guess is that this has something to do with #35800 and that inference of seekstart(::IOBuffer) somehow gets poisoned by the call at

julia/base/strings/io.jl

Line 367 in 462d7f4

read(seekstart(io), AnnotatedString{String})

[simeonschaub]

I tracked down the performance regression in seekstart to #54070, if I revert that commit the performance difference of @b seekstart($buffer) goes away. Since that's a bugfix though, I'm not sure we can just revert it.

It also doesn't fully fix the issue here since readuntil is still slower:

# master with #54070 reverted
julia> @b readuntil(seekstart($buffer), "AA")
44.819 ns (3 allocs: 192 bytes)

# 1.10
julia> @b readuntil(seekstart($buffer), "AA")
23.820 ns (1 allocs: 64 bytes)

[vtjnash]

The remaining delta turns out to be because of an artifact of the benchmark definition itself: because the return value is a zero length string, the runtime could cheat and skip much of the work. However, the newly added copyuntil now preallocates a buffer of 70 bytes (previously it did not) and adds an extra IOBuffer wrapper to simplify the interface. So essentially the benchmark is measuring the ratio of the penalty of the hardcoded size on the guess for the output (0 or 70 bytes) against the actual observed bytes. That is entirely synthetic though. I measure that they have the same performance on this benchmark if I remove that pre-allocation, but that there is little real-world advantage to removing it.