Incorporate all issues, add tests, add features, improve performance. #30
Conversation
Fixed the issue where +-inf of NaNs would be encoded as integers. Added a header file which isolates inclusions and conditionally defines macros for inspecting float-type to integral-type conversions in a portable fashion.
Now pack can receive any number of arguments returning a stream of the serialized data (on 0 objects, returns the empty string). unpack deserialized any stream of objects, returning the result of the whole stream deserialization (this, the number of return values of unpack is equal to the number of objects unpacked). If given the empty string, returns nothing. I also removed all warnings and made the code C++ compatible. [untested at this point]
@moteus suggested this change in moteus@7f64c4f That patch is a bit of a mess (it's a merge of somebody else's work squashed with about four other changes too), so I re-implemented the safe functionality in a slightly cleaner way (using Lua C closures with upvalues). Now we have a cmsgpack.safe module that won't throw errors, but rather return them as clean (nil, err) return values. Minor API change: calling pack() with no arguments now throws an error instead of returning an empty string (because nothing was encoded, we shouldn't return anything resembling usable data). Also: now we prefer luaL_argerror for cleaner errors caused by invalid arguments and also luaL_error for easier msg-and-return-error functionality. Double also: now we prefer to call return on error functions so it's clear to us the Lua error functions do long jumps and never return control to the original function from then onward.
This was a complex way of saying "make the top of stack the current top of the stack" and caused no side effects.
We're using CMake here because building a Makefile takes
longer and won't automatically find your system-wide Lua
for including against.
You can run the tests with:
mkdir build
cd build
cmake ..
lua ../test.lua
As of this commit all tests pass:
TEST PASSED: 207
TEST FAILED: 0
TEST SKIPPED: 0
This commit adds many new tests and test checking methods for existing
functionality and the new streaming/multi-pack/multi-return functionality.
Thanks to @moteus for testing suggestions which I've included.
Sadly, I couldn't include the changes directly because they were too intermixed
with the rest of a 250 line commit[1].
[1]: moteus@7f64c4f
Lua 5.3 adds an actual Integer type, which by default
will be a 64-bit long long. Currently, Lua {5.1,5.2}
only has a Number type which by default is a 64-bit
double.
Note: Lua can be custom compiled to re-define Integer
to be 32 bits (as well as Number to be just a float), but
that should be rare and the interal cmsgpack conversion
routes store integers in the most compact representation
discernable.
This also uses the existing lua_tointeger function instead of
casting a lua_Number to an int64_t directly.
Also remove empty end-of-line spaces from a few places.
Inspired by antirez#10, I added two new functions allowing you to limit multiple return from unpack. If you don't trust your msgpack input or want to manage flow control of returned object generation yourself, you can now limit the objects returned from unpack (but not with unpack() itself because it already returns an arbitrary number of objects). You can now unpack_one(msgpack) to get (next_offset, obj) returned; then call unpack_one(msgpack, next_offset) to get another offset and one more object> You can now also unpack_limit(msgpack, N) to get (next_offset, obj1, obj2, ..., objN) returned; then call unpack_limit(msgpack, K, next_offset) to get another offset and K more objects. This also refactors the copy/paste command addition in module bring-up. Tests added for new commands and all tests pass: TEST PASSED: 225 TEST FAILED: 0 TEST SKIPPED: 0 Closes antirez#10
We don't really need malloc here since we use it and collect it all in the same spot without ever needing to resize anything.
We need a test to pack something larger than the default allocation buffer.
This fixes Lua 5.3 compatability and works fine on Lua 5.1 too. Lua 5.3 has an actual 64-bit Integer type, so we need to use _pushinteger instead of _pushnumber so our unpacked integers don't convert to floating point values. For older versions, lua_pushinteger casts input to the default numeric type and behavior is unchanged. Lua 5.3 also has an unsigned integer type, so I added a compatibility macro to allow us to properly use lua_pushunsigned on 5.3+ with a fallback to the regular lua_pushinteger on other versions.
Lua 5.3 doesn't iterate tables in any specific order, so the resulting msgpack may vary when using associative tables with more than one element. I've only seen two variations on the the regression test output so far, so tracking those are still simple. If they get wildly out of sync and start generating new msgpack on every run, we may want to drop the exact output verification check. The recursive regression test now has a circular test too, so even if the output doesn't match, we can verify the unpacked result matches the input result (to a fixed depth). As of this commit, all tests pass on Lua 5.1 and Lua 5.3: TEST PASSED: 226 TEST FAILED: 0 TEST SKIPPED: 0
We can reuse the same malloc'd buffer for encoding multiple arguments. Just tell the buffer all current contents is now free space and reset the current offset back to zero. We now only malloc once per pack() call instead of argc times per call, giving us better performance with large argument counts. All 226 tests pass.
|
That's a great effort Matt! Thanks, this both provides a lot of value and incorporates past contribs that needed some inspection. The only thing I'm not sure to merge is the |
|
tl;dr: Setting up a working cross-platform Makefile to auto-discovers system Lua locations and build shared objects properly across platforms is a project all by itself. It took me 30 seconds to copy a working cmake config. All I wanted to do is run some tests. :) Without cmake here, we have:
With cmake we have:
cmake does the right thing with very minimal configuration. It discovers the system Lua, uses the correct compile arguments for the system (OS X, Linux, Windows, Solaris, ...), and builds a usable shared object all in under 40 lines. Most Makefiles are still doing system detection and variable setting in the first 40 lines. The cmake file has three lines defining our build. The rest of the file just defines cmake internals that are common to most other projects. But—using cmake is completely optional and makes developing and testing the code easier/possible. Plus, it has pretty output. We all like pretty make output: matt@ununoctium:~/repos/lua-cmsgpack/build% make
[100%] Building C object CMakeFiles/cmsgpack.dir/lua_cmsgpack.c.o
Linking C shared module cmsgpack.so
[100%] Built target cmsgpack |
cmsgpack is a much better Lua citizen now because we use the memory allocator assigned to our lua_State instead of grabbing memory from the system ourselves. Typically the memory allocator is just the system's realloc, but some use cases involve providing custom allocation routines (for accounting or performance or limiting memory). This closes antirez#20 too because those commits were just trying to remove the previous direct allocation behavior. All tests pass under Lua 5.1 and Lua 5.3-work2.
If more than ~20 objects were being returned at once, Lua would segfault because the default stack size is 20 and nobody was resizing the stack. Now we can return up to ~8,000 objects at once before erroring out the function properly instead of segfaulting. Also added test for segfault. All tests currently pass.
Closes antirez#31
'unpack' is only on 5.1, in 5.2 it got moved to 'table.unpack' This line gives us the proper 'unpack' based on what's in our global namespace.
Changes: - Improve table vs. array detection - Improve packing +-inf - Add multiple pack/unpack support - Add cmsgpack.safe module variant - Add local build infrastructure for easier testing - Add user-controlled unpack support limiting returned objects - Add Lua 5.3 compatibility - Remove an unnecessary malloc - Use Lua memory allocator instead of malloc for buffer creation Issues involved: - closes antirez#16 - allow multi pack/unpack by default - closes antirez#10 - unpack one/limit API added - closes antirez#13 and closes antirez#20 - use Lua allocator - closes antirez#15 - (included in antirez#16) - ignores antirez#22 because it's confusing - closes antirez#23 - fixed elsewhere - closes antirez#26 - extracted some useful parts from a difficult commit - closes antirez#28 - we started tagging versions again recently - closes antirez#27 - that failure case works for me now - closes antirez#31 - fix comment typos I merged commits with original author information where possible, but each commit required manual cleanup of one or more of: formatting fixes (no tabs, please), commit message fixes (more details please), extracting contents from a single 300 line commit with 5 different logical changes merged together, and general correctness checking after merging with newer code. As of this commit, all tests pass on Lua 5.1.5 and Lua 5.3-work2.
|
Love the changes, but share the anxiety about cmake. It's a preference and I can see the merit in cmake. For me, the verbosity of On a a scale of 1 - 7.5, my cmake anxiety is about a 3. If it's there, I'll move on and be happy. All of that said, we've got some additions and are using Lua 5.3. So I'd love to see this package get merged up and unified so that we know to whom we should submit our pull requests. Thanks! |
|
Short version: there was no build process. I added simple build process. Feel free to remove the build process to return it to the previous state. :) |
|
Cool. I guess my short version is: @antirez Please decide and merge this pull request (with or without cmake) so that harmony may be restored and we can all move on. :) Our pull request isn't ready (nor is it a slam dunk to be generally useful), but when we submit it, I'd rather submit it against the generally accepted That said, my confidence in my grasp of github etiquette is much less than my confidence in my cmake knowledge. :) -Andrew |
The 0xFFFFFFFF test can potentially fail on 32 bit systems if our internal conversions are casting to wrong Lua types. lua_Integer != lua_Number
mattsta
force-pushed
the
add-streaming-and-refactor-and-tests
branch
3 times, most recently
from
5433ba2 to
3f29140
Compare
5.3.0-work2 has been obsoleted by the newer 5.3.0-beta This testing infrastructure is taken from: https://github.com/moteus/lua-vararg/tree/246dc3126dd66935a020a0a26c165ceb035268e5/.travis
This is the solution to #4 but rewritten to use compile-time pointer size determination.
mattsta
force-pushed
the
add-streaming-and-refactor-and-tests
branch
2 times, most recently
from
869c662 to
23d6f07
Compare
checkint -> checkinteger optint -> optinteger Also removes `< 503` check for creating pushunsigned. Older lua versions had redundant functions, but one with a shorter name than the other. Newer Lua versions only retain the long version. one is an alias of the other, but one was removed in 5.3+, so it's better to use the common function across all versions.
mattsta
force-pushed
the
add-streaming-and-refactor-and-tests
branch
from
23d6f07 to
afbc009
Compare
I spent this weekend living inside lua-cmsgpack code. We now have a newly updated and much improved lua-cmsgpack implementation.
All current tests pass using Lua 5.1.5 and Lua 5.3-work2.
Changes included:
Issues involved:
cmsgpack.safemodule and some fixes #26 - extracted some useful parts from a difficult commit