Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace dksjon #11

Closed
astoff opened this issue Dec 18, 2019 · 8 comments
Closed

Replace dksjon #11

astoff opened this issue Dec 18, 2019 · 8 comments

Comments

@astoff
Copy link
Owner

astoff commented Dec 18, 2019

Some profiling showed that the language server may spend around 50% of the CPU time inside dkjson's encode routines, and take around 20 ms to encode a large completion response.

We should replace dkjson with a C library, but currently cjson doesn't work reliably in Lua 5.3, see this issue. So we need to wait for a fix or find another native json library.

@elliott-wen
Copy link

I confirmed that by replacing dkjson with cjson, we can get a much smoother experience.

By the way, do you think it would be better if we bundle a lua interpreter with digestif so that users can skip the configuration hassle?

In my setting, I compiled lpeg + cjson + lua together. The binary is rather small like 250Kb in Ubuntu.

@astoff
Copy link
Owner Author

astoff commented Jul 8, 2020

I confirmed that by replacing dkjson with cjson, we can get a much smoother experience.

Did you find this is the case overall, or just for specific LSP requests like completions with lots of candidates? The latter case can be solved more simply, by limiting to 100 items or so.

That said, if you managed to compile cjson and found it to work reliably in the webasm environment, it's easy to make digestif use it when available and fall back to dkjson otherwise. If this works well for you I'll make it so.

By the way, do you think it would be better if we bundle a lua interpreter with digestif so that users can skip the configuration hassle?

I considered this at some point, but now I think the self-installing script, which runs on LuaTeX's own interpreter, is even easier – for the user and also for us, since the distribution remains portable across OSs. Do you see any additional advantage of the bundled stuff?

@elliott-wen
Copy link

elliott-wen commented Jul 8, 2020 via email

@elliott-wen
Copy link

elliott-wen commented Jul 8, 2020 via email

@quicquid
Copy link

quicquid commented Dec 22, 2020

@elliott-wen would you be willing to share your patch? It seems dkjson is (marked as) not compatible with Lua 5.4 (see the rock description) and prevents digestif from being built on my machine (Fedora 33, see below). Another reason to replace dkjson might be that there hasn't been an update to the library in five years - I'm not sure it is maintained at all anymore.

$ luarocks install dkjson 2.5 --local --check-lua-versions
dkjson not found for Lua 5.4.
Checking if available for other Lua versions...
Checking for Lua 5.1...
Checking for Lua 5.2...
Checking for Lua 5.3...

Error: No results matching query were found for Lua 5.4.
dkjson 2.5 supports only Lua 5.1 and Lua 5.2 and Lua 5.3 but not Lua 5.4.

@astoff
Copy link
Owner Author

astoff commented Dec 22, 2020

@quicquid If you just want a workaround for this problem, you can probably edit the dkjson rockspec and install it. Or use the self-installing wrapper mentioned in the README.

@quicquid
Copy link

@astoff thank you, I will givethis a try!

@astoff
Copy link
Owner Author

astoff commented Dec 23, 2020

Ok, since dkjson was the only dependency external to texlua, I decided to replace it with my own json implementation. It's also a bit faster than dkjson since it doesn't need to be compatible with old versions of LPeg. I have also decided that Digestif will never depend on cjson, in part because it does not look so well maintained.

I also made it so that Digestif checks if cjson is present at runtime and uses it if that's the case. However, I couldn't find any way to compile cjson and test this. @elliott-wen If you get the chance to test this, please let me know.

@astoff astoff closed this as completed Dec 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants