Skip to content

Json builder init citm bench#2351

Merged
FranciscoThiesen merged 13 commits intojson_builder_initfrom
json_builder_init_citm_bench
Mar 27, 2025
Merged

Json builder init citm bench#2351
FranciscoThiesen merged 13 commits intojson_builder_initfrom
json_builder_init_citm_bench

Conversation

@FranciscoThiesen
Copy link
Member

  • Adding benchmark for citm_catalog data. (not working for rust yet)
  • In the process I've identified that serialization is not supporting std::map<K, V> yet. I can work on that later, might create an issue to track it.

@lemire
Copy link
Member

lemire commented Mar 23, 2025

@FranciscoThiesen Please rebase on json_builder_init. We now have std::map support.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a new benchmark for the citm_catalog data along with supporting changes for serialization and deserialization, and also includes minor documentation fixes.

  • Adds CitmCatalog benchmark functionality with custom deserializers in the serde benchmark crate.
  • Updates documentation examples for map-like type deserialization and corrects several typos in the docs.
  • Revises the README port list and fixes textual errors in CONTRIBUTING.md, doc/dom.md, and doc/iterate_many.md.

Reviewed Changes

Copilot reviewed 24 out of 36 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
doc/basics.md Updates code examples for map-like types; note typo in namespace usage.
benchmark/static_reflect/serde-benchmark/lib.rs Introduces CitmCatalog benchmark code and custom deserializers.
README.md Revises port list with duplicate entries added.
CONTRIBUTING.md Fixes typos in discussion guidelines.
doc/dom.md Corrects spelling of "directive".
doc/iterate_many.md Fixes typo in "data structure".
Files not reviewed (12)
  • benchmark/static_reflect/CMakeLists.txt: Language not supported
  • benchmark/static_reflect/citm_catalog_benchmark/CMakeLists.txt: Language not supported
  • benchmark/static_reflect/citm_catalog_benchmark/benchmark_serialization_citm_catalog.cpp: Language not supported
  • benchmark/static_reflect/citm_catalog_benchmark/citm_catalog_data.h: Language not supported
  • benchmark/static_reflect/citm_catalog_benchmark/nlohmann_citm_catalog_data.h: Language not supported
  • benchmark/static_reflect/serde-benchmark/serde_benchmark.h: Language not supported
  • benchmark/static_reflect/twitter_benchmark/CMakeLists.txt: Language not supported
  • benchmark/static_reflect/twitter_benchmark/benchmark_serialization_twitter.cpp: Language not supported
  • benchmark/static_reflect/twitter_benchmark/nlohmann_twitter_data.h: Language not supported
  • include/simdjson/concepts.h: Language not supported
  • include/simdjson/dom/document.h: Language not supported
  • include/simdjson/dom/parser.h: Language not supported
Comments suppressed due to low confidence (1)

README.md:178

  • Duplicate entry detected for the Zig port 'simdjzon'; consider removing one to avoid confusion.
- [simdjzon](https://github.com/travisstaloch/simdjzon): Zig port.

Haven't been able to validate this locally for now, as the reflection branch that I am building clang from doesn't have the new ^^ operator.
}

template <concepts::string_view_keyed_map T>
constexpr void atom(string_builder &b, const T &m) {
Copy link
Member

Choose a reason for hiding this comment

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

Very nice!

ondemand::document doc = parser.iterate(json);
std:map<std::string,Car> cars;
error = doc.get<std:map<std::string,Car>>().get(cars);
std::map<std::string,Car> cars;
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for fixing the typo.

#include "twitter_data.h"
#include "nlohmann_twitter_data.h"
#include "benchmark_helper.h"
#include "../benchmark_utils/benchmark_helper.h"
Copy link
Member

Choose a reason for hiding this comment

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

This is fine but we may want to simplify this in a future PR by adding an extra path?

Copy link
Member

@lemire lemire left a comment

Choose a reason for hiding this comment

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

It looks good to merge!!!

@FranciscoThiesen FranciscoThiesen merged commit 601fab5 into json_builder_init Mar 27, 2025
139 checks passed
@FranciscoThiesen FranciscoThiesen deleted the json_builder_init_citm_bench branch March 27, 2025 04:38
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.

3 participants