OhMyREPL.jl icon indicating copy to clipboard operation
OhMyREPL.jl copied to clipboard

Run OhMyREPL keybindings in a fixed world age

Open c42f opened this issue 2 years ago • 8 comments

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.

c42f avatar Apr 27 '23 10:04 c42f

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.

jakobnissen avatar Apr 27 '23 12:04 jakobnissen

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.

c42f avatar Apr 27 '23 20:04 c42f

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
@@            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 Coverage Δ
src/BracketInserter.jl 1.09% <ø> (-20.02%) :arrow_down:
src/hooks.jl 0.00% <0.00%> (ø)
src/repl.jl 0.00% <0.00%> (-13.13%) :arrow_down:
src/OhMyREPL.jl 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.

codecov-commenter avatar May 18 '23 21:05 codecov-commenter

Good to go?

KristofferC avatar May 18 '23 21:05 KristofferC

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 avatar May 18 '23 22:05 c42f

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

c42f avatar May 18 '23 22:05 c42f

Ugh this has gotten stale/conflicted.

I've fixed the conflicts and pushed again.

It would be nice to merge this? it contains improvements to precompilation, along with the invoke_in_world stuff.

c42f avatar Aug 09 '24 01:08 c42f

Windows nightly test failures are probably due to https://github.com/JuliaDebug/TerminalRegressionTests.jl/pull/14 ?

c42f avatar Aug 09 '24 01:08 c42f