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

HSEARCH-2576 FacetingManager should allocate maps lazily #1306

Merged
merged 2 commits into from Feb 7, 2017

Conversation

@yrodiere

This comment has been minimized.

Copy link
Member

yrodiere commented Feb 7, 2017

Jenkins, test this please

Copy link
Member

yrodiere left a comment

Looks good to me. I'll merge when the build finishes.

@Sanne I suppose you created this ticket following performance testing in Infinispan?

@Sanne

This comment has been minimized.

Copy link
Member Author

Sanne commented Feb 7, 2017

I suppose you created this ticket following performance testing in Infinispan?

I have been busy with performance testing, but actually not (yet) with Infinispan..

It's actually amazing what a difference this small change makes: we have a flaw in our container based performance tests, which triggers an occasional deadlock in H2 when we're "too fast". After this change, that test fails much more frequently (from "almost never" to "almost all the time" .. on my pc at least).

@yrodiere

This comment has been minimized.

Copy link
Member

yrodiere commented Feb 7, 2017

I have been busy with performance testing, but actually not (yet) with Infinispan..

So you used the existing performance testing project from our own code? Great :)

After this change, that test fails much more frequently (from "almost never" to "almost all the time" .. on my pc at least).

I suppose there's no way to fix this? Maybe upgrade H2? It would be a shame if, after merging this PR, our CI became useless because it would always fail...

@Sanne

This comment has been minimized.

Copy link
Member Author

Sanne commented Feb 7, 2017

I suppose there's no way to fix this? Maybe upgrade H2? It would be a shame if, after merging this PR, our CI became useless because it would always fail...

Yes, let's keep an eye on this.. I hope not as CI is quite slow. let's not upgrade it! :D More seriously, yes fixing the test is on my watchlist..

So you used the existing performance testing project from our own code? Great :)

I have some more perf tests as well, being developed. Will need to tidy them up eventually and share, it's currently a mess of experiments and various branches trying different things, e.g. different combinations of dependencies.

Moving ahead we'd want to have nightly performance regression reports built by CI.

@yrodiere yrodiere force-pushed the Sanne:facetingPerfImprovements branch from 7b46c75 to 22891b4 Feb 7, 2017
@yrodiere yrodiere merged commit 22891b4 into hibernate:master Feb 7, 2017
1 check was pending
1 check was pending
default Build started sha1 is merged.
Details
@yrodiere

This comment has been minimized.

Copy link
Member

yrodiere commented Feb 7, 2017

Merged, thanks!

We'll see how well the CI behaves and prioritize this H2 locking bug accordingly.

@Sanne Sanne deleted the Sanne:facetingPerfImprovements branch Feb 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.