feat(lsp): improve json deserialization performance

[mjlbach]

closes: #15109

We currently convert vim.NIL to nil in tables in a lua loop. This is a hot loop and probably the single biggest remaining bottleneck for the lsp client. This pushes nil instead of vim.NIL in the cjson conversion function which removes the bottleneck.

[mjlbach]

@hrsh7th nevermind, try this one :)

[mjlbach]

I just hardcoded this for now, but we can add a second optional arg, or maybe a second json_decode_drop_nil function instead.

[hrsh7th]

I will try it! Sorry for late.

[mjlbach]

I want to know if this is the right approach for optional args/how people feel about the interface/naming:

vim.json.decode(body) -- this retains the default lua-cjson behavior with vim.NIL instead of cjson.NULL
vim.json.decode(body, {table_use_luanil=true}) --this is what we use for LSP
vim.json.decode(body, {table_use_luanil=true, array_use_luanil=false}) -- this is the same as the above, both args default to false

We don't have to have array_use_luanil, but I figured if I was adding the option why not, because it has symmetry with the other (I don't think there should be a perf impact).

[justinmk]

I want to know if this is the right approach for optional args/how people feel about the interface/naming:

vim.json.decode(body, {table_use_luanil=true, array_use_luanil=false}) 

The verbose names can be avoided by using a single tuple/enumish param (which is usually more ergonomic that a bunch of booleans, anyway--also less awkward to document, which again is a signal of a better interface...):

vim.json.decode(body, {luanil={'table','list'}})

• Does "table" here mean "map"/"dict", or does it mean "Lua table which could be dict-like or list-like"?
• I think "list" is generally what we call non-dict tables? (Not really sure why we have tbl_islist() but its doc calls the thing "array". Sigh.)

[mjlbach]

The verbose names can be avoided by using a single tuple/enumish param (which is usually more ergonomic that a bunch of booleans, anyway--also less awkward to document, which again is a signal of a better interface...):

If you like it better this way, I'll change it, I think the way it current is, is a bit more unambiguous because we aren't going to have more than 2 options for this, but I don't have a strong opinion.

Does "table" here mean "map"/"dict", or does it mean "Lua table which could be dict-like or list-like"?

A json object maps onto a lua table, and a json array maps onto a lua list. Happy to change the name, but I meant the names to be json centric and match as close as possible the implementation in cjson. In lua, I would call it a list.

[justinmk]

, is a bit more unambiguous because we aren't going to have more than 2 options for this

Not really worried about the number options, just (1) elegant documentation, (2) ergonomics, (3) verbosity.

Multiple, related, boolean options is clumsy. They are related, but the reader/user has to figure it out, by reading more documentation or inspecting all the param names. A single option implicitly signals that "luanil" is a topic with X number of forms.

A json object maps onto a lua table, and a json array maps onto a lua list. Happy to change the name, but I meant the names to be json centric and match as close as possible the implementation in cjson.

wouldn't json centric be luanil={'object','array'} ?

[mjlbach]

Not really worried about the number options, just (1) elegant documentation, (2) ergonomics, (3) verbosity.... Wouldn't json centric be luanil={'object','array'} ?

Thanks for the input :) I'll go ahead and change it to follow your suggestion with object instead of table as well.

[hrsh7th]

I've tried this. I can't see the cursor lags anymore with native omnifunc!
@mjlbach did the awesome work! Thank you!

[mjlbach]

Addressed review (see above note about runtime vs module initialization with config vs. option) and squashed the commits.

I think I prefer something closer to my original implementation for the interface (option 1) which is option 2 below:

Option 1: vim.json.decode(text, { luanil_object = true, luanil_array = false })
Option 2: vim.json.decode(text, { luanil = {object = true, array = false }})
Option 3: vim.json.decode(text, { luanil = { "object" })

I changed it to option 2.

[mjlbach]

@tjdevries and I were talking about this, I added handling for if luanil isn't passed https://github.com/neovim/neovim/pull/15854/files#diff-770e34bd301f419ac67cceed5626e9e6818d933bf5ba20b453d1530fe1d2b4d3R1419-R1423

Otherwise, I'd like to merge this tonight.

[tjdevries]

LGTM. Looking forward to how speedy this will be :)

(we chatted on discord a bit about this -- but I'll just mention that I prefer the { luanil = { object = true } } pattern over the array of items, mostly for me because it will be easier to do something like:

opts.luanil.object = true
-- or
opts.luanil.object = false

and override, rather than writing something to iterate over the list and remove or add stuff to the list.)