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

[WIP]Performance[mqbi::DispatcherEvent]: replace multiple inheritance with variant #396

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

678098
Copy link
Collaborator

@678098 678098 commented Aug 9, 2024

Before

sizeof(mqbi::DispatcherEvent): 880
mqbi::DispatcherEvent::reset():
       total: 708.62 ms (100000000 iterations)
    per call: 7 ns

After

sizeof(mqbi::DispatcherEvent): 320
sizeof(mqbi::DispatcherPutEvent): 128
sizeof(mqbi::DispatcherPushEvent): 288
sizeof(mqbi::DispatcherDispatcherEvent): 152
sizeof(mqbi::DispatcherCallbackEvent): 80
sizeof(mqbi::DispatcherReceiptEvent): 32
sizeof(mqbi::DispatcherAckEvent): 80
sizeof(mqbi::DispatcherControlMessageEvent): 192
mqbi::DispatcherEvent::reset():
       total: 51.14 ms (100000000 iterations)
    per call: 0 ns

@678098 678098 requested a review from a team as a code owner August 9, 2024 23:58
@678098 678098 force-pushed the t2463_DispatcherEvent_opt branch from e16b562 to ef96296 Compare August 10, 2024 00:19
DispatcherEvent& setType(DispatcherEventType::Enum value);
inline DispatcherPutEvent& makePutEvent()
{
d_type = DispatcherEventType::e_PUT;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we even need to set this type anymore?

Comment on lines 1285 to 1277
bsl::shared_ptr<bdlbb::Blob> d_blob_sp;
// Blob of data embedded in this
// event. Refer to the corresponding
// accessor on the various
// DispatcherEvent view interfaces
// for more specific information.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
bsl::shared_ptr<bdlbb::Blob> d_blob_sp;
// Blob of data embedded in this
// event. Refer to the corresponding
// accessor on the various
// DispatcherEvent view interfaces
// for more specific information.
/// Blob of data embedded in this
/// event. Refer to the corresponding
/// accessor on the various
/// DispatcherEvent view interfaces
/// for more specific information.
bsl::shared_ptr<bdlbb::Blob> d_blob_sp;

Docstring style

{
// NOTHING
}

/// Destructor.
virtual ~DispatcherAckEvent();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need virtual destructors still?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, will clean up this

Comment on lines 395 to 398
// TODO refactor
if (mqbi::DispatcherEventType::e_PUSH == ev.type()) {
const_cast<mqbi::DispatcherPushEvent*>(ev.asPushEvent())
->setClusterNode(d_clusterNode_p);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the refactoring here to make asPushEvent and family to have a const and non-const overload?

.setType(mqbi::DispatcherEventType::e_CALLBACK)
.setCallback(mqbi::Dispatcher::voidToProcessorFunctor(f))
.setDestination(const_cast<mqbi::DispatcherClient*>(d_client_p));
.setDestination(const_cast<mqbi::DispatcherClient*>(d_client_p))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Little wary of this const cast, what's the reason this is okay?

.setSource(this)
.makePushEvent()
.setBlob(appData)
.setOptions(options)
.setGuid(msgGUID)
Copy link
Collaborator Author

@678098 678098 Aug 12, 2024

Choose a reason for hiding this comment

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

Note that initially we are building DispatcherEvent, and after makePushEvent() we are calling setters for DispatcherPushEvent. So we switch between objects amidst these setters calls. This interface is not perfect, but I have plans to replace it by calling constructors with all args instead.

So instead of this

(*dispEvent)
        .setSource(this)  // DispatcherEvent
        .makePushEvent()  // !!! DispatcherPushEvent
        .setBlob(appData) // DispatcherPushEvent
        .setOptions(options) // DispatcherPushEvent
        .setGuid(msgGUID) // DispatcherPushEvent

We will have smth like this:

(*dispEvent)
        .setSource(this)
        .makePushEvent(appData, options, msgGUID, ...)

I plan to do it in a separate PR, with performance benchmarks, to limit the number of changes in this PR.

@678098 678098 force-pushed the t2463_DispatcherEvent_opt branch from 5870886 to adf9611 Compare August 14, 2024 18:14
@678098 678098 force-pushed the t2463_DispatcherEvent_opt branch 2 times, most recently from 9c1186a to 4f7f605 Compare September 18, 2024 16:29
@678098 678098 force-pushed the t2463_DispatcherEvent_opt branch from 4f7f605 to 2f56f75 Compare October 8, 2024 19:37
Signed-off-by: Evgeny Malygin <emalygin@bloomberg.net>
@678098 678098 force-pushed the t2463_DispatcherEvent_opt branch from 2f56f75 to 6f1e149 Compare October 24, 2024 13:06
Signed-off-by: Evgeny Malygin <emalygin@bloomberg.net>
@678098 678098 force-pushed the t2463_DispatcherEvent_opt branch from 6f1e149 to 4968e09 Compare October 24, 2024 17:43
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