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 some unnecessary ArrayUtil#grow calls #13171

Closed
wants to merge 1 commit into from

Conversation

easyice
Copy link
Contributor

@easyice easyice commented Mar 9, 2024

This change will slightly improve the performance of DataOutput#writeGroupVInts and some other methods related to XXXRefBuilder#append.

Here is a JMH benchmark for Util.toIntsRef, using java 21 on my MAC (intel chip).

Benchmark                    Mode  Cnt   Score   Error   Units
PR   ToIntsRefBenchMark.run  thrpt    5  38.208 ± 1.054  ops/us
main ToIntsRefBenchMark.run  thrpt    5   7.862 ± 0.832  ops/us

JMH benchmark code for Util#toIntsRef:

public class ToIntsRefBenchMark {
  IntsRefBuilder intsRefBuilder = new IntsRefBuilder();
  BytesRef bytesRef = new BytesRef(new byte[32]);

  @Benchmark
  public void run() {
    Util.toIntsRef(bytesRef, intsRefBuilder);
  }
}

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 Mar 24, 2024
@stefanvodita
Copy link
Contributor

@easyice - I'm probably missing something obvious, but I thought ArrayUtil.grow already made the same check. Can you explain why this PR helps?

public static byte[] grow(byte[] array, int minSize) {
assert minSize >= 0 : "size must be positive (got " + minSize + "): likely integer overflow?";
if (array.length < minSize) {
return growExact(array, oversize(minSize, Byte.BYTES));
} else return array;
}

@easyice
Copy link
Contributor Author

easyice commented Mar 25, 2024

@stefanvodita Thank you for looking into this! when an array does not need to grow, The ArrayUtil.grow will return the input array, then it will call an extra assignment like ref.bytes = ref.bytes in BytesRefBuilder#grow. I observed there was some performance impact, which was noticeable in the append method. Maybe i'm being overly picky, Overall, it doesn't have a significant impact on performance.

@stefanvodita
Copy link
Contributor

Thanks for the explanation! The way I think about it is whether it would be ok to make this check every time we call grow. Probably not, it's nice that grow does the check for you. Maybe simplicity is worth more than the performance gain here? If you think the performance win is compelling, I won't argue against it.

@github-actions github-actions bot removed the Stale label Mar 26, 2024
@easyice
Copy link
Contributor Author

easyice commented Mar 26, 2024

I originally wanted to speed up writeGroupVInts in this way. Now there is a better way to speed up it (by #13203) . I agree with you that only make the change where performance is important to keep things simple, Currently, I haven't seen any other places yet, so i will close it. 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.

2 participants