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

Move contexts from SyntaxSet into individual SyntaxReferences #382

Merged
merged 3 commits into from
Nov 22, 2021

Conversation

Enselic
Copy link
Collaborator

@Enselic Enselic commented Oct 18, 2021

This is in preparation of lazy-loading syntaxes on-demand. I think this is the most complicated change of the bunch.

Overview of the syntax lazy-loading work:

This is in preparation of lazy-loading syntaxes on-demand.
@Enselic
Copy link
Collaborator Author

Enselic commented Oct 18, 2021

The performance impact of this change is ~5% slower deserialization of SyntaxSet. Measured with cargo bench load_internal_dump. This will of course be compensated for several times over in upcoming PRs that introduces lazy-loading.

before:

load_internal_dump      time:   [34.414 ms 34.446 ms 34.485 ms]                              
                        change: [-0.4719% -0.2492% -0.0508%] (p = 0.02 < 0.05)
                        Change within noise threshold.

after:

load_internal_dump      time:   [36.373 ms 36.406 ms 36.451 ms]                              
                        change: [+5.5424% +5.6902% +5.8790%] (p = 0.00 < 0.05)
                        Performance has regressed.

@Enselic
Copy link
Collaborator Author

Enselic commented Oct 18, 2021

As you can see, all syntect regression tests pass.
Here is proof all bat regression tests pass with this change: Enselic/bat#51

So risk of behavioral regressions from this PR is low. I would actually go as far as to say "very low".

Copy link
Owner

@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.

Looks good! Just one thing which isn't a big deal.

src/parsing/syntax_definition.rs Outdated Show resolved Hide resolved
@trishume
Copy link
Owner

Also I've added you as a collaborator to the repo to recognize you for your valuable contributions. Your PRs have consistently been high-quality, well-explained and well-tested. I also added you to the major contributors listed in the Readme (540555e). Thanks so much, sorry I've been so lazy about reviewing!

What does this mean:

  • If you want to review other people's PRs, I'll trust your judgement and just quickly skim them, which will also make me more likely to merge promptly.
  • I'd still prefer to review PRs before merging them, but if I'm ever MIA for at least a month you may review PRs and merge them if you like. You may need to ping me for crates.io deploys or to get crates.io access if this happens though.

It is better to not compile than to crash later. And very few if any clients are
likely to actually use this API. See
trishume#382 (comment)
@Enselic
Copy link
Collaborator Author

Enselic commented Oct 25, 2021

Also I've added you as a collaborator to the repo to recognize you for your valuable contributions. Your PRs have consistently been high-quality, well-explained and well-tested. I also added you to the major contributors listed in the Readme (540555e). Thanks so much, sorry I've been so lazy about reviewing!

Thank you! I really appreciate that. On many levels. Don't worry too much about it sometimes taking some days before you get around to doing reviews. It really isn't that bad. I am grateful for that you still are nurturing this important and great project.

A commit to fix your only comment has been pushed.

Enselic added a commit to Enselic/syntect that referenced this pull request Oct 25, 2021
It is better to not compile than to crash later. And very few if any clients are
likely to actually use this API. See
trishume#382 (comment)
@Enselic Enselic requested a review from trishume October 27, 2021 11:54
@Enselic Enselic mentioned this pull request Nov 9, 2021
@Enselic
Copy link
Collaborator Author

Enselic commented Nov 17, 2021

@trishume Hi! Just wanted to let you know we are closing in on this being in review for a month. In light of #382 (comment) I will go ahead and merge it after a month has passed.

Let me know if you want more time, that would be absolutely no problem at all :) And of course feel to ask me about anything related to this (or any other) PR!

@trishume
Copy link
Owner

Thanks for the ping and sorry for the delay. It's been a busy time in my life including some traveling recently. I may have time to do some review Sunday but if I don't then I trust you to do a good job and am happy with you just merging it.

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.

None yet

2 participants