Skip to content

change IOBuffer to use Memory internally#53192

Merged
vtjnash merged 1 commit intomasterfrom
jn/iobuffer-memory
Feb 13, 2024
Merged

change IOBuffer to use Memory internally#53192
vtjnash merged 1 commit intomasterfrom
jn/iobuffer-memory

Conversation

@vtjnash
Copy link
Copy Markdown
Member

@vtjnash vtjnash commented Feb 5, 2024 •
edited
Loading

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
Copy Markdown
Member

oscardssmith commented Feb 5, 2024

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

@vtjnash
Copy link
Copy Markdown
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
Copy Markdown
Member

IanButterworth commented Feb 5, 2024

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

@vtjnash vtjnash mentioned this pull request Feb 6, 2024
Merged
@vtjnash
Copy link
Copy Markdown
Member Author

vtjnash commented Feb 6, 2024

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

@oscardssmith
Copy link
Copy Markdown
Member

oscardssmith commented Feb 6, 2024

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

@nanosoldier
Copy link
Copy Markdown
Collaborator

nanosoldier commented Feb 7, 2024

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

@vtjnash
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

IanButterworth commented Feb 10, 2024

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
@vtjnash
change IOBuffer to use Memory internally
e6d028d 
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
@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
@longemen3000 longemen3000 mentioned this pull request Feb 25, 2024
Closed
@Zentrik Zentrik mentioned this pull request Apr 4, 2024
Closed
@nhz2 nhz2 mentioned this pull request Apr 16, 2024
Closed
@KristofferC KristofferC mentioned this pull request Jun 19, 2024
Closed
@nhz2 nhz2 mentioned this pull request Oct 10, 2024
Closed
@caleb-allen caleb-allen mentioned this pull request Feb 7, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

performance Must go faster

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@vtjnash @oscardssmith @IanButterworth @nanosoldier