Skip to content

Conversation

Zentrik
Copy link
Collaborator

@Zentrik Zentrik commented Aug 20, 2023

The main performance improvement is from no longer calling length(src) which is O(n). I don't understand why that was ever being called as starting the parse from there shouldn't give anything useful. Also, I don't see any tests failing here or in TypedSyntax after that change so I think it should be fine.

Around a 5-10% improvement for long files.

Main change is not calling `length(src)` which is linear in `src`.
`linestarts` could be converted to a StaticArray with size 21 but I don't think that would do much.
Also remove type assertion just in case
@codecov
Copy link

codecov bot commented Aug 20, 2023

Codecov Report

Patch coverage: 94.11% and project coverage change: -0.25% ⚠️

Comparison is base (9db8d0e) 88.82% compared to head (d4bc396) 88.58%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #121      +/-   ##
==========================================
- Coverage   88.82%   88.58%   -0.25%     
==========================================
  Files           3        3              
  Lines         367      368       +1     
==========================================
  Hits          326      326              
- Misses         41       42       +1     
Files Changed Coverage Δ
src/CodeTracking.jl 87.24% <94.11%> (-0.59%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Zentrik
Copy link
Collaborator Author

Zentrik commented Aug 20, 2023

This pr Zentrik#3 just removes the length call and so is a significantly simpler change whilst still retaining most of the performance improvement of this pr.

@timholy
Copy link
Member

timholy commented Aug 22, 2023

I noticed that other was closed, does that mean this one offers significant extra benefits?

@Zentrik
Copy link
Collaborator Author

Zentrik commented Aug 22, 2023

Sorry, I forgot about that. I closed it because that was on my personal repo, I've created a pr for this repo #122.

@Zentrik Zentrik closed this Aug 22, 2023
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

Successfully merging this pull request may close these issues.

2 participants