refactor: Add yyjson as Primary UniValue Storage Backend#326
Conversation
|
The lint failure here is caused by this PR, not by pre-existing CI breakage. Exactly one linter exits non-zero:
I checked out the PR head and ran every The rest of the lint log is non-fatal output that also appears on
On lint having been updated since the branch: the merge-base is Suggested fix, which keeps # test/lint/lint-include-guards.py
EXCLUDE_FILES_WITH_PREFIX = ['contrib/devtools/bitcoin-tidy',
'src/crypto/ctaes',
'src/tinyformat.h',
'src/bench/nanobench.h',
'src/univalue/yyjson/',
'src/test/fuzz/FuzzedDataProvider.h'] + SHARED_EXCLUDED_SUBTREESWith that one line, Related: if |
I have used your recommended fix. |
- Thread safety: Use std::atomic<bool> and std::mutex for lazy const access in materialization to support re-entry when materialized state changes - Performance: Preserve yyjson tree through copy operations using yyjson_mut_val_mut_copy() to avoid performance regression from unconditional tree discarding - Fix push_backV to handle nested aliases and prevent iterator invalidation - Change pushKVs to take UniValue by value to avoid aliasing issues - Remove unnecessary std::string allocation in UniValue::read - Fix writeYyjson to not serialize entire document for child nodes - Add vendor.md for yyjson version tracking - Add Doxygen documentation for thread safety and performance considerations - Add lint exclusion for yyjson vendor directory
|
I have added commit 62cc41f to address @kwsantiago comments. Specifically addressed includes:
|
| mutable std::shared_ptr<yyjson_mut_doc> m_yyjson_doc; //!< Shared pointer to yyjson mutable document (primary storage) | ||
| mutable yyjson_mut_val* m_yyjson_node{nullptr}; //!< Pointer to the root node in the yyjson tree | ||
| mutable std::atomic<bool> m_materialized{false}; //!< Whether lazy caches (val/keys/values) have been populated | ||
| mutable std::mutex m_materialize_mutex; //!< Protects materialization to allow re-entry when m_materialized changes |
There was a problem hiding this comment.
A std::mutex per UniValue is heavy: 40 bytes plus a non-trivial ctor/dtor on every instance, and UniValue is created per field and per array element. A getblock/getblocktemplate response builds and tears down thousands of them. A single doc-level lock, or the std::call_once you mentioned, would avoid paying this on every node.
There was a problem hiding this comment.
I did attempt to use std::call_once, but I ran into an issue with rematerialization, and was forced to change.
The UniValue design requires re-running materialization when the yyjson tree changes. Operations like push_back and pushKV modify the tree, then call "m_materialized.store(false);".
I like your idea of a single doc-level lock. I plan to implement this idea.
There was a problem hiding this comment.
The single doc-level lock sounded so simple. It has had a little complexity explosion. :-)
There was a problem hiding this comment.
I have now removed all mutexes to match the original UniValue implementation.
| UniValue::UniValue(UniValue::VType type, std::string str) : typ(type) { | ||
| // Containers need yyjson documents for tree building | ||
| if (type == VOBJ || type == VARR) { | ||
| m_yyjson_doc = std::shared_ptr<yyjson_mut_doc>(yyjson_mut_doc_new(nullptr), yyjson_doc_deleter); |
There was a problem hiding this comment.
The copy ctor now allocates a fresh yyjson doc and deep-copies the tree on every container copy. That fixes the dropped-tree issue but copies are cheap in the legacy path, so this could be net slower. Worth re-running the benchmark table, it predates this commit and both this and the mutex change affect it.
There was a problem hiding this comment.
The current benchmark numbers in the top description have been re-run, but I will look into this further.
pdath
changed the title
7e882ed to
2a3b190
Compare
pdath
changed the title
6a27316 to
660880e
Compare
660880e to
838afc8
Compare
838afc8 to
84a23e3
Compare
|
tACK 2a3b190 Tested using four different AWS Graviton-based instances: t4g.medium (2 x Graviton2 ARM) bench_bitcoin results — yyjson vs stock UniValueBitcoin Knots v29.3.knots20260507 + pdath cherry-picks (yyjson, ARM crypto fix, jemalloc) Compile flagsBenchmark commandyyjson ON
yyjson OFF (stock UniValue)
BlockToJsonVerboseWrite speedup (getblocktemplate path)
RpcMempool regression
Key findings
|
3 participants
refactor: Add yyjson as Primary UniValue Storage Backend
Summary
This pull request introduces yyjson as an optional, high-performance primary storage backend for the UniValue library. yyjson is a battle-hardened, widely deployed JSON parser that is recognised as one of the fastest available JSON libraries.
Key Features:
-DWITH_YYJSON=ON. Existing users unaffected.Architecture Overview
The implementation uses yyjson as the primary storage mechanism for all UniValue containers, with the original
val,keys, andvaluesmembers serving as lazy caches. This approach ensures that all UniValues benefit from yyjson's optimised serialisation, not just those parsed from JSON input.Semantics Change
Neither the original UniValue nor this yyjson version are thread-safe. The caller must take precautions (such as using a mutex) to protect access.
This yyjson version is even less thread-safe. A "const" UniValue parameter could potentially be changed by a different concurrent thread, breaking the "const" guarantee. This limitation is accepted to maintain public API compatibility (and to avoid changes to anything else in Bitcoin Knots). Everything in Bitcoin Knots already either uses a mutex to protect access to UniValue or is a per-request local, which prevents this from being an issue.
Implementation Details
Build System Integration
The implementation uses a CMake option
-DWITH_YYJSONto toggle between the original UniValue implementation and the new yyjson-based implementation:-DWITH_YYJSON=OFF(default): Uses the original UniValue files-DWITH_YYJSON=ON: Uses the new yyjson-based implementationThis dual-path approach enables zero-risk development and deployment.
Code Organisation
To minimise duplication and maintain consistency, common methods shared between both implementations have been extracted to:
univalue_common.cppunivalue_common.hThese common files contain approximately 99% identical code to the originals. Several bugs resulting from failure to perform proper bounds checking were identified and fixed
The yyjson-specific implementation consists of:
univalue_yyjson.cpp- Main yyjson backend implementationunivalue_yyjson_get.cpp- Getter methods for yyjson backendunivalue_yyjson_read.cpp- JSON reading/parsing with yyjsonunivalue_yyjson_write.cpp- JSON writing/serialization with yyjsonyyjson/yyjson.candyyjson/yyjson.h- The yyjson library itselfProblem Being Solved
This pull request addresses the performance bottlenecks in Bitcoin Knots JSON serialisation and deserialization, particularly for mining-related operations. The standard UniValue implementation, while correct and reliable, has become a performance-limiting factor in high-throughput scenarios such as:
getblocktemplate,getblock, etc.)Testing
Extensive testing has been performed on Ubuntu to ensure correctness, stability, and memory safety:
All tests have been run with both
WITH_YYJSON=OFFandWITH_YYJSON=ON:All tests pass cleanly with both the original and yyjson implementations.
I mined a block on [testnet4] against commit a103f93: 143281
I mined a block on [testnet4] against commit 2a3b190: 144657
Benchmark Results (on my machine)
Comprehensive benchmarking demonstrates significant improvements in RPC performance across various operations. The test BlockToJsonVerboseWrite models the getblocktemplate function, and rpcmempool models mempool processing.
It is likely that there will be other flow-on benefits from reduced global mutex processing, but these have not been measured.
Reproducing Benchmark Results
To reproduce the benchmark results, use the following commands. Note that each benchmark will take 2.5 minutes to run.
Build Without yyjson
Build With yyjson
The above commands will output "ns/op" followed by the test name.
Compare the results to verify the performance improvements shown in the benchmark table above.