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

ffi: Add support to serialize msgpack array/map into a JSON string. #465

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

Conversation

LinZhihao-723
Copy link
Member

Description

In the next IR stream format, we use msgpack to represent a structured log event before CLP IR serialization. To handle the arrays inside the structured log event, we store them as unstructured arrays represented by a JSON string (same as clp-s). This requires methods to turn a msgpack object into JSON string. There are two existing ways to do this:

  1. nlohmann JSON library provides this feature by turning the msgpack bytes into an in-memory JSON object and then dumping the object into a raw string. However, from our experimental benchmark, this introduces significant run time overhead. It's more efficient to directly turn a msgpack in-memory objects into JSON string.
  2. Using << operator on an in-memory msgpack object will also return a JSON string: [https://github.com/msgpack/msgpack-c/wiki/v_2_0_cpp_json].

This PR adds methods to support serialization from msgpack array/map to JSON string without turning it into other intermediate representations. Even though msgpack-c already has support on JSON serialization, a microbenchmark shows that our implementation is 3 times faster than using <<.

Validation performed

  1. Add unit tests to ensure the generated strings are valid JSON strings, and have the same schema and values before/after serialization.
  2. Workflow passed in all platforms.

// Recursively create inner maps and arrays
// Note: the execution time and memory consumption will grow exponentially as we increase the
// recursive depth.
constexpr size_t cRecursiveDepth{6};
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This depth is chosen to not take too long. Depth == 6 takes 3 seconds on my laptop, whereas Depth == 7 takes 25 seconds (which might be too long).

@kirkrodrigues
Copy link
Member

Can you resolve the conflicts?

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

Successfully merging this pull request may close these issues.

2 participants