Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

change IOBuffer to use Memory internally #53192

Merged
merged 1 commit into from
Feb 13, 2024
Merged

change IOBuffer to use Memory internally #53192

merged 1 commit into from
Feb 13, 2024

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Feb 5, 2024

An Array is often still allocated on output, but this gives the compiler a chance to potentially elide that in certain cases.

For measurement, it seems about 10% faster as a string builder:

julia> @btime repr("hello\nworld"^10);
  1.096 μs (10 allocations: 640 bytes) # master
  973.000 ns (9 allocations: 608 bytes) # PR
  994.000 ns (8 allocations: 576 bytes) # also PR, after Revise-ing Base.wrap

@oscardssmith
Copy link
Member

This looks good to me, but it is a lot of tricky code.

@vtjnash
Copy link
Member Author

vtjnash commented Feb 5, 2024

Yeah, looks like we trigger a CSV bug added in JuliaData/CSV.jl@953636a, which somehow gets involved with the Pkg tests (Pkg maybe is setting up a bad environment, as I don't think it looks like CSV should be installed there). As well as a problem exposed with the REPL precompile test coverage which should be an easy fix.

@IanButterworth
Copy link
Member

CSV was added pinned as a test for a specific issue JuliaLang/Pkg.jl#3361

@vtjnash
Copy link
Member Author

vtjnash commented Feb 6, 2024

@nanosoldier runbenchmarks(!"scalar", vs=":master")

@oscardssmith
Copy link
Member

the CSV issue should be fixed by JuliaData/CSV.jl#1121

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

@vtjnash
Copy link
Member Author

vtjnash commented Feb 7, 2024

several good runtime improvements, no runtime regressions

["inference", "optimization", "Base.init_stdio(::Ptr{Cvoid})"]	0.96 (5%)	0.97 (1%) ✅
["io", "array_limit", ("display", "Matrix{Float64}(10000, 10000)")]	0.99 (5%)	0.97 (1%) ✅
["io", "array_limit", ("display", "Matrix{Float64}(100000000, 1)")]	1.00 (5%)	0.99 (1%) ✅
["io", "array_limit", ("display", "Vector{Float64}(100000000,)")]	1.00 (5%)	0.99 (1%) ✅
["dates", "string", "Date"]	0.90 (5%) ✅	0.94 (1%) ✅
["dates", "string", "DateTime"]	0.91 (5%) ✅	0.92 (1%) ✅
["io", "read", "readstring"]	0.70 (5%) ✅	0.50 (1%) ✅
["micro", "parseint"]	0.88 (5%) ✅	0.66 (1%) ✅
["problem", "imdb", "centrality"]	1.00 (5%)	0.99 (1%) ✅

@vtjnash vtjnash added merge me PR is reviewed. Merge when all tests are passing and removed merge me PR is reviewed. Merge when all tests are passing labels Feb 10, 2024
@vtjnash
Copy link
Member Author

vtjnash commented Feb 10, 2024

Still encountering some sort of error in the REPL package, but seems possible to work around this

      From worker 14:	┌ Info: REPL: trace compile output:
2024-02-10 04:00:40 UTC	      From worker 14:	└ precompile(Tuple{typeof(Base.write), Base.GenericIOBuffer{GenericMemory{:not_atomic, UInt8, Core.AddrSpace{Core}(0x00)}}, String})

@IanButterworth
Copy link
Member

Fixes would be adding that precompile statement or relax the test. Currently allows no compilation during interactive startup

@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label Feb 10, 2024
An Array is often still allocated on output, but this gives the compiler
a chance to potentially elide that in certain cases.

For measurement, it seems about 10% faster as a string builder:

julia> @Btime repr("hello\nworld"^10);
  1.096 μs (10 allocations: 640 bytes) # master
  973.000 ns (9 allocations: 608 bytes) # PR
  994.000 ns (8 allocations: 576 bytes) # also PR, after Revise-ing Base.wrap
@vtjnash vtjnash merged commit d7b9ac8 into master Feb 13, 2024
4 of 7 checks passed
@vtjnash vtjnash deleted the jn/iobuffer-memory branch February 13, 2024 15:32
@oscardssmith oscardssmith added performance Must go faster and removed merge me PR is reviewed. Merge when all tests are passing labels Feb 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Must go faster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants