Skip to content

Conversation

Meehdi
Copy link

@Meehdi Meehdi commented Oct 9, 2025

This aims to reduce memory usage by reducing allocations when resolving dirty attribute indexes

Changes

I added the tests before making the changes to make sure my modifications didn't break the logic afterwards.

This optimizes memory usage by replacing ArrayList with BitSet for better memory allocation by:
Eliminating boxing overhead (Integer objects)
More efficient lookup with BitSet comapred to ArrayList "contains"
Converting to int[] only at the end and allocating exact size using bitSet.cardinality()

Related Issue

Fixes HHH-19850


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license
and can be relicensed under the terms of the LGPL v2.1 license in the future at the maintainers' discretion.
For more information on licensing, please check here.


https://hibernate.atlassian.net/browse/HHH-19850

…indexes

This aims to reduce memory usage by reducing allocations when resolving
Copy link
Member

@mbellade mbellade left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution @Meehdi, I left a few comments.

As with any primarily performance-related changes, we usually run (micro)benchmarks and profiling to verify we are actually seeing improvements in real-world scenarios, see for example this repository.

Have you tried verifying if there is a significant gain (or loss) in performance with this change?

Comment on lines +2483 to +2484
if ( propertyUpdateability[position] && !fields.get( position ) ) {
fields.set( position );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to check !fields.get( position ), as setting an already-set bit is idempotent.

Comment on lines +2507 to +2508
if ( index != null && propertyUpdateability[index] && !fields.get( index ) ) {
fields.set( index );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See previous comment

@JiraKey("HHH-19850")
public class AbstractEntityPersisterTest extends BaseCoreFunctionalTestCase {

private AbstractEntityPersister persister;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have existing tests which should check the behavior of the method you changed, no need to create one which artificially tests it through internal-only calls. It would be great if you could modify an existing one instead, adding any assertions if needed.

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