JuliaEcosystem / PackageAnalyzer.jl

https://juliaecosystem.github.io/PackageAnalyzer.jl/dev/
MIT License
59 stars 5 forks source link

perf bugfix: only get line number, not col #88

Closed ericphanson closed 1 year ago

ericphanson commented 1 year ago
julia> @time analyze("WinEncoding");
  4.360394 seconds (61.29 k allocations: 26.116 MiB)

still slow, but down from a reported 5 mins

giordano commented 1 year ago

But tests on 1.6 are failing 😕

giordano commented 1 year ago

Sounds like Flux us using syntax not available in v1.6?

ericphanson commented 1 year ago

This wouldn't throw on actual usage btw, but I turn off exception handling for the tests so we aren't silently missing things. I think the fix here is to just choose another package for that test instead of Flux

giordano commented 1 year ago

For the record, on v3.0.1 I get

julia> @time analyze("WinEncoding");
  5.636592 seconds (11.69 M allocations: 814.376 MiB, 2.88% gc time, 78.45% compilation time)

julia> @time analyze("WinEncoding");
  0.457882 seconds (60.69 k allocations: 25.983 MiB)

After compilation (😳) the run seems to be pretty fast.

For comparison, on v3.0.0 it was

julia> @time analyze("WinEncoding");
292.698418 seconds (11.71 M allocations: 815.138 MiB, 0.05% gc time, 1.50% compilation time)

julia> @time analyze("WinEncoding");
289.030513 seconds (60.87 k allocations: 26.189 MiB, 0.00% gc time)
giordano commented 1 year ago

For the record, I believe the slow parsing before this PR was due to this recursive for loop: https://github.com/JuliaEcosystem/PackageAnalyzer.jl/blob/f7e51cc2d95224da38938cef5f05885744d79ce0/src/LineCategories.jl#L91-L94 Files like https://github.com/testercwt/WinEncoding.jl/blob/3b83ff5d13078bc2daa03f7cfa9e844945daecaa/src/cp936.jl stressed this out very hard.

ericphanson commented 1 year ago

Ah ok. Semantically we check everything on each line to see if we want to change our categorization of it (eg white space then inline comment then docstring), so it’s not super obvious to me how to optimize that loop but maybe there’s a way to do so. But your 0.5s seems alright, and this is some really weird source code, so maybe it’s fine as-is.

Definitely nice to see it isn’t a JuliaSyntax perf bug!