feat(transport): bump up msgpack to latest official #136
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
TLDR: THIS PR IS 🚀 , @kwonoj you're my hero Tested the latency with/without the PR with Original neovim-client (Notice around 200-500ms delay for actual grid_lines/grid_cursor_goto notification sometimes!!!! (this is second/third notification after second chord of `gg` or `G`)Original neovim-client: Sending keys to nvim: G Notification: 1.536300003528595ms Notification: 4.614601016044617ms Sending keys to nvim: g Notification: 4.003499984741211ms Sending keys to nvim: g Notification: 1.1575999855995178ms Notification: 0.49390000104904175ms Notification: 3.7296999990940094ms Sending keys to nvim: G Notification: 1.2824990153312683ms Notification: 213.0753009915352ms Sending keys to nvim: g Notification: 2.0127000212669373ms Sending keys to nvim: g Notification: 5.922201007604599ms Notification: 1.8919000029563904ms Notification: 34.609599977731705ms Sending keys to nvim: G Notification: 2.0221999883651733ms Notification: 8.06470000743866ms Sending keys to nvim: g Notification: 1.6171999871730804ms Sending keys to nvim: g Notification: 2.0698999762535095ms Notification: 0.7240000069141388ms Notification: 45.29800000786781ms Sending keys to nvim: G Notification: 1.1888999938964844ms Notification: 11.05979898571968ms Sending keys to nvim: g Notification: 1.2100000083446503ms Sending keys to nvim: g Notification: 1.2712999880313873ms Notification: 0.4941999912261963ms Notification: 37.36149901151657ms Sending keys to nvim: G Notification: 1.1398990154266357ms Notification: 207.17140099406242ms Sending keys to nvim: g Notification: 1.697299987077713ms Sending keys to nvim: g Notification: 1.1644999980926514ms Notification: 0.49470001459121704ms Notification: 92.6773000061512ms Sending keys to nvim: G Notification: 1.6306999921798706ms Notification: 4.451099991798401ms Sending keys to nvim: g Notification: 1.349399983882904ms Sending keys to nvim: g Notification: 1.246099978685379ms Notification: 0.5309000015258789ms Notification: 561.8795000016689ms Sending keys to nvim: G Notification: 3.9097999930381775ms Notification: 4.824500024318695ms Sending keys to nvim: g Notification: 2.1088989973068237ms Sending keys to nvim: g Notification: 1.3113000094890594ms Notification: 0.4814999997615814ms Notification: 6.062199980020523ms Sending keys to nvim: G Notification: 1.3472000062465668ms Notification: 7.442499995231628ms Sending keys to nvim: g Notification: 1.3474999964237213ms Sending keys to nvim: g Notification: 1.8323990106582642ms Notification: 0.48980098962783813ms Notification: 5.080399990081787ms Sending keys to nvim: G Notification: 1.3792009949684143ms Notification: 6.226699978113174ms Sending keys to nvim: g Notification: 1.318800002336502ms Sending keys to nvim: g Notification: 0.9252000153064728ms Notification: 0.4724000096321106ms Notification: 4.589699983596802ms Sending keys to nvim: G Notification: 1.3585000038146973ms Notification: 4.838899999856949ms Sending keys to nvim: g Notification: 3.8021990060806274ms Sending keys to nvim: g Notification: 2.5458000004291534ms Notification: 1.0259990096092224ms Notification: 8.20500099658966ms Sending keys to nvim: G Notification: 1.6672010123729706ms Notification: 55.58770000934601ms Sending keys to nvim: g Notification: 1.2921990156173706ms Sending keys to nvim: g Notification: 1.428600013256073ms Notification: 0.5412999987602234ms Notification: 3.9738999903202057ms Sending keys to nvim: G Notification: 1.7117010056972504ms Notification: 48.479499995708466ms Sending keys to nvim: g Notification: 1.5313000082969666ms Sending keys to nvim: g Notification: 1.4230989813804626ms Notification: 0.5142010152339935ms Notification: 290.420500010252ms Sending keys to nvim: 2 Notification: 1.3240990042686462ms Sending keys to nvim: 0 Notification: 1.3149999976158142ms Sending keys to nvim: 0 Notification: 1.444599986076355ms Sending keys to nvim: 0 Notification: 1.3019990026950836ms Sending keys to nvim: G Notification: 1.4440999925136566ms Notification: 588.0019000172615ms PR without WASM=forceNo latency problems PR with WASM=forceSeems almost equal to without WASM. I'll try to repeat later same test with larger file and +scrolling (wasm mode should perform better for large payloads, but since nvim width/height is restricted anyway, it may not affect it much) Small issue (not sure but may be something on my side actually), when running the extension in debug mode client initially spams ~30 messages with: |
|
@kwonoj thanks! And @asvetliakov thanks for validating with concrete numbers. I raised a similar concern in #78 . Waiting for comments by @neovim/nodejs , otherwise ping me in 1-2days and I'll merge this. |
rhysd
reviewed
Dec 30, 2019
|
@neovim/nodejs @justinmk Since this PR introduces big change under the hood, I feel one more review and approval would be better. Would you review this? |
@rhysd what big changes do you mean (example)? Since it passes tests I assume/hope it didn't change API at least. And it was verified by at least one user, which is great. Perhaps we should cut a release, then merge this and release it as a major-version bump? @billyvg |
I'm sorry I did not have exact example. But I was not confident to replace underlying encoding/decoding library. I believe this introduce no API change so I said 'under the hood'. Yeah, it is great that already one user confirmed this. |
|
I meant, I'm not fully understanding the difference of behavior between msgpack-lite and @msgpack/msgpack. If it has some difference which affects user's code, we should bump up major version, otherwise bumping minor version is enough, I believe. |
|
I’m currently traveling but this looks good to me. This doesn’t seem to affect any apis, so a major version bump shouldn’t be necessary. |
|
|
Wonder if we can fix this. It spams the VS Code Debug Console for example, if an extension is using this cilent. |
|
i've disabled winston logger in vscode-neovim, but it still spams few messages. The cause is the transport spawns node-client/packages/neovim/src/utils/transport.ts Lines 68 to 71 in 2cb74f9 |
Hmm is that because the logger isn't useful or...? |
Yes, i don't feel any need in low-level debug messages |
@asvetliakov can you point to the code in vscode-neovim where you disable it? Does node-client need to provide a way to disable the logger such that these spam messages are avoided? These messages show up in the vscode Debug Console for anyone that has vscode-neovim client installed, if one is developing/debugging a (unrelated) vscode extension. |
|
@justinmk here: The left spam is going through |
|
ok so by "disable" you mean rather you set the log-level to "error"? Trying to make sure I understand.
That sounds like a workaround, I'm looking for a way to fix this for all vscode-neovim (or node-client) users by default. |
Oh, yes, sorry for confusion 😅 . btw, spam in the vscode console is debug-level, so setting logger level to |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR attempts to bump up msgpack from
msgpack-liteto latest official (https://github.com/msgpack/msgpack-javascript).msgpack-lite has not been updated for amount of time, meanwhile official bindings improved implementation, as well as some experimental perf improvement approach as well like wasm backend (https://github.com/msgpack/msgpack-javascript/blob/c969bc9045a2fc2e70cf3bddd7ccd2e71cff1a78/src/wasmFunctions.ts#L3, still experimental though)
to conform
@msgpack's apitransport's internal implementation has changed, but does not exposes any breaking changes so far.