Repository navigation
Replace yajl with vendored yyjson for JSON parsing - #134
Conversation
The REST VOL depended on the unmaintained lloyd/yajl library to parse HSDS's JSON responses. Its ancient CMakeLists (cmake_minimum_required 2.6) fails under CMake 4.x, which broke the Windows CI job that builds yajl from source. This PR entirely replaces lloyd/yajl with the actively-maintained yyjson library. More specifically: - Add JSON helpers in rest_vol.c/.h using yyjson: RV_json_parse, RV_json_get, RV_json_obj_key_at/RV_json_obj_val_at, and RV_json_get_*/RV_json_is_* accessors. - Port all yajl usage to the new routines. Build changes: - CMake: drop find_package(YAJL) and the yajl link, add yyjson.c to sources, remove the unused CMake/FindYAJL.cmake, its install rule, and the HDF5_VOL_REST_RESOURCES_MOD_DIR variable. - Autotools: add yyjson.c to src/Makefile.am, remove -lyajl, the YAJL install-dir flags, the libyajl check and --with-yajl from configure.ac.
The connector supported both CMake and Autotools builds, but the Autotools path was Linux-only and effectively unused. Drop it in favor of CMake as the single build system.
Replace the vendored copy of yyjson with CMake-based acquisition. Build uses an installed yyjson if one is found, otherwise FetchContent downloads and builds a pinned version (0.12.0).
jhendersonHDF
left a comment
jhendersonHDF
left a comment
There was a problem hiding this comment.
Overall looks fine, but see my comment about the new RV_json_obj_key_at() and RV_json_obj_val_at() functions; those seem inefficient enough to make me feel on the fence about yyjson
| set (HDF5_VOL_REST_YYJSON_TARGET yyjson::yyjson) | ||
| else () | ||
| message (FATAL_ERROR "YAJL not found; please check YAJL_INCLUDE_DIR") | ||
| message (STATUS "yyjson not found; fetching v${HDF5_VOL_REST_YYJSON_VERSION} with FetchContent") |
There was a problem hiding this comment.
This works for the time being, but should probably be deprecated in the future; FetchContent often causes more problems than it's worth and most of the time we should prefer that users have the dependencies installed and findable with find_package().
| *------------------------------------------------------------------------- | ||
| */ | ||
| const char * | ||
| RV_json_obj_key_at(yyjson_val *obj, size_t idx) |
There was a problem hiding this comment.
I'm really not excited about replacing what was previously a constant-time array index with a linear search, especially when looking at the compound datatype code in rest_vol_datatype.c where both RV_json_obj_key_at() and RV_json_obj_val_at() are called for each compound member, leading to quadratic complexity. This also seems like it could drastically hurt iteration performance and the performance of _by_index APIs. I assume this had to be done due to the way yyjson models the parse tree internally?
For this and the function below, can you leave a prominent warning in the existing comment in rest_vol.h that the functions are of linear complexity so that future developers know to avoid using these functions when possible? We will definitely need a better solution going forward.
There was a problem hiding this comment.
I've added a warning to note that it's O(n) with respect to how many members an object has, and should be avoided in nested loops.
Yes, yyjson models the parse tree differently. Where yajl's JSON tree gave every obj/array their own heap-allocated keys[]/values[] arrays that could be indexed into in O(1) time, yyjson instead stores the whole document as a contiguous buffer of values, such that reaching a child of a container requires walking over the subtrees of every sibling that's ordered before it. On the bright side, this should dramatically speed up the actual parsing operation - though whether that will lead to a performance improvement on net, I'm not sure. Regardless, this switch is somewhat forced by the fact that YAJL is unmaintained, so performance is a second-order concern.
I also changed around which routines are used to avoid unnecessary linear time ops. I changed the link, attr, compound, and filter loops to use yyjson_arr_iter structure, which can be advanced in O(1). The search for link title and compound names are now direct RV_json_get() lookups. After that, RV_json_obj_val_at() no longer had any callers, so I've removed it.
The decreasing-order case computed the index from yyjson_obj_size() on the links array, which returns 0 for arrays, so the index wrapped and every decreasing-order lookup failed. Use yyjson_arr_size() instead.
|
This won't pass CI on its own without the changes in #135 |
The REST VOL depended on the unmaintained lloyd/yajl library to parse HSDS's JSON responses. Its ancient CMakeLists (cmake_minimum_required 2.6) fails under CMake 4.x, which broke the Windows CI job that builds yajl from source.
This PR entirely replaces lloyd/yajl with the actively-maintained yyjson library. More specifically:
Some associated build changes: