Perf[BMQP]: minimizing copies in MessageProperties #1014
Conversation
dorjesinpo
force-pushed
the
dev/shallow-mps
branch
from
d8af6aa to
f06a195
Compare
![bmq-oss-ci[bot]](https://avatars.githubusercontent.com/u/1416818?s=60&v=4){kind=small}
dorjesinpo
force-pushed
the
dev/shallow-mps
branch
from
f06a195 to
1ada41f
Compare
Signed-off-by: dorjesinpo <[email protected]>
Signed-off-by: dorjesinpo <[email protected]>
dorjesinpo
force-pushed
the
dev/shallow-mps
branch
from
1ada41f to
f0bee45
Compare
678098
changed the title
| if (ret == 0) { | ||
| // Do not align | ||
| if (position.buffer() == end.buffer() || | ||
| (position.buffer() + 1 == end.buffer() && end.byte() == 0)) { |
There was a problem hiding this comment.
I think it's worth extracting this to bmqu::BlobUtil::isDataContinuous(start, end)
| doCopy = false; | ||
| } | ||
| } | ||
| if (doCopy) { |
There was a problem hiding this comment.
We can still get rid of doCopy if we just return when we read a property
There was a problem hiding this comment.
Sure, but is it that much better?
There was a problem hiding this comment.
Let's keep the current one
There was a problem hiding this comment.
Without micro-benchmarks we can only speculate if it is better or not
|
|
||
| const PropertyVariant& value = getPropertyValueAsString(it->second); | ||
|
|
||
| BSLS_ASSERT(value.is<bsl::string>() && "Property data type mismatch"); |
There was a problem hiding this comment.
There was just 1 call in this function before, now there are at least 2 calls and 2 extra checks. Not sure if it doesn't harm the performance more than the benefit we get from this PR.
There was a problem hiding this comment.
There were the same calls findProperty and getPropertyValue (in getProperty).
The same check.
What is added is this:
if (v.is<bsl::string_view>()) {
v = bsl::string(v.the<bsl::string_view>(), d_allocator_p);
}
Consider the fact that MessagePropertiesReader does not make a copy of string anymore (it does not execute this code). That is one of main benefits of this PR. The second one being not copying blob.
Introducing
bmqp::MessageProperties ::d_doDeepCopy(trueby default). Whenfalse, there is noblobcopy. Meaning,MessagePropertiesvalid only when the originalblobis alive. This helpsRouters::MessagePropertiesReaderwhen iterating subscriptions.avoiding extra
bsl::stringcopy when reading strings from blob. Usebsl::string_viewinstead. Note thatsetPropertystill needs to copy strings. AndgetPropertyneeds to convertbsl::string_viewintobsl::string. Again,Routers::MessagePropertiesReaderdoes benefit from the use ofbsl::string_view.