KristofferC / OhMyREPL.jl

Syntax highlighting and other enhancements for the Julia REPL
https://kristofferc.github.io/OhMyREPL.jl/latest/
Other
746 stars 57 forks source link

Run OhMyREPL keybindings in a fixed world age #321

Open c42f opened 1 year ago

c42f commented 1 year ago

Hopefully this should avoid most invalidations caused by loading other packages as discussed in https://github.com/JuliaLang/JuliaSyntax.jl/issues/252.

Also refactor/replace the NEW_KEYBINDINGS global as it's hard to follow where this is added to, and surface a bunch of other uses of global variables like Base.active_repl up to the top level init.

I'm not sure whether there's some additional wrapping which is needed around the pirating in refresh_lines.jl, etc?

Seems to fix #316 ?

@jakobnissen You've used OhMyREPL a lot more than me, perhaps you could give this a test drive? I'm not entirely sure of all the ways I should be trying to test this.

jakobnissen commented 1 year ago

This PR indeed removes all (noticable) latency even when running with JuliaSyntax 0.3.4 (before Kristoffer's recent type-stability improvements to JuliaSyntax) and importing ProgressLogging.

When using the latest JuliaSyntax, loading ProgressLogging still leads to 277 invalidations before your PR, and 253 with your PR. In either case, latency is not noticable even when the invalidated code is in the REPL - maybe I just can't figure out how to trigger the invalidated code to recompile.

Still, from a latency viewport this is a minor win. If there a no/few downsides, it looks good. If there are any downsides to running in a fixed world age, perhaps a better solution might be to make REPL.jl more type stable.

c42f commented 1 year ago

loading ProgressLogging still leads to 277 invalidations before your PR, and 253 with your PR

Right. This doesn't prevent invalidations in the latest world. But running the key bindings in an older world avoids the compiler having to do work to deal with them. So latency should be much improved.

codecov-commenter commented 1 year ago

Codecov Report

Patch coverage: 1.69% and project coverage change: -5.22 :warning:

Comparison is base (0d60955) 56.88% compared to head (b360ca4) 51.67%.

:exclamation: Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #321 +/- ## ========================================== - Coverage 56.88% 51.67% -5.22% ========================================== Files 15 13 -2 Lines 1111 1134 +23 ========================================== - Hits 632 586 -46 - Misses 479 548 +69 ``` | [Impacted Files](https://app.codecov.io/gh/KristofferC/OhMyREPL.jl/pull/321?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kristoffer+Carlsson) | Coverage Δ | | |---|---|---| | [src/BracketInserter.jl](https://app.codecov.io/gh/KristofferC/OhMyREPL.jl/pull/321?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kristoffer+Carlsson#diff-c3JjL0JyYWNrZXRJbnNlcnRlci5qbA==) | `1.09% <ø> (-20.02%)` | :arrow_down: | | [src/hooks.jl](https://app.codecov.io/gh/KristofferC/OhMyREPL.jl/pull/321?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kristoffer+Carlsson#diff-c3JjL2hvb2tzLmps) | `0.00% <0.00%> (ø)` | | | [src/repl.jl](https://app.codecov.io/gh/KristofferC/OhMyREPL.jl/pull/321?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kristoffer+Carlsson#diff-c3JjL3JlcGwuamw=) | `0.00% <0.00%> (-13.13%)` | :arrow_down: | | [src/OhMyREPL.jl](https://app.codecov.io/gh/KristofferC/OhMyREPL.jl/pull/321?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kristoffer+Carlsson#diff-c3JjL09oTXlSRVBMLmps) | `19.35% <5.26%> (-1.40%)` | :arrow_down: |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

KristofferC commented 1 year ago

Good to go?

c42f commented 1 year ago

I've updated this to also fix the world age of the pirated LineEdit.refresh_line, and to precompile the implementations of all pirated methods. This should reduce latency a little.

c42f commented 1 year ago

Oh I didn't see your comment. Yes, I think it's good to go now :-)