Avoid object pool for message builder#595
Conversation
| } | ||
| } | ||
|
|
||
| TEST(MessageTest, testMessageBuilderConcurrentBuild) { |
There was a problem hiding this comment.
This test seems meaningless. Thread safety of concurrent shared_ptr creation is guaranteed by the standard library.
There was a problem hiding this comment.
Check whether it will crash.
There was a problem hiding this comment.
I mean, this is guaranteed by the standard library and our unit tests should only focus on the SDK side.
This does not like a regression test as well because the object pool is not used to avoid concurrent build crash.
There was a problem hiding this comment.
From LLM:
The test
testMessageBuilderConcurrentBuildis not meaningful.Each thread creates its own independent
MessageBuilderinstances — there is no shared mutable state between threads. The test is equivalent to running the existing sequentialtestMessageBuildertest in parallel 80,000 times, which doesn't validate any concurrency behavior specific to the code under test.std::make_shared(the replacement) is inherently thread-safe, andMessageBuildermethods operate on per-instanceimpl_state with no global or shared data.To be meaningful, the test would need to exercise a shared resource — for example, if it used a shared pool (the old
ObjectPool) or shared metadata state. As written, it's just a stress test of the heap allocator, which isn't what the change is about.
From myself: testing concurrent creation of std::shared_ptr objects is not commonly seen. Such tests are meaningless, otherwise, we should also test concurrent creation of Producer, Consumer, etc.
Fixes #476
Master Issue: #
Motivation
Modifications
Verifying this change
(Please pick either of the following options)
This change is a trivial rework / code cleanup without any test coverage.
(or)
This change is already covered by existing tests, such as (please describe tests).
(or)
This change added tests and can be verified as follows:
(example:)
Documentation
doc-required(Your PR needs to update docs and you will update later)
doc-not-needed(Please explain why)
doc(Your PR contains doc changes)
doc-complete(Docs have been already added)