Skip to content

Conversation

drewdzzz
Copy link
Collaborator

@drewdzzz drewdzzz commented Mar 31, 2025

The commit adapts connector to multithreaded scenarios and populates CI with thread sanitizer. See commits for details.

Closes #110

@drewdzzz drewdzzz force-pushed the multithread branch 4 times, most recently from 1934461 to e522777 Compare April 1, 2025 09:03
@CuriousGeorgiy
Copy link
Member

Could you please consider adding the ThreadSanitizer 1 or/and the Helgrind 2 tools for multithreaded tests?

Footnotes

  1. https://clang.llvm.org/docs/ThreadSanitizer.html

  2. https://valgrind.org/docs/manual/hg-manual.html

@drewdzzz drewdzzz force-pushed the multithread branch 19 times, most recently from 6182ffa to e23e678 Compare April 9, 2025 13:07
@drewdzzz drewdzzz changed the title multithread wip client: support multi-threading Apr 9, 2025
@drewdzzz drewdzzz force-pushed the multithread branch 5 times, most recently from d723f4f to 3593238 Compare April 10, 2025 08:36
@drewdzzz drewdzzz force-pushed the multithread branch 3 times, most recently from aae1408 to b3cc5d2 Compare April 15, 2025 12:48
Mempool uses default implementations of move and copy semantics and it's
not great - it owns resources, hence, trivial implementations are not
appropriate. Let's implement convenient move constructor and operator.
What's about copying ones, let's explicitly delete them - I can hardly
imagine a case when one needs to copy an allocator instead of moving it.

The move semantics in MempoolInstance is implemented as a simple swap so
that the destructor of moved-from object will release resources that
moved-to object owned before the move.

All the newly introduced constructors and operators are explicitly
marked as `noexcept` because thay actually are.

Part of tarantool#110
Allocator can be not copyable, so it's better to consume it in
constructor. It is needed because we are going to use non-copyable
allocator for `Buffer` by default.

Part of tarantool#110
Default implementations of move semantics are not suitable because
buffer owns resources - let's implement proper ones. Note that the move
cannot be implemented as a swap here because allocator, that is stored
in the buffer, shouldn't be used after it is moved.

Part of tarantool#110
Currently we use MempoolHolder as default allocator - it makes all
buffers use the same MempoolInstance. This buffer cannot be used in
Multi-Threaded scenarios, so let's simply use `MempoolInstance`
instead - now each connection will use its own allocator. To reduce
memory footprint of each connection, let's set default parameter
`M = slab_size / block_size` to 16 to use 256KB slabs by default.

Along the way, move a brace to the same line with `Buffer` class name to
make it conform to our clang format.

Part of tarantool#110
@drewdzzz drewdzzz force-pushed the multithread branch 6 times, most recently from 4252a54 to de2104f Compare April 15, 2025 15:07
@drewdzzz drewdzzz requested a review from CuriousGeorgiy April 15, 2025 15:16
@drewdzzz drewdzzz assigned CuriousGeorgiy and unassigned drewdzzz Apr 15, 2025
drewdzzz and others added 5 commits April 16, 2025 13:49
The previous commits adapted Tntcxx for multithreaded execution - this
adds a test for the scenario.

Part of tarantool#110
We have a cmake option to build Tntcxx with sanitizers. Currently, only
memory and UB sanitizers are used - let's populate the list with thread
sanitizer since we have a test for a multithreaded scenario and use it
on CI.

Since address and thread sanitizers cannot be used together, existing
cmake option `TNTCXX_ENABLE_SANITIZERS` is extended - now it accepts
"address" and "thread" arguments to choose a sanitizer.

Part of tarantool#110
Trailing whitespaces are never appreciated, so let's remove them all.
The section describes how the connector should be used from several
threads.

Closes tarantool#110

Co-authored-by: Georgiy Lebedev <[email protected]>
We are using the first version of setup-tarantool action and it causes
some problems - sometimes it just fails because the NodeJS version
installed on the runner is too new for this old action. Let's simply use
the third (the newest) version of this action - it behaves in the same
way, but it is more stable.
@drewdzzz
Copy link
Collaborator Author

@CuriousGeorgiy
I pushed a new commit to use setup-tarantool@v3 in our CI. Currently we use the old version (v1), and it is unstable (AFAIU, because NodeJS version on the runner is too new for it) - CI was failing all the time. Hope you don't mind.

@drewdzzz drewdzzz merged commit f9dae01 into tarantool:master Apr 17, 2025
49 checks passed
@drewdzzz drewdzzz deleted the multithread branch April 17, 2025 07:46
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.

Allow to use in multithreaded environment, one connector per thread

4 participants