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

Check target array length before allocating. #4159

Open
wants to merge 4 commits into
base: series/3.x
Choose a base branch
from

Conversation

counter2015
Copy link

The current implementation of the array inside ArrayStack does not verify the length of the new array, and this may lead to overflow problem.

private[this] def checkAndGrow(): Unit =
if (index >= buffer.length) {
val len = buffer.length
val buffer2 = new Array[AnyRef](len * 2)
System.arraycopy(buffer, 0, buffer2, 0, len)
buffer = buffer2
}

Here we add a check on

  • Int overflow
  • Array length exceed VM limit

see also: #4135

@armanbilge
Copy link
Member

Thanks for the PR! See also:

@djspiewak
Copy link
Member

Looks like we need to run prePR. But apart from that, I'd like to sanity check this with a benchmark run. This specific function is very, very easy to knock out of its JIT fastpath, and adding this type of branching very much runs that risk. Apart from that, I think this is a very good change. If we are knocking the JIT around, we might have to either experiment with different ways of encoding the branching to avoid tripping over the optimizer, or we may need to do the check in some other area.

@counter2015
Copy link
Author

Sure, I will provide a benchmark of this change later this week.

@counter2015
Copy link
Author

counter2015 commented Oct 30, 2024

I'm not sure my implementation of benchmark is right or not. Since some branch only excuted at a very large parameter, and it cant be test on old code since it will lead to overflow problem.

And I'm not familiar with how to do comparative benchmarks between versions. benchmarks/run-benchmark ArrayStackBenchmark seems doesn't work rightly, may be I missed something.

So I just do this compare manually, here is the benchmark result on my laptop.

[info] running (fork) org.openjdk.jmh.Main -i 10 -wi 10 -f 2 -t 1 cats.effect.benchmarks.ArrayStackBenchmark
[info] # JMH version: 1.37
[info] # VM version: JDK 21.0.2, OpenJDK 64-Bit Server VM, 21.0.2+13-LTS
[info] # VM options: -Dcats.effect.tracing.mode=none -Dcats.effect.tracing.exceptions.enhanced=false
[info] # Blackhole mode: compiler (auto-detected, use -Djmh.blackhole.autoDetect=false to disable)

# new version
Jmh / run -i 10 -wi 10 -f 2 -t 1 cats.effect.benchmarks.ArrayStackBenchmark
[info] Benchmark                              (size)   Mode  Cnt    Score   Error  Units
[info] ArrayStackBenchmark.push  1000000  thrpt   20  126.439 3.123  ops/s

# base line
Jmh / run -i 10 -wi 10 -f 2 -t 1 cats.effect.benchmarks.ArrayStackBenchmark
[info] Benchmark                              (size)   Mode  Cnt    Score   Error  Units
[info] ArrayStackBenchmark.push  1000000  thrpt   20  126.187  2.612  ops/s

This result indicates that when the size is small, the performance of the two is comparable.

I try to test on large size such as 1147483639 which is bigger than VM_MaxArraySize / 2, but it will cause OOM probelm during jmh test. it need 8G memory I guess ?

I changed javaOptions inside project benchmarks in build.sbt, but my laptop cant allocate such memory to it.

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