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

Large performance regression with skipchars on small/medium inputs #21109

Closed
fcard opened this issue Mar 20, 2017 · 3 comments
Closed

Large performance regression with skipchars on small/medium inputs #21109

fcard opened this issue Mar 20, 2017 · 3 comments

Comments

@fcard
Copy link
Contributor

fcard commented Mar 20, 2017

Julia 0.5:

julia> mktemp() do _, file
                println(file, "G")
                flush(file)
                seek(file, 0)
                skipchars(file, islower)
                @time for i in 1:1000000; skipchars(file, islower) end
              end
  0.016611 seconds

Julia 0.6 (alpha.182):

julia> mktemp() do _, file
                       println(file, "G")
                       flush(file)
                       seek(file, 0)
                       skipchars(file, islower)
                       @time for i in 1:1000000; skipchars(file, islower) end
                     end
  0.951664 seconds (1000.00 k allocations: 30.518 MiB, 0.33% gc time)

Booo! Whoever introduced this sucks!

Just so people won't forget about it. This problem happens until there are about 40/50 characters to be skipped with an inexpensive predicate. (islower, isspace, etc)

I've got a first draft of a fix >>here<< (code under MIT license, etc etc), but I am not willing to make a PR at the moment.

@nalimilan
Copy link
Member

nalimilan commented Mar 20, 2017

What do you mean by "not willing"? Do you need some time, or do you need somebody else to fix this?

Cf. #18752

@fcard
Copy link
Contributor Author

fcard commented Mar 20, 2017

Someone else, plz.

@tkelman tkelman added the potential benchmark Could make a good benchmark in BaseBenchmarks label Mar 20, 2017
@fcard
Copy link
Contributor Author

fcard commented Mar 20, 2017

I was convinced to give a PR a go, because the solution turned out to be really simple. There it is: #21117

Still not perfect but much better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants