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 regression on predicate queries #12018

Closed
yozank opened this issue Dec 18, 2017 · 18 comments
Closed

Performance regression on predicate queries #12018

yozank opened this issue Dec 18, 2017 · 18 comments

Comments

@yozank
Copy link
Contributor

@yozank yozank commented Dec 18, 2017

Performace of predicate queries are still ~2x slower on (3.9.1 and 3.8.7) than 3.7.x.
#11705 resolved most of the performance degradation. When setting hazelcast.index.copy.behavior=NEVER, we expect to get the same performance as 3.7.x. However, it is ~2x slower. I am sharing the screenshot of test results:

screen shot 2017-12-18 at 13 25 28

The tested query includes an AND query with 7 indexed fields of a map.
I can share the code if you need.

@tombujok
Copy link
Contributor

@tombujok tombujok commented Dec 18, 2017

Initial bug: #11280 and a fix that didn't solve it completely #11705

@taburet
Copy link
Contributor

@taburet taburet commented Dec 19, 2017

@yozank could you please share the code.

@yozank
Copy link
Contributor Author

@yozank yozank commented Dec 19, 2017

@taburet shared on Slack

@mmedenjak
Copy link
Contributor

@mmedenjak mmedenjak commented Dec 19, 2017

@yozank I can't seem to find the code. Can you paste a link to the slack message?

@yozank
Copy link
Contributor Author

@yozank yozank commented Dec 19, 2017

@mmedenjak shared on #core as hz_benchmark.zip

@taburet
Copy link
Contributor

@taburet taburet commented Dec 22, 2017

After profiling it's clear that on recent Hazelcast versions we are just unable to saturate the CPU fully. On 3.7.2 queries are executed on a common client thread pool, by default thread count is calculated as number of cores multiplied by 20. On more recent versions queries are executed on a separate thread pool, by default thread count is calculated as number of cores multiplied by 1. See ClientEngineImpl#newClientExecutor and ClientEngineImpl#newClientQueryExecutor for more details.

The workaround is to set hazelcast.clientengine.query.thread.count manually to some reasonable number. A possible fix is raising ClientEngineImpl#QUERY_THREADS_PER_CORE multiplier while calculating default thread count.

@mmedenjak
Copy link
Contributor

@mmedenjak mmedenjak commented Jan 3, 2018

@yozank can you confirm this fixes the issue?

@yozank
Copy link
Contributor Author

@yozank yozank commented Jan 3, 2018

@mmedenjak yes I confirm that suggested workaround fixes the issue.

@Holmistr
Copy link
Contributor

@Holmistr Holmistr commented Jan 30, 2018

@taburet Given the fact that your workaround worked, maybe you could apply the fixed that you suggested?

A possible fix is raising ClientEngineImpl#QUERY_THREADS_PER_CORE multiplier while calculating default thread count.
@mmedenjak
Copy link
Contributor

@mmedenjak mmedenjak commented Jan 30, 2018

Discussion on slack: https://hazelcast.slack.com/archives/C8MGBBY3U/p1514987744000166
The regression was introduced because of this: #9581
Basically the PR that introduced it avoids OOME and full GCs by lowering the thread count - #9568

From the looks of it, it’s preferable to keep the lowered thread count by default to avoid OOME and full GCs and have a configuration to increase the thread count or use a different thread pool in case a client is heavy on the queries and can sustain a high thread count.

As the client can increase the thread count by setting hazelcast.clientengine.thread.count and hazelcast.clientengine.query.thread.count, I think we don't need to apply any fix.

@Holmistr
Copy link
Contributor

@Holmistr Holmistr commented Jan 31, 2018

@mmedenjak So if I understand it correctly, we don't want to apply any fix, but this should be clearly documented, right?

@mmedenjak
Copy link
Contributor

@mmedenjak mmedenjak commented Jan 31, 2018

This is how I see it. @sertugkaya @yozank WDYT?

@sertugkaya
Copy link
Contributor

@sertugkaya sertugkaya commented Jan 31, 2018

Yes, as long as it's clearly documented, all good.

@Holmistr
Copy link
Contributor

@Holmistr Holmistr commented Feb 1, 2018

@Serdaro Can you please take it from here? I will be very happy to assist you if you needed anything.

@Serdaro
Copy link
Member

@Serdaro Serdaro commented Feb 1, 2018

@Holmistr, I checked all the mentioned issues/PRs, I'd be glad to get a hand on how to convey the resulting information. It shouldn't be just like "for the 3.8.x and higher releases you should increase thread.count and query.thread.count to a "reasonable" number to not face some decreased performance"

@mmedenjak
Copy link
Contributor

@mmedenjak mmedenjak commented Feb 1, 2018

I guess it should be something along the lines of:

  • changed default thread count to avoid OOME : #9581
  • if you are experiencing performance regression and can tolerate a higher thread count, increase them by using the 2 properties
@Serdaro
Copy link
Member

@Serdaro Serdaro commented Feb 1, 2018

thank you @mmedenjak

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.

None yet
8 participants