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

use a type parameter for stream in EachLine, improves performance #28253

Merged
merged 1 commit into from
Jul 26, 2018

Conversation

KristofferC
Copy link
Member

@KristofferC KristofferC commented Jul 24, 2018

From @martinholters idea in #19141 (comment)

function countchars(filename)
    io = open(filename, "r")
    c = 0
    for line in eachline(io, keep=true)
        c += length(line)
    end
    close(io)    
    c
end

@time countchars("PF00089.fasta")

File at https://raw.githubusercontent.com/diegozea/mitos-benchmarks/master/data/PF00089.fasta

Before:

0.255199 seconds (1.63 M allocations: 57.544 MiB, 0.66% gc time)

After:

0.064163 seconds (1.09 M allocations: 49.241 MiB, 5.91% gc time)

Julia 0.6

0.157770 seconds (1.63 M allocations: 99.056 MiB, 9.22% gc time)

@KristofferC KristofferC added performance Must go faster potential benchmark Could make a good benchmark in BaseBenchmarks labels Jul 24, 2018
@KristofferC
Copy link
Member Author

Not sure this is tracked but might as well

@nanosoldier runbenchmarks(ALL, vs = ":master")

@nanosoldier
Copy link
Collaborator

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

base/io.jl Outdated
end
EachLine(stream::IO=stdin; ondone::Function=()->nothing, keep::Bool=false) =
EachLine(stream, ondone, keep)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line should be unindented one level as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try fix it before merging with ci skip to not waste CI.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should remain inside the struct block so we don't expose a 3-argument EachLine method.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@martinholters
Copy link
Member

While at it, we could consider also making the type of ondone a parameter. Not sure it's worth it.

@KristofferC
Copy link
Member Author

I thought about it but it is only called once, right, so I felt that it probably wasn't needed.

@martinholters
Copy link
Member

Same feeling here, just wanted to make sure it's at least considered.

@JeffBezanson JeffBezanson merged commit 9e7ae17 into master Jul 26, 2018
@JeffBezanson JeffBezanson deleted the kc/eachline_io branch July 26, 2018 02:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Must go faster potential benchmark Could make a good benchmark in BaseBenchmarks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants