feat(lua): expose lua-cjson as vim.json #14871
Conversation
github-actions
bot
added
the
build
|
I'm curious what the rationale for this is. What does it offer that |
|
In my knowledge, vim.fn.json_encode/json_decode needs |
|
@hrsh7th is correct, I'd also like to add in my benchmark lua-cjson was able to decode the same file in ~%30 of the time. |
|
And what is the rationale to embed / vendor the lua_cjson code? Is it just for testing? Is it similar to treesitter which was first vendored, and after it matured a bit outsourced again to be able to use the upstream version? |
|
To make it work directly with our lua mpack typesystem, i e |
|
I'm looking into handling NULL and empty tables correctly (according to our conventions). |
|
Added to interpreter to |
clason
added
dependencies
This comment has been minimized.
This comment has been minimized.
|
How feasible is it to remove the old Vimscript implemenation of |
|
I'll pick this PR back up this weekend. It's feasible but we should also consider benchmarking it to see if the conversion cost to vim data structures is high. |
|
I rebased, cleaned up some conflicts, and moved the source of cjson as requested. I'll squash the commits when everything is ready. I guess tests are left, and silencing some of the missing/strict prototype warnings. |
|
I silenced the warnings (not sure if this was the cleanest way) and added some basic tests. |
|
|
Yes, once people feel this is in a mergeable state I will clean up the commit history. There will be two commits:
|
mjlbach
changed the title
|
I need to add the real benchmarks, but I cleaned up the commits (let me know if this is consistent with what you wanted, each commit should build). There's still the open warnings from fpconv to take care of, and Justin had some concerns about a circular dependency, but I pointed out where the included functions are used. |
test/functional/lua/json_spec.lua
Outdated
|
|
||
| eq( | ||
| { hello = 'world'}, | ||
| exec_lua([[return vim.json.decode('{"hello":"world"}')]]) |
There was a problem hiding this comment.
(not a blocker, but at least mentioning for reference...)
i kinda assumed more tests were on the way. Maybe copy a few from json_functions_spec.lua ?
Or parameterize (most of) the tests in json_functions_spec.lua, then in this file, something like:
local json_spec = require('test.functional.json_functions_spec')
local json_encode = function(o)
exec_lua([[return vim.json.encode(...)]], o)
end
local json_decode = function(o) ... end
json_spec.run(json_encode, json_decode)
There was a problem hiding this comment.
Looking through the tests, I think parameterizing them is going to be non-trivial, seems like many of them deal with vimscript specific behavior, or eval-ing a string representation of the vim command.
justinmk
changed the title
Derived from the openresty lua-cjson fork at commit openresty/lua-cjson@3d93d29
* add vim.json.encode and vim.json.decode
* use vim.NIL instead of cjson.null
* resolve strict-prototypes warnings
* The following benchmark shows an approximately 2.5x (750 ms vs 300 ms) improvement in deserialization performance over
vim.fn.json_decode on a medium package.json
```lua
local uv = vim.loop
local function readfile(path)
return
end
local json_url = "https://raw.githubusercontent.com/rust-analyzer/rust-analyzer/b24c8d5c89ee93d1172b4127564f5da3b0c88dad/editors/code/package.json"
io.popen(string.format('curl -v -f -L -O %q &> /dev/null', json_url))
local json_string = io.open('package.json'):read '*a'
uv.update_time()
local start = uv.hrtime()
for i = 1,1000 do
vim.fn.json_decode(json_string)
end
uv.update_time()
print(string.format("Deserialization time vim.fn.json_decode: %s ms", (uv.hrtime() - start) * (1e-6)))
uv.update_time()
local start = uv.hrtime()
for i = 1,1000 do
vim.json.decode(json_string)
end
uv.update_time()
print(string.format("Deserialization time vim.json.decode: %s ms", (uv.hrtime() - start) * (1e-6)))
```
Co-Authored-By: Björn Linse <[email protected]>
|
Added the benchmark, added some tests from json functional test. Ready for review and final comments. The commit lint just doesn't like the embedded URL which is over 100 chars long. |
|
Functional test failure: It doesn't like the |
|
@mjlbach I took the liberty of fixing the test with the minimal (whitespace) change. (I did what I had to, because I could!) Hope you don't mind; I assumed you'd squash the |
|
I actually wasn't planning on squashing the test + lua-cjson commits together, but keeping them separate to delineate the c-side changes/modifications. I just squashed the test commit together so if there aren't any objects I'll merge this after @bfredl or @justinmk approves. I think it's ok to ignore the commit linter in this case. |
|
otherwise LGTM :) |
This adds a direct to/from lua json encode/decode using lua-cjson. There are performance benefits to having direct conversion from json to the lua datastructures, and this avoids any issue calling in luv callbacks.