Switch to using the rope-utf16-splay library for ropes #70
Conversation
test/VspSpec.hs
Outdated
| ] | ||
| new = applyChange (Yi.fromString orig) | ||
| $ J.TextDocumentContentChangeEvent (mkRange 1 0 1 3) (Just 3) "𐐀𐐀" | ||
| lines (Yi.toString new) `shouldBe` |
There was a problem hiding this comment.
Yi.toString new
Does this compile?
There was a problem hiding this comment.
Thanks, I must have messed up a rebase. Fixed!
|
Could you enable CircleCI for your fork? |
|
Also how does this compare with yi-rope on memory usage? |
ollef
force-pushed
the
rope
branch
3 times, most recently
from
745732e
to
f9559d5
Compare
|
@wz1000: CI enabled. I have yet to run any memory benchmarks but it uses the strategy of merging adjacent chunks if they're below a certain size just like Yi.Rope, so it should be very similar in terms of memory usage. |
|
@MaskRay: Can you explain the relevance of that link to this PR? I agree with it, but as far as I can tell that change hasn't been made yet and it seems uncertain when or if it will be. |
|
I kind of forgot about it. I'll get the library updated and conflicts fixed when I have time. I'd also be interested in hearing about reservations, e.g. if any of the reverse dependencies are strongly committed to using |
|
I want to first sort out haskell/haskell-ide-engine#538 before landing this, do not want to confuse possible sources of problems. |
|
That's fair and there's no rush from my end. Feel free to ping me when the time's right! :) |
|
Seems like haskell/haskell-ide-engine#538 has been closed now. Would be good to get this merged I think. |
* This makes the licensing story simpler because rope-utf16-splay is BSD3 licensed. (haskell#16) * LSP uses UTF-16 code unit based indexing, while the Yi rope library indexes by characters. This means that haskell-lsp would previously not correctly count characters that are encoded as two code units in UTF-16. The rope-utf16-splay library uses code unit indexing, which fixes this issue. * From my benchmarks, this new library, which uses splay trees internally, can be about twice as fast as finger tree based ones (like the Yi one) for use cases that are similar to what haskell-lsp might be doing (many consecutive modifications).
This tests indexing around characters encoded as two code units in UTF-16.
|
I've rebased this on master now. |
|
Given microsoft/language-server-protocol#376 and https://github.com/Avi-D-coder/lsp-range-unit-survey we should perhaps hold off until we know if anything is going to change. |
|
Could I suggest this gets merged sooner rather than later? Anyone hoping to build on haskell-lsp in a BSD-only context is blocked without it (which is me 😄 - see https://github.com/digital-asset/haskell-lsp and 1509628 which is based on this patch). It seems from the links above that this is the right approach today. If it stops being the right approach that seems the right time to go off in a different direction. |
|
Ok, I guess we can do it, and it is early in the month, so the bleeding edger's can pick up any problems that arise. |
BSD3 licensed. (Commercial user GPL cleanup #16)
indexes by characters. This means that haskell-lsp would previously not
correctly count characters that are encoded as two code units in
UTF-16. The rope-utf16-splay library uses code unit indexing, which
fixes this issue.
internally, can be about twice as fast as finger tree based ones (like
the Yi one) for use cases that are similar to what haskell-lsp might be
doing (many consecutive modifications).