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

LUCENE-10605: fix error in 32bit jvm object alignment gap calculation #949

Merged
merged 1 commit into from
Jun 10, 2022

Conversation

wormday
Copy link
Contributor

@wormday wormday commented Jun 8, 2022

Description

https://issues.apache.org/jira/browse/LUCENE-10605
ArrayUtil.oversize()
The generated array length cannot eliminate the Object alignment gap, wasting some space

Copy link
Contributor

@zhaih zhaih left a comment

Choose a reason for hiding this comment

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

Thanks, overall LGTM, some minor comments on docs

Copy link
Contributor

@uschindler uschindler left a comment

Choose a reason for hiding this comment

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

I think this is a non-risky change. If you did some tests and this behaves better, I am fine!

@wormday wormday requested a review from zhaih June 9, 2022 09:27
Copy link
Contributor

@zhaih zhaih left a comment

Choose a reason for hiding this comment

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

Looks great! Thank you for adding the explanation!

Copy link
Contributor

@zhaih zhaih left a comment

Choose a reason for hiding this comment

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

Please add an entry in CHANGES.txt

@wormday
Copy link
Contributor Author

wormday commented Jun 9, 2022

Please add an entry in CHANGES.txt

Thanks for reminding

@zhaih zhaih merged commit 26f21ae into apache:main Jun 10, 2022
@zhaih
Copy link
Contributor

zhaih commented Jun 10, 2022

Merged, thank you! Do you want to open a backport PR yourself? @wormday

@wormday
Copy link
Contributor Author

wormday commented Jun 10, 2022

backport PR

@zhaih Sorry, I don't know how to operate it. Is there a tutorial?

@zhaih
Copy link
Contributor

zhaih commented Jun 10, 2022

@wormday Ah basically just apply the same change to branch_9x and open a PR, because the main branch is actually next major version, so if the change is intended to be merged into lucene 9 then we have to open another PR to merge it to branch_9x

The easiest way I guess is to use git cherry-pick to pick the just merged commit.

@wormday
Copy link
Contributor Author

wormday commented Jun 10, 2022

@zhaih
Thank you very much. I see.
But there seems to be no 32bit since JDK9. Lucene9 does not run under JDK8.
If so, this change only makes sense for Lucene8.
But does the Lucene8 continue to be updated? Do I still need to update branch_9x?

@zhaih
Copy link
Contributor

zhaih commented Jun 10, 2022

Hmmm I think we'll only update Lucene8 if there's a bug need to be fixed, so not actively developed.

Let's merge this change into branch_9x anyway, since in the CHANGES.txt we claim it to be in version 9.3, at least it is not harmful

@uschindler
Copy link
Contributor

Hi,
I don't think we need to backport to 8.11 now. We will only do bug fixes anymore and a new release will be seldom. This is not a serious problem, it fixes no bug leading to corrupt indexes or a non working query. It just wastes heap space.

About 32 bit: You can download 32 bit versions of Java also after 9. Oracle does not have them anymore, but other vendors for sure do, e.g. Adoptium: https://adoptium.net/temurin/archive

We don't test them anymore since Lucenes 9, but we could reenable it.

P.S. for backports to 9.x separate pull requests are not needed. For simple changes like this the person who merged the pull request can easily do a simple cherry pick. I do that in most cases automatically. Costs a few seconds and pushing the change to other branch. The additional PR is IMHO too noisy also on mailing lists and really not needed.
Uwe

shaie pushed a commit to mdmarshmallow/lucene that referenced this pull request Jun 22, 2022
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.

3 participants