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

Performance issue in MapProxyImpl.readFromEventJournal with projection #11410

Closed
lukasherman opened this issue Sep 21, 2017 · 5 comments
Closed

Performance issue in MapProxyImpl.readFromEventJournal with projection #11410

lukasherman opened this issue Sep 21, 2017 · 5 comments

Comments

@lukasherman
Copy link
Contributor

@lukasherman lukasherman commented Sep 21, 2017

The application wastes CPU cycles on serialization here:

projection = serializationService.toObject(serializationService.toData(projection));

when called from Jet StreamEventJournalP.poll() repeatedly.

Example: 10 active SourceProcessors.streamMap() instances generate 20% cpu constant load on empty maps.
I would expect the projection to be applied only when event arrives.

@mmedenjak mmedenjak added this to the 3.9 milestone Sep 21, 2017
@mmedenjak
Copy link
Contributor

@mmedenjak mmedenjak commented Sep 21, 2017

Thanks @lukasherman for reporting it. I saw that and I already have a fix in mind, I just haven't gotten around to making the PR. It will be included in 3.9 GA.

mmedenjak added a commit to mmedenjak/hazelcast that referenced this issue Sep 21, 2017
Inject all managed contexts into user supplied objects. This includes
the Spring context and Node which were previously not injected. The
fix also avoids cloning the object to inject the dependencies as that
can cause a performance issue.

Fixes : hazelcast#11410
mmedenjak added a commit to mmedenjak/hazelcast that referenced this issue Sep 22, 2017
Inject all managed contexts into user supplied objects. This includes
the Spring context and Node which were previously not injected. The
fix also avoids cloning the object to inject the dependencies as that
can cause a performance issue.

Fixes : hazelcast#11410
mmedenjak added a commit to mmedenjak/hazelcast that referenced this issue Sep 28, 2017
We perform defensive cloning in some cases such as:
- receiving user supplied objects
- providing hazelcast internal object to the user
If the user makes a promise not to mutate the object or provide state
which changes during cloning, we can skip cloning and gain performance.

See:
hazelcast#11410
@tombujok
Copy link
Contributor

@tombujok tombujok commented Oct 4, 2017

@mmedenjak Matko, could we have this new method added for jet ASAP, where we do not clone?
Jet should clone their projections/aggregations just once instead of cloning in every call. This is a quick fix, the Immutable fix we have decided to postpone to 3.9.1 or 4.0.
We need to close this asap or move it to 3.9.1.

@jerrinot jerrinot modified the milestones: 3.9, 3.9.1 Oct 4, 2017
@tombujok
Copy link
Contributor

@tombujok tombujok commented Oct 4, 2017

@mmedenjak ok, we're actually moving it to 3.9.1.

@mmedenjak
Copy link
Contributor

@mmedenjak mmedenjak commented Oct 4, 2017

I would rather that we avoid cloning on this method in 3.9 as it is invoked repeatedly (unlike other methods accepting projections and aggregations) and does not use the query engine so not the same reasons for cloning may apply.

mmedenjak added a commit to mmedenjak/hazelcast that referenced this issue Oct 5, 2017
The projection was being cloned for defensive reasons. This usually
applies when calling methods using the query engine as user supplied
objects can carry state that will negatively affect the results. In
this case, it is acceptable to clone the user supplied objects since
the method is called only once and all results are returned.
This is not the case for the readFromEventJournal method - it does not
use the query engine and possibly does not suffer from such issues and
the method is called repeatedly to receive new events. This increases
the overhead of cloning.
The fix will skip cloning and emphasises that the caller may clone the
objects once to avoid any unwanted state and to cache the cloned object
to avoid the overhead.

Fixes: hazelcast#11410
@mmedenjak
Copy link
Contributor

@mmedenjak mmedenjak commented Oct 5, 2017

@tombujok I've amended the PR and I think it's simple enough to merge in 3.9 and will allow jet to be more performant when using this API without upgrading to 3.9.1. But if you feel like this is too risky, feel free to move the PR to 3.9.1 as well.

tombujok added a commit that referenced this issue Oct 12, 2017
The projection was being cloned for defensive reasons. This usually
applies when calling methods using the query engine as user supplied
objects can carry state that will negatively affect the results. In
this case, it is acceptable to clone the user supplied objects since
the method is called only once and all results are returned.
This is not the case for the readFromEventJournal method - it does not
use the query engine and possibly does not suffer from such issues and
the method is called repeatedly to receive new events. This increases
the overhead of cloning.
The fix will skip cloning and emphasises that the caller may clone the
objects once to avoid any unwanted state and to cache the cloned object
to avoid the overhead.

Fixes: #11410
@tombujok tombujok modified the milestones: 3.9.1, 3.9 Oct 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

4 participants
You can’t perform that action at this time.