Perf[BMQ,MQB]: simplify evaluator - #837
Conversation
| if (context.d_stop) { | ||
| if (context.hasError()) { | ||
| return false; // RETURN | ||
| } |
There was a problem hiding this comment.
This ensures the same behaviour as the code that was removed from mqbblp::Routers without adding extra performance cost:
if (d_evaluationContext_p->hasError()) {
return false; // RETURN
}
ee0a853 to
bcd2182
Compare
678098
changed the title
![bmq-oss-ci[bot]](https://avatars.githubusercontent.com/u/1416818?s=60&v=4){kind=small}
| /// Rb-tree containing property name->value pairs. | ||
| /// Note: when the number of properties is small map works faster than | ||
| /// unordered_map. Also, map takes less space. | ||
| mutable PropertyMap d_properties; |
There was a problem hiding this comment.
- This is a
map, notunordered_map. - I tried switching it to
unordered_map, and it works much slower thanmapwhen we only have 1-2 properties. String hashing takes a lot of time in such cases.
The same benchmark, this function takes 4% of CPU frames when map is used:
4.08% BloombergLP::bmqp::MessageProperties::findProperty
If we switch it to unordered_map, it uses 9% of CPU frames and mostly it is string hashing.
|
|
||
| if (left.theBoolean()) { | ||
| return bdld::Datum::createBoolean(true); // RETURN | ||
| return left; // RETURN |
There was a problem hiding this comment.
the return type is not const, are we sure we can use the same instance?
(wee seem to be ok returning right)
There was a problem hiding this comment.
We keep left by value, not by reference. We also return it by value. Datum is a simply copyable type, so value copy is safe.
There is no need to construct another Datum if we can simply return left that 1)owned by the local scope 2)is already constructed 3)has the correct true value.
|
|
||
| // Set by `Expression::evaluate` functions when a property does not | ||
| // have the type deduced during compilation. Stop evaluation and return | ||
| // `true`. |
There was a problem hiding this comment.
was the comment wrong and we return false?
There was a problem hiding this comment.
The comment was wrong indeed. That is the problem with comments describing implementation in a distant part of the code.
| /// | false | true | IMPOSSIBLE | | ||
| /// | true | false | true | | ||
| /// | true | true | d_evaluator.evaluate() | | ||
| /// |============|=========|========================| |
There was a problem hiding this comment.
thank you for documenting this. we could also document the result when a property is missing
There was a problem hiding this comment.
Added a comment below
3c24e82 to
ecfe28a
Compare
Signed-off-by: Evgeny Malygin <emalygin@bloomberg.net>
Signed-off-by: Evgeny Malygin <emalygin@bloomberg.net>
Signed-off-by: Evgeny Malygin <emalygin@bloomberg.net>
Signed-off-by: Evgeny Malygin <emalygin@bloomberg.net>
Signed-off-by: Evgeny Malygin <emalygin@bloomberg.net>
ecfe28a to
0379b1b
Compare
2 participants
Changes
bmqeval::EvaluationContext: get rid of the bool flagd_stopand used_lastErrorto exit evaluation early. This also speeds up the reset of evaluation context. The idea is that if we have an error stored in the field already we must stop early, and we don't need a special bool flag for this.mqbblp::Routers: remove unnecessary checkif (!d_evaluator.isCompiled()). The same behaviour can be achieved by only checkingd_evaluator.isValid(). Look at the table in the comments that explains return values ofbool Routers::Expression::evaluate().mqbblp::Routers: remove unnecessary checkif (d_evaluationContext_p->hasError()). The same can be checked in evaluator itself.mutable int bmqp::MessageProperties::d_lastErrorbmqp::MessageProperties::Property: reorder fields and putbdlb::Variant7in the beginning to improve field packing and alignment.sizeofof this structure reduced from 88 bytes to 80. This struct has to be aligned by 16 byte offset, so the old version used 96 bytes if places continuously in memory, now these structures can be placed in memory without gaps because80 = 16*5.bmqp::MessageProperties::findProperty: do not construct next property on stack if not needed.