Refactor throttle messaging

[joshuachp]

Reopened PR #42

[joshuachp]

Sorry, there were some problems with the branch being called refactor and the new having the same leading directory.

Yes, this is more expensive than a fixed, known number of concatenations, but is more convenient and more efficient than concatenating in a loop (ref: https://gitspartv.github.io/LuaJIT-Benchmarks/#test18).

Thanks, this is good to know. I steal believe that the insert and new allocation are more expensive since in the example they use a fixed array, but reading those tests the change wouldn't probably be worth it.

One suggestion, though I haven't gone through the changes in depth yet: what if, instead of a constantly running timer, we test if the time since last update is within one interval-length of the current time, and issue a redrawstatus! if it is not? That would simplify the flow of the code a bit, and potentially save on computation (though probably not beyond the level of a micro-optimization).

I have tried with an interval, but Lua doesn't implement a way to get the time in milliseconds. Additionally, the current implementation leverages well the functions provided by the vim/libuv event loop.

The timer only checks the vim.b.lsp_status_redraw variable for an update before calculating the text and only redraws the statusline if needed, so I don't think there could be an optimization there. I'm happy to know if there is something that I'm missing since my knowledge of Lua is limited.

Finlay, the timer is started when on_attach and could be stopped when there is no active LSP client. So it's running only when there is something to show on the statusline.

[wbthomason]

Sorry, there were some problems with the branch being called refactor and the new having the same leading directory.

No worries, thanks for the change!

I steal believe that the insert and new allocation are more expensive since in the example they use a fixed array

Your results may vary, but I tested this on my system and found that using concat is still roughly 5x faster than using .. in a loop, even including the time required to build the table. Regardless, this is probably moot - the concat code is cleaner. I do have some ideas for rewriting this loop around string.format, which may provide the best of both performance and clean code, but I think that's separate from this PR.

If you're interested, here's the script I used, adapted from the linked benchmark page:

```lua local bs = string.rep("----------", 1000) local concat = table.concat local function naive_concat() local naive_times = {} for _ = 1, 100 do local START = os.clock() for _ = 1, 1000 do local s = bs for i = 1, 9 do s = s .. bs end end local END = os.clock() naive_times[#naive_times + 1] = END - START collectgarbage() end return naive_times end print('Starting naive') local naive_times = naive_concat() print('Done naive') local function tbl_concat() local concat_times = {} for _ = 1, 100 do local START = os.clock() for _ = 1, 1000 do local t = {} for i = 1, 10 do t[#t + 1] = bs end concat(t) end local END = os.clock() concat_times[#concat_times + 1] = END - START collectgarbage() end return concat_times end print('Starting concat') local concat_times = tbl_concat() print('Done concat') local naive_avg = 0 local concat_avg = 0 for i = 1, 100 do naive_avg = naive_avg + naive_times[i] concat_avg = concat_avg + concat_times[i] end naive_avg = naive_avg / 100 concat_avg = concat_avg / 100 print('Naive: ' .. naive_avg) print('Concat: ' .. concat_avg) ```

I have tried with an interval, but Lua doesn't implement a way to get the time in milliseconds. Additionally, the current implementation leverages well the functions provided by the vim/libuv event loop. The timer only checks the vim.b.lsp_status_redraw variable for an update before calculating the text and only redraws the statusline if needed, so I don't think there could be an optimization there. I'm happy to know if there is something that I'm missing since my knowledge of Lua is limited. Finlay, the timer is started when on_attach and could be stopped when there is no active LSP client. So it's running only when there is something to show on the statusline

I generally agree - viewed as an optimization, what I propose is probably not worth it. However, I prefer using an interval check more for clarity of control flow than optimization. With the current implementation, there's the message handler, which can (via b.lsp_status_redraw trigger the (separately running) timer to generate text, which then triggers redraw! to force the statusline to redraw. With what I'm proposing, there's only the message handler, which - if it has been more than interval since the last redraw - simply triggers redraw!, leaving it to the statusline to generate the text it wants and optionally cache it in a variable (though I'm not strongly opposed to having the message handler call text generation and caching too, if we feel that most people will want this behavior). To my mind, this is much simpler (and may be very slightly more performant).

If you'd like, I can push some changes to this branch implementing what I'm suggesting, for you to review.

Lua doesn't implement a way to get the time in milliseconds

Fortunately, luv does.

[joshuachp]

Today I learned. Thanks for correcting me 😄, the benchmark speaks for it self. Surprisingly It works even with the Lua interpreter.

I am happy to leave this PR, please note that with the interval an update can get stuck and not be shown. If the message arrives and it not enough time has passed it will never show. This method can solve the lag problem but maybe it's not the best answer.

On the other hand the use of a timer is the same method used by coc.nvim. I don't believe my implementation is the best, but it solves the lag issue and can be improved upon. Maybe an idea could be to have a Timer or Statusbar class instance shared between the modules and we can ditch the vim.b.redraw variable.

[wbthomason]

please note that with the interval an update can get stuck and not be shown

Fair point.

Maybe an idea could be to have a Timer or Statusbar class instance shared between the modules and we can ditch the vim.b.redraw variable

Yeah, this might be the best option. That said, I think we probably merge this implementation for now to fix #36.

Could you tell me how you have lsp-status integrated into your statusline? I'm trying the PR branch, and I get no messages output.

[joshuachp]

I'm using lualine and directly showing the variable b:lsp_status_statusline, you can also use the require('lsp-status/statusline').status() function which is just a wrapper function for compatibility.

The documentation should probably be updated, but the plugin is still works the same way.

[wbthomason]

That's what I thought - the problem is, it doesn't work for me like that (I use the status function in my statusline). I get zero messages displayed.

I suspect this is an issue with the message being overwritten before it's displayed by my statusline (which is part of why the original design just issued redrawstatus! and let the statusline decide to get the text/why I proposed the interval implementation instead).

I'll see if I can debug it on my end.

[joshuachp]

My bad, I made the mistake of returning the variable from vim.g instead of vim.b, thanks for catching it.

[nerosnm]

After testing this for a few hours, this does fix #36 for me, thanks very much @joshuachp! And I'm able to display the status in my statusline correctly in the same way as usual, using Lightline.

[wbthomason]

Yup, I've had a chance to test this now, and I'm good to merge it. Might look at simplifying the control flow in a later PR.

Thanks, @joshuachp!

[andrewrynhard]

👋🏻 Hello. After this change my status bar is broken. Looks like :lua print(require('lsp-status').status()) returns nothing. There is a client, :lua print(#vim.lsp.buf_get_clients()) outputs 1. Reverting to the previous commit, and everything works again.

[wbthomason]

@andrewrynhard Which server are you using? I've tested this change with pyright, sumneko lua, JuliaLS, and rust-analyzer, and had everything work as expected.

[andrewrynhard]

@andrewrynhard Which server are you using? I've tested this change with pyright, sumneko lua, JuliaLS, and rust-analyzer, and had everything work as expected.

rust-analyzer and sumneko lua

[wbthomason]

Hmm, interesting. Would you mind (1) making sure you're on the latest commit (I pushed a fix related to this change and multiple buffets, which might help) and (2) linking your config, if you're still not seeing output?

[andrewrynhard]

Hmm, interesting. Would you mind (1) making sure you're on the latest commit (I pushed a fix related to this change and multiple buffets, which might help) and (2) linking your config, if you're still not seeing output?

[I] pwd
~/.local/share/nvim/site/pack/packer/start/lsp-status.nvim
[N] git log -n1
commit af54cfc2542fa9bbf7b922828840b0bb200d9d38 (HEAD -> master, origin/master, origin/HEAD)
Author: Wil Thomason <wil.thomason@gmail.com>
Date:   Sat Mar 27 15:22:11 2021 -0400

    Run lua-format

local lspconfig = require "lspconfig"
local lsp_status = require("lsp-status")
local completion = require "completion"

-- Configure the capabilities.
local capabilities = lsp_status.capabilities
capabilities.textDocument.completion.completionItem.snippetSupport = true

-- Configure the on_attach.
local on_attach = function(client) completion.on_attach(client) end

-- Enable diagnostics
vim.lsp.handlers["textDocument/publishDiagnostics"] =
    vim.lsp.with(vim.lsp.diagnostic.on_publish_diagnostics, {
        underline = true,
        virtual_text = true,
        signs = true,
        update_in_insert = true
    })

-- Configure lsp-status.
lsp_status.config {
    kind_labels = {},
    current_function = true,
    indicator_separator = ' ',
    indicator_errors = 'E',
    indicator_warnings = 'W',
    indicator_info = 'I',
    indicator_hint = 'H',
    indicator_ok = '✔️',
    spinner_frames = {'⣾', '⣽', '⣻', '⢿', '⡿', '⣟', '⣯', '⣷'},
    status_symbol = '',
    select_symbol = nil
}

-- Register the progress handler with Neovim's LSP client. Call once before
-- starting any servers.
lsp_status.register_progress()

-- See https://github.com/neovim/nvim-lspconfig/blob/master/CONFIG.md for a
-- list of available language servers.

-- Enable Lua

lspconfig.sumneko_lua.setup({
    on_attach = on_attach,
    cmd = {
        "/usr/local/bin/lua-language-server/bin/Linux/lua-language-server",
        "-E", "/usr/local/bin/lua-language-server/main.lua"
    },
    settings = {
        Lua = {
            completion = {keywordSnippet = "Disable"},
            diagnostics = {
                globals = {"vim", "use"},
                disable = {"lowercase-global"}
            },
            runtime = {version = "LuaJIT", path = vim.split(package.path, ";")},
            workspace = {
                library = {
                    [vim.fn.expand("$VIMRUNTIME/lua")] = true,
                    [vim.fn.expand("$VIMRUNTIME/lua/vim/lsp")] = true
                }
            }
        }
    }
})

-- Enable gopls

lspconfig.gopls.setup({capabilities = capabilities, on_attach = on_attach})

-- Enable rust-analyzer

lspconfig.rust_analyzer.setup({
    capabilities = capabilities,
    on_attach = on_attach,
    settings = {
        -- See https://rust-analyzer.github.io/manual.html#configuration.
        ["rust-analyzer"] = {
            assist = {importMergeBehavior = "full", importPrefix = "by_crate"},
            checkOnSave = {extraArgs = {"--target-dir", "/tmp/rust-analyzer"}},
            cargo = {
                -- allFeatures = true,
                loadOutDirsFromCheck = true
            },
            procMacro = {enable = true}
        }
    }
})

function LspStatus()
    if #vim.lsp.buf_get_clients() < 1 then return "" end

    return lsp_status.status()
end

[joshuachp]

You need to register the lsp-status on_attach to initialize the timer.

Before the autocmd where registered there, I think it's strange that it was working without it.

[andrewrynhard]

You need to register the lsp-status on_attach to initialize the timer.

Before the autocmd where registered there, I think it's strange that it was working without it.

🤦🏻 yep:

-local on_attach = function(client) completion.on_attach(client) end
+local on_attach = function(client)
+  completion.on_attach(client)
+  lsp_status.on_attach(client)
+end

Works.