Use Base.hash

[pfitzseb]

This improves performance by a bit when tokenizing Base and reduces code complexity:

Tokenize on master [$!+] 
λ julia --project=. test/profile.jl
First run took 0.51364849 seconds with 8.73827 MB allocated
Tokenized 1556 files in 0.8023270789999989 seconds with 923.379736 MB allocated

Tokenize on sp/better-hash [$] 
λ julia --project=. test/profile.jl      
First run took 0.485027563 seconds with 4.93732 MB allocated
Tokenized 1556 files in 0.6423206330000001 seconds with 923.379736 MB allocated

[codecov]

Codecov Report

Merging #192 (65f8b74) into master (860bdc6) will increase coverage by 0.12%.
The diff coverage is 96.42%.

@@            Coverage Diff             @@
##           master     #192      +/-   ##
==========================================
+ Coverage   82.80%   82.93%   +0.12%     
==========================================
  Files           4        4              
  Lines         826      832       +6     
==========================================
+ Hits          684      690       +6     
  Misses        142      142              

Impacted Files	Coverage Δ
src/lexer.jl	93.80% <96.42%> (+0.05%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 860bdc6...65f8b74. Read the comment docs.

[KristofferC]

Any idea where all the allocations are from?

[pfitzseb]

--track-allocation attributes 3.7MB to

Tokenize.jl/src/lexer.jl

Line 853 in bed7f32

l2 = copy(l)

and 1 MB to

Tokenize.jl/src/lexer.jl

Line 100 in bed7f32

tokenize(x, ::Type{RawToken}) = Lexer(x, RawToken)

Still looking for the rest...

[KristofferC]

Tokenize.jl/src/lexer.jl

Line 853 in bed7f32

l2 = copy(l)

looks kinda crazy to me. Why would we need to copy the whole lexer?

[pfitzseb]

Because interpolation resets everything?

Anyways, the remaining allocs were due to me reading the whole file into a string while profiling; without that, I get

λ julia --project=. test/profile.jl
First run took 0.561283966 seconds with 73.583024 MB allocated
Tokenized 1556 files in 3.632539304999996 seconds with 2.368488 MB allocated

[pfitzseb]

is what the profile looks like. So yes, allocating the new IOBuffer() in copy looks expensive, but that's mostly because the GC runs then.

[KristofferC]

Because interpolation resets everything?

Hmm, well not everything? You are still reading characters into the same char buffer, so that one seems unnecessary to copy, or?

[pfitzseb]

Right, but crucially we can't reuse l.charstore. The latest commit removes the copy and is faster in the RawToken case, but slightly worse for normal Tokens. I'll leave the decision on what implementation to use up to you :)

[KristofferC]

RawToken seems what you would use if you want the best performance so I think it makes sense to optimize for that.

[c42f]

simple_hash currently has the property that it's a perfect hash on the subset of keywords, so identifiers can't collide with keywords.

But I guess using Base.hash won't necessarily have this property and #189 could reoccur?

It may be better just to optimize simple_hash a bit and see if all the branches can be removed. Tracking the length of the identifier (in bytes not chars?) as well as this hash would probably make the 0xff special cases unnecessary.

[pfitzseb]

That's a fair point. I pushed a commit that should implement a correct perfect hash which is only barely slower than Base.hash. Would be great if you (or someone else) could also try benchmarking this because timings are somewhat unstable on my machine.

[c42f]

Would be great if you (or someone else) could also try benchmarking this because timings are somewhat unstable on my machine.

I tried this but I also found quite a lot (~ 5%) difference between runs (on a linux laptop with julia 1.6). As far as I could tell the changes in this PR were performance neutral with respect to that measurement noise level.

I also tried a table-based hash like the following which seemed significantly (~2x) faster in a microbenchmark. But it does cost some memory and didn't change the performance overall compared to the noise.

const _keyword_hash_charvals = let
    x = fill(0x1f, 256)
    x[Int.('a':'z')] .= 1:26
    x
end

@inline function keyword_hash(c::Char, h::UInt64)
    b = Base.first_utf8_byte(c)
    # Saturate top 5 bits to indicate overflow at >= 12 chars.
    (h & 0xf800000000000000) | (h << 5) | _keyword_hash_charvals[b]
end

[c42f]

It's not strictly part of this PR but I just noticed the T == Token check here. Why do we have this here when readon(l) already contains the logic for Token vs RawToken?

[KristofferC]

@pfitzseb, is this ready to go?

[pfitzseb]

I think so, but it doesn't actually do much :P

[KristofferC]

The benchmark in the first post looks pretty good, or?

[c42f]

FWIW I based my copy/fork in JuliaSyntax.Tokenize off this branch. I'd be happy to see it merged.