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

[ADDED] Support padding when allocating buffer for incoming messages #624

Merged
merged 1 commit into from
Jan 19, 2023
Merged

[ADDED] Support padding when allocating buffer for incoming messages #624

merged 1 commit into from
Jan 19, 2023

Conversation

dmitrmax
Copy link
Contributor

@dmitrmax dmitrmax commented Jan 18, 2023

When json is used as messages' payload and simdjson library is used to parse them it is strictly desirable to have SIMDJSON_PADDING bytes in the end of payload buffer. Otherwise parsing can't be done in zero copy manner - simdjson will reallocate it and copy all the payload into a temporary buffer.

I would like to introduce a new NATS option which will provide ability to set such a custom padding in message delivery path.

Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

Thank you for this pull request! Since this is an addition, I would like you to update your PR to be against the dev branch (instead of main). Also, would you be able to add a test (see https://github.com/nats-io/nats.c#testing for instructions). If you can't that's ok, I can come up with one and validate that your changes are ok before merge, then will submit a PR myself to add testing.

* this option.
*
* To clear the custom message buffer padding, call this function with 0.
*
Copy link
Member

Choose a reason for hiding this comment

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

I would like to add a note here that says that changing the option has no effect on existing NATS connections.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@dmitrmax dmitrmax changed the base branch from main to dev January 19, 2023 12:57
@dmitrmax
Copy link
Contributor Author

Thank you for this pull request! Since this is an addition, I would like you to update your PR to be against the dev branch (instead of main). Also, would you be able to add a test (see https://github.com/nats-io/nats.c#testing for instructions). If you can't that's ok, I can come up with one and validate that your changes are ok before merge, then will submit a PR myself to add testing.

@kozlovic ok, I've updated PR against dev branch and added test. As usual, creating a test is a good point to rethink what you are doing and I've change the logic a bit ) Previously the buffer for payload was allocated with an additional byte to null-terminate it. Now this byte is the part of my padding (if any was configured). And now all the padding is zeroed to suppress valgrind warnings for reading of uninitialized memory.

$ ctest -T memcheck -V -R "MessagePadding"
UpdateCTestConfiguration  from :/home/user/nats_build/DartConfiguration.tcl
Parse Config file:/home/user/nats_build/DartConfiguration.tcl
   Site: localhost
   Build name: Linux-c++
UpdateCTestConfiguration  from :/home/user/nats_build/DartConfiguration.tcl
Parse Config file:/home/user/nats_build/DartConfiguration.tcl
Memory check project /home/user/nats_build
Constructing a list of tests
Done constructing a list of tests
Updating test list for fixtures
Added 0 tests to meet fixture requirements
Checking test dependency graph...
Checking test dependency graph end
test 125
    Start 125: Test_MessagePadding
Memory check command: /usr/bin/valgrind "--log-file=/home/user/nats_build/Testing/Temporary/MemoryChecker.125.log" "--leak-check=full" "--track-fds=yes" "--show-reachable=yes" "--num-callers=50"

125: MemCheck command: /usr/bin/valgrind "--log-file=/home/user/nats_build/Testing/Temporary/MemoryChecker.125.log" "--leak-check=full" "--track-fds=yes" "--show-reachable=yes" "--num-callers=50" "/home/user/nats_build/test/testsuite" "124" "124"
125: Test timeout computed to be: 1500
125: 
125: == MessagePadding ==
125: #01 Create options: PASSED
125: #02 Setting message buffer padding: PASSED
125: #03 Setting servers: PASSED
125: #04 Test generating message for subscriber: PASSED
125: #05 Test access to memory in message buffer beyond data length: PASSED
125: ALL PASSED
1/1 MemCheck #125: Test_MessagePadding ..............   Passed    1.02 sec
125: process test output now: Test_MessagePadding Test_MessagePadding
PostProcessTest memcheck results for : Test_MessagePadding

The following tests passed:
        Test_MessagePadding

100% tests passed, 0 tests failed out of 1

Total Test time (real) =   1.02 sec
-- Processing memory checking output:
MemCheck log files can be found here: (<#> corresponds to test number)
/home/user/nats_build/Testing/Temporary/MemoryChecker.<#>.log
Memory checking results:

Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

Thank you for adding the test, I appreciate it! I have posted a comment. If you think that it is ok to keep it this way (suits your use-case), then I will approve and merge.

src/msg.c Show resolved Hide resolved
Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution!

@kozlovic kozlovic changed the title Support padding when allocating buffer for incoming messages [ADDED] Support padding when allocating buffer for incoming messages Jan 19, 2023
@kozlovic kozlovic merged commit e1bfb7b into nats-io:dev Jan 19, 2023
@dmitrmax
Copy link
Contributor Author

@kozlovic thank you for such a turbo review and accepting the PR )

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