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

Reduce ArrayUtil#grow in decompress #12996

Merged
merged 6 commits into from Feb 25, 2024

Conversation

easyice
Copy link
Contributor

@easyice easyice commented Jan 6, 2024

Description

@@ -128,10 +128,12 @@ public void decompress(DataInput in, int originalLength, int offset, int length,
}

// Read blocks that intersect with the interval we need
if (offsetInBlock < offset + length) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if growExact would be better here. I think grow will try to oversize it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kept the logic that if the new length is less than the current array then we don't do anything. it appears in some cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather keep using grow over growExact. This helps make sure we don't keep allocating a new array in cases lengths grow in small increments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, it's fixed :)

Copy link

This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the dev@lucene.apache.org list. Thank you for your contribution!

@github-actions github-actions bot added the Stale label Jan 23, 2024
Copy link
Contributor

@vigyasharma vigyasharma left a comment

Choose a reason for hiding this comment

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

This is a neat and simple optimization. We grow the bytes array to hold all the decompressed data in one shot, instead of doing it in multiples of blockLength.

Thanks for this improvement @easyice !

@github-actions github-actions bot removed the Stale label Feb 21, 2024
@vigyasharma
Copy link
Contributor

@easyice Let's add a changes.txt entry for this?

@easyice
Copy link
Contributor Author

easyice commented Feb 21, 2024

Thank you for reviewing! @vigyasharma

I have added the CHANGES entry under Lucene 9.11.0

@vigyasharma vigyasharma merged commit ca06693 into apache:main Feb 25, 2024
3 checks passed
@jpountz
Copy link
Contributor

jpountz commented Apr 9, 2024

@vigyasharma I wonder if you missed to backport this change to branch_9x?

@vigyasharma
Copy link
Contributor

@jpountz my bad, I think I missed the back-port. Merged it now.

@jpountz
Copy link
Contributor

jpountz commented Apr 10, 2024

Thank you!

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

4 participants