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

Limit the row list allocation based on the row count #3756

Merged
merged 1 commit into from Mar 16, 2023

Conversation

franz1981
Copy link
Contributor

Profiling https://github.com/topicusonderwijs/tribe-krd-quarkus startup it reports a huge amount of allocations due to this:

image

and zooming in (the orange ones are "out of TLAB allocations":

image

Configuration of H2 is mem and the ORM policy is drop and create, but still, with this PR it would save a huge amount of allocations (it's a quite large model, actually).

@franz1981 franz1981 changed the title Lazy-allocate the Row list to speedup drop or create use cases Limit the row list allocation based on the row count Mar 14, 2023
@franz1981
Copy link
Contributor Author

franz1981 commented Mar 15, 2023

I've forgot to mention @katzyn
re

Configuration of H2 is mem

In the reproducer linked, the H2 version is 1.4.197 that, despite being configured as mem isn't using the in-memory branch i.e. rebuildIndexBuffered, but rebuildIndexBlockMerge, see:

if (session.getDatabase().getMvStore() == null ||
index instanceof MVSpatialIndex) {
// in-memory
rebuildIndexBuffered(session, index);
} else {
rebuildIndexBlockMerge(session, index);
}

It seems suspicious to me, but this patch still apply here, I think.
Let me know if I should fill a separate issue for it

@grandinj
Copy link
Contributor

Thank you for your contribution!

Please send a licence statement as described here
https://h2database.com/html/build.html#providing_patches
to our mailing list / Google group:
https://groups.google.com/g/h2-database

@grandinj
Copy link
Contributor

In the reproducer linked, the H2 version is 1.4.197 that, despite being configured as mem isn't using the in-memory branch i.e. rebuildIndexBuffered, but rebuildIndexBlockMerge, see:

if (session.getDatabase().getMvStore() == null ||
index instanceof MVSpatialIndex) {
// in-memory
rebuildIndexBuffered(session, index);
} else {
rebuildIndexBlockMerge(session, index);
}

It seems suspicious to me, but this patch still apply here, I think. Let me know if I should fill a separate issue for it

Does using rebuildIndexBuffered for your example make a difference?

@franz1981
Copy link
Contributor Author

franz1981 commented Mar 15, 2023

thanks @grandinj to reach out! I'll send the licence statement ASAP!

re

Does using rebuildIndexBuffered for your example make a difference?

I'm sure it will because it contains

int bufferSize = (int) Math.min(total, database.getMaxMemoryRows());

that's the key optimization to save huge arraylists to be created when no rows are found

Sole problem is that even by setting a mem store 1.4.197 isn't not able to hit that code path (that looks like a different issue to me eh!)

@grandinj
Copy link
Contributor

On current GIT master, that line of code reads

 if (!session.getDatabase().isPersistent() || index instanceof MVSpatialIndex) {

so it should work better for your case.

@franz1981
Copy link
Contributor Author

@grandinj waiting subscription to the google group to be accepted and ready to send the first msg :)

@franz1981
Copy link
Contributor Author

@grandinj email sent; given that this is not a huge patch I didn't add the full statement, let me know if I should, instead.

@grandinj grandinj merged commit 14bd90c into h2database:master Mar 16, 2023
@grandinj
Copy link
Contributor

Awesome, thanks!

@franz1981
Copy link
Contributor Author

many thanks for the quick review!!! @grandinj

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.

None yet

2 participants