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

refactor gateway proxy factories #132

Merged

Conversation

jlabedo
Copy link
Contributor

@jlabedo jlabedo commented May 31, 2023

Simplify gateways building during messaging system build phase.

This PR is linked to #133

@jlabedo jlabedo force-pushed the refactor-gateway-proxy-factory branch from 61ff601 to ce7d508 Compare May 31, 2023 12:23
@jlabedo jlabedo requested a review from dgafka May 31, 2023 17:06
docker-compose.yml Outdated Show resolved Hide resolved
@jlabedo jlabedo force-pushed the refactor-gateway-proxy-factory branch from 804fe16 to aedf5e4 Compare May 31, 2023 21:23
@dgafka
Copy link
Member

dgafka commented Jun 1, 2023

I think we lost some performance along the way for the scenario where Ecotone is executed.

Running Ecotone on Master Branch
Running Ecotone Application in this PR

I think what happens right now is MessagingSystem::configureGateways is called on each execution. Which means we are creating proxies even if we are not using them at all. Before we had this lazy load, which delayed the creation of Gateway till the moment it was actually executed.

I would love this PR to be merged as it simplifies the code and lower the barrier for understanding what actually happens.
However we need to tackle this lazy creation.
Maybe instead of building it on boot, we can move the creation to getGatewayByName and build only the gateway that is requested. Wdyt?

PS. Your benchmarks are doing the job ;)

@dgafka
Copy link
Member

dgafka commented Jun 1, 2023

Maybe instead of building it on boot, we can move the creation to getGatewayByName and build only the gateway that is requested. Wdyt?

PS. Your benchmarks are doing the job ;)

You may take a look on how pollingConsumerBuilders are built. Only the one chosen to be run is built and then built consumer is cached, so in case of reruning it will be reused.

@jlabedo
Copy link
Contributor Author

jlabedo commented Jun 1, 2023

Maybe instead of building it on boot, we can move the creation to getGatewayByName and build only the gateway that is requested. Wdyt?

This is actually my final goal, this PR was just for understanding the MessagingSystem and clean the room before moving forward.

From my actual understanding, MessagingSystem is mainly a mix between a container builder and a container. I think it may be dropped for a classic container implementation, left to Lite/Symfony/Laravel responsibility, and some of the code thrown to MessagingSystemConfiguration

I think what happens right now is MessagingSystem::configureGateways is called on each execution. Which means we are creating proxies even if we are not using them at all. Before we had this lazy load, which delayed the creation of Gateway till the moment it was actually executed.

I should have looked at it ! I missed something with this lazy boot.

@jlabedo
Copy link
Contributor Author

jlabedo commented Jun 1, 2023

@dgafka , the performance is the same on my machine:

branch: main
    bench_running_ecotone_lite..............I4 - Mo150.045ms (±0.61%)
    bench_running_ecotone_lite_with_fail_fa.I4 - Mo158.993ms (±0.97%)
    bench_running_ecotone_lite_with_cache...I4 - Mo14.784ms (±1.16%)

branch: PR
    bench_running_ecotone_lite..............I4 - Mo148.963ms (±0.48%)
    bench_running_ecotone_lite_with_fail_fa.I4 - Mo157.774ms (±1.06%)
    bench_running_ecotone_lite_with_cache...I4 - Mo14.751ms (±0.39%)

The problem with benchmarks on CI is that you cannot trust the hardware the benchmark is running on (it may be shared computing power, or even different cpus).
Moreover, I have left you a comment on your PR, as xdebug is enabled in CI which is not ideal : #131 (comment)

Speaking of this PR, my understanding was that all gateways were currently built on boot. The final goal discussed in #133 is to lazy build everything. And this PR is just cleaning before moving on. Or maybe you prefer a single big PR ?

@dgafka
Copy link
Member

dgafka commented Jun 1, 2023

The problem with benchmarks on CI is that you cannot trust the hardware the benchmark is running on (it may be shared computing power, or even different cpus).

Ah true, really good point :)

From my actual understanding, MessagingSystem is mainly a mix between a container builder and a container. I think it may be dropped for a classic container implementation, left to Lite/Symfony/Laravel responsibility, and some of the code thrown to MessagingSystemConfiguration

MessagingSystem is actually connected messaging system.
As MessagingConfiguration holds the configurations, yet it does not resolve them anyhow.
This part of docs explains it in more details.
In messaging system everything is decoupled and may be connected together by Message Channels and Messages, that is also why it's so easily to change the flow adding interceptors for example.
In case of Messaging System if you will take a look on eventDrivenConsumers they are booted in order to connect to each other, which allows for the message flow.
Right now Ecotone have great optimalization, but in ideal implementation we could go even further.
As right now each Event Driven Handler is booted (and Lazy Config is configured), but we could boot only the Event Driven Consumers that are related to given message flow. You can imagine that this could lead to extremely efficient system.

That's why when you say you would love to see more optimalization in place, I am leaving the space for you to explore.
Let's see along the way where we can reach with that and I will share the knowledge with you along the way.

@dgafka dgafka merged commit a29c7de into ecotoneframework:main Jun 1, 2023
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