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

Improve style resolution performance #453

Merged
merged 3 commits into from
Dec 18, 2017
Merged

Conversation

cmyr
Copy link
Member

@cmyr cmyr commented Dec 15, 2017

This patch significantly improves the speed at which we resolve scope spans into styles and greatly improves startup performance, especially noticeable on debug builds.

before:

screen shot 2017-12-14 at 8 17 09 pm

after:

screen shot 2017-12-14 at 8 18 00 pm

This uses a simple cache to keep track of already-computed scope stacks,
which greatly improves the efficiency of computing of new styles (at
some cost in memory.)
Copy link
Member

@raphlinus raphlinus left a comment

Choose a reason for hiding this comment

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

I haven't experimented carefully with this, but looks good, and looking forward to the performance improvement.

Copy link
Collaborator

@trishume trishume left a comment

Choose a reason for hiding this comment

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

This looks reasonable to me. One caveat: I don't remember why we went with this approach instead of using the pushes/pops I use in syntect. I know we discussed it a bunch, but I can't comment knowing that.

Also I seem to recall that at some point in that discussion I did benchmarks on how many different stacks you could expect to see in typical and atypical files. If you find the right old issue discussion you may find a comment with those results. That may inform concerns about memory.

// a prefix tree.
/// style state of existing scope spans, so we can more efficiently
/// compute styles of child spans.
style_cache: HashMap<Vec<Scope>, StyleModifier>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

If hashing shows up any significant amount in the time profile, you can switch this to use a FNV hasher, which is what I use in syntect.

Copy link
Member Author

Choose a reason for hiding this comment

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

@trishume okay, good note, thanks!

@cmyr cmyr merged commit 9f2e163 into xi-editor:master Dec 18, 2017
@cmyr cmyr deleted the fix/scope-perf branch December 18, 2017 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants