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

feat: use simdjson in some FromJson calls #2240

Merged
merged 33 commits into from
Dec 7, 2024

Conversation

nic11
Copy link
Collaborator

@nic11 nic11 commented Dec 3, 2024

Important

Introduces SimdJsonInputArchive for JSON parsing with simdjson, updating relevant classes and tests for improved performance.

  • New Feature:
    • Introduces SimdJsonInputArchive class in SimdJsonInputArchive.h for JSON parsing using simdjson.
  • Integration:
    • Replaces JsonInputArchive with SimdJsonInputArchive in MessageBase.h, Equipment.cpp, and Inventory.cpp.
    • Updates MessageSerializerFactory.cpp to use simdjson for parsing JSON messages.
  • Tests:
    • Adds ArchiveTest.cpp to test SimdJsonInputArchive functionality.
    • Updates MovementSerializationTest.cpp to test JSON serialization/deserialization with simdjson.

This description was created by Ellipsis for 56050b2. It will automatically update as commits are pushed.

@Pospelove
Copy link
Contributor

MSVC fix please

@nic11 nic11 changed the title simdjson archive draft feat: use simdjson in some FromJson calls Dec 6, 2024

template <class T>
concept ArithmeticSimdjsonUnsupported = Arithmetic<T> && !
std::is_same_v<T, SimdJsonSupportedType<T>>;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

bruh

TODO: update clang, it's fine in 18

@nic11 nic11 marked this pull request as ready for review December 6, 2024 23:53
Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

❌ Changes requested. Reviewed everything up to 56050b2 in 1 minute and 44 seconds

More details
  • Looked at 913 lines of code in 9 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. serialization/include/archives/SimdJsonInputArchive.h:255
  • Draft comment:
    Ensure that the lifetime of simdjson::dom::element outlives the SimdJsonInputArchive instance to avoid dangling references.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The SimdJsonInputArchive class has a private member input which is a reference to a simdjson::dom::element. This is fine, but it should be noted that the lifetime of the simdjson::dom::element must outlive the SimdJsonInputArchive instance to avoid dangling references. This is a potential source of bugs if not handled carefully.
2. skymp5-server/cpp/server_guest_lib/Inventory.cpp:179
  • Draft comment:
    Good use of SimdJsonInputArchive to parse simdjson::dom::element directly for performance.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The Inventory::FromJson function is correctly using SimdJsonInputArchive to parse the simdjson::dom::element. This is a good practice for performance and should be followed in other similar functions.

Workflow ID: wflow_Wtj2AibcMVRk2OqW


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

struct SimdJsonSupportedTypeAdapter
{
template <std::signed_integral T>
std::decay<int64_t> MapType(T);
Copy link

Choose a reason for hiding this comment

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

std::decay<int64_t> should be std::decay_t<int64_t>. This applies to other uses of std::decay in this struct as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(no idea if you can talk to this bot, but they say you can, so let's try lol)

On purpose to have ReturnType::type return the target type , otherwise would get this

template <class T>
using SimdJsonSupportedType =
  typename decltype(std::declval<SimdJsonSupportedTypeAdapter>().MapType(
    std::declval<T>()));

Expected a qualified name after 'typename' clang(expected_qualified_after_typename)

If there's a cleaner way to do so, please suggest

@Pospelove Pospelove merged commit f1aaff4 into skyrim-multiplayer:main Dec 7, 2024
9 of 10 checks passed
@Pospelove Pospelove deleted the simdjson-arch branch December 7, 2024 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants