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

Use growNoCopy in some places #12951

Merged
merged 4 commits into from
Feb 4, 2024
Merged

Use growNoCopy in some places #12951

merged 4 commits into from
Feb 4, 2024

Conversation

easyice
Copy link
Contributor

@easyice easyice commented Dec 16, 2023

Found some grow method can be change to growNoCopy when reading code.

@@ -60,6 +60,13 @@ public void grow(int capacity) {
ref.bytes = ArrayUtil.grow(ref.bytes, capacity);
}

/**
* Used to grow the builder without coping bytes. see {@link ArrayUtil#growNoCopy(byte[], int)}.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Used to grow the builder without coping bytes. see {@link ArrayUtil#growNoCopy(byte[], int)}.
* Used to grow the builder without copying bytes. see {@link ArrayUtil#growNoCopy(byte[], int)}.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @dweiss , sorry for the typo!

Copy link

github-actions bot commented Jan 8, 2024

This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the [email protected] list. Thank you for your contribution!

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

Choose a reason for hiding this comment

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

I think toUTF16 is another method in this class that can use growNoCopy. But it would require implementing it for IntsRefBuilder. Which can lead to using it in a few other places where we call IntsRefBuilder#grow now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice find! Thank you @epotyom !

@github-actions github-actions bot removed the Stale label Jan 19, 2024
# Conflicts:
#	lucene/codecs/src/java/org/apache/lucene/codecs/simpletext/SimpleTextDocValuesReader.java
Copy link
Contributor

@epotyom epotyom left a comment

Choose a reason for hiding this comment

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

Nice improvement, approved!

@easyice
Copy link
Contributor Author

easyice commented Jan 19, 2024

Thank you for reviewing!

Copy link

github-actions bot commented Feb 3, 2024

This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the [email protected] list. Thank you for your contribution!

@github-actions github-actions bot added the Stale label Feb 3, 2024
@easyice
Copy link
Contributor Author

easyice commented Feb 4, 2024

@epotyom @dweiss Hi, can you help me to merge if it looks okay?

Copy link
Contributor

@dweiss dweiss left a comment

Choose a reason for hiding this comment

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

LGTM. Will merge shortly. Thank you.

@dweiss dweiss merged commit c02f547 into apache:main Feb 4, 2024
4 checks passed
@dweiss dweiss added this to the 9.10.0 milestone Feb 4, 2024
@dweiss dweiss self-assigned this Feb 4, 2024
@dweiss
Copy link
Contributor

dweiss commented Feb 4, 2024

I've backported to branch_9x as well.

asfgit pushed a commit that referenced this pull request Feb 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants