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

Implement remaining Lua runtime libraries #1075

Open
wants to merge 26 commits into
base: main
Choose a base branch
from

Conversation

kevin-montrose
Copy link
Contributor

@kevin-montrose kevin-montrose commented Mar 7, 2025

Changes:

  • Allowlist for imported functions (LuaAllowedFunctions)
  • cjson.encode and cjson.decode
  • bit.* functions
  • math.* functions
  • cmsgpack.pack and cmsgpack.unpack
  • struct.pack, struct.unpack, and struct.size
  • Removing a bunch of default included functions that are in Lua 5.4 but not Lua 5.1

TODOs:

  • Tests for the weirder math.* functions
  • Test that configs cannot include unintended functions
  • Tests for deeply recursive (de)serialization
  • Test for Invalid lua can trigger assertion and affect future command #1079
  • Fix various TODOs where we allocate List<byte> or byte[] instances and instead use ScratchBufferManager
  • Crawl though issues for everything this closes
  • Benchmarks

While large, this is basically rote as it's just filling it a bunch of missing functions.

Note that getfenv and setfenv remain unimplemented, as they're Lua 5.1-isms that would be very difficult to implement and are rarely used. Users can adapt their scripts to use load instead as a workaround.

LuaAllowedFunctions

This controls the set of global functions available to a Lua script, including functions grouped under redis.

This is mostly a defense in depth thing for when Garnet is hosted as a service, allowing problematic functions to be disabled without code changes.

cjson

Serialization is bespoke because of the need to handle data passed on the Lua stack.
Deserialization uses .NET's built-in JsonNode.

cmsgpack

Rather than pull in a dependency, I implemented both serialization and deserialization.

The format isn't too complicated and serialization would be bespoke for the same reasons as cjson.encode, so it seemed like a reasonable decisions.

Linux test skips

Some of these tests are finally reproducing the longjmp issue the .NET folks warned about, and failing pretty reliably. For these tests, I'm skipping the exception parts on not-Window with a TODO for the next phase.

Benchmarks

main is as of 225b2ae13c37e9583b752e068a4058b603f69c0a
luaRuntimeLibraries is as of 225b2ae13c37e9583b752e068a4058b603f69c0a

Just reporting LuaRunnerOperations (and just Native memory management) as that's where the impact is expected to be largest.

TL;DR - Construction costs are up, as expected, since the loader block is much larger now. Biggest relative hit is in ConstructSmall where the loader block dominates, and is ~34%. Compilation is somewhat improved (~20%), as more of the loader runs at construction time.

I can picture a way to speed this up (basically, keeping some LuaStateWrappers pre-initialized and having sessions obtain them on demand), but I'm not sure it's worth the complexity.
main

Method Params Mean Error StdDev Median Allocated
ResetParametersSmall Native,None 6.660 us 0.5793 us 1.653 us 6.450 us 688 B
ResetParametersLarge Native,None 6.585 us 0.6188 us 1.785 us 6.200 us 1024 B
ConstructSmall Native,None 297.882 us 16.8782 us 48.427 us 282.100 us 856 B
ConstructLarge Native,None 295.097 us 14.0580 us 40.560 us 276.750 us 4256 B
CompileForSessionSmall Native,None 38.819 us 1.7545 us 4.891 us 37.700 us 736 B
CompileForSessionLarge Native,None 122.918 us 7.1262 us 20.331 us 115.650 us 688 B

luaRuntimeLibraries

Method Params Mean Error StdDev Median Allocated
ResetParametersSmall Native,None 6.839 us 0.3603 us 1.051 us 6.950 us 688 B
ResetParametersLarge Native,None 6.181 us 0.5141 us 1.475 us 5.850 us 688 B
ConstructSmall Native,None 399.046 us 23.0923 us 66.627 us 369.500 us 1152 B
ConstructLarge Native,None 361.200 us 17.1312 us 47.470 us 349.900 us 3928 B
CompileForSessionSmall Native,None 30.435 us 1.1521 us 3.287 us 30.600 us 400 B
CompileForSessionLarge Native,None 109.432 us 5.9090 us 17.049 us 102.150 us 1024 B

Related Issues

@badrishc badrishc linked an issue Mar 8, 2025 that may be closed by this pull request
@kevin-montrose kevin-montrose marked this pull request as ready for review March 10, 2025 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant