Skip to content

HBASE-29253: Avoid allocating a new closure on every row processed by StoreScanner - #6901

Merged
Apache9 merged 1 commit into
apache:masterfrom
HubSpot:HBASE-29253/store-scanner-lambda
Apr 20, 2025
Merged

HBASE-29253: Avoid allocating a new closure on every row processed by StoreScanner#6901
Apache9 merged 1 commit into
apache:masterfrom
HubSpot:HBASE-29253/store-scanner-lambda

Conversation

@charlesconnell

@charlesconnellcharlesconnell commented Apr 11, 2025

Copy link
Copy Markdown
Contributor

I've looked at a lot of allocation profiles of RegionServers doing a read-heavy workload. Some allocations that dominate the chart can be easily avoided. The method StoreScanner#read() contains this code

 heap.recordBlockSize(blockSize -> {
if (rpcCall.isPresent()) {
rpcCall.get().incrementBlockBytesScanned(blockSize);
}
scannerContext.incrementBlockProgress(blockSize);
});

that runs for every iteration of its main loop. A closure can be created before the loop and re-used instead.

@Apache-HBase

Apache-HBase commented Apr 11, 2025

Copy link
Copy Markdown

🎊 +1 overall

VoteSubsystemRuntimeLogfileComment
+0 🆗reexec0m 28sDocker mode activated.
_ Prechecks _
+1 💚dupname0m 0sNo case conflicting files found.
+0 🆗codespell0m 0scodespell was not available.
+0 🆗detsecrets0m 0sdetect-secrets was not available.
+1 💚@author0m 0sThe patch does not contain any @author tags.
+1 💚hbaseanti0m 0sPatch does not have any anti-patterns.
_ master Compile Tests _
+1 💚mvninstall3m 22smaster passed
+1 💚compile3m 11smaster passed
+1 💚checkstyle0m 39smaster passed
+1 💚spotbugs1m 38smaster passed
+1 💚spotless0m 49sbranch has no errors when running spotless:check.
_ Patch Compile Tests _
+1 💚mvninstall3m 4sthe patch passed
+1 💚compile3m 13sthe patch passed
+1 💚javac3m 13sthe patch passed
+1 💚blanks0m 0sThe patch has no blanks issues.
+1 💚checkstyle0m 38sthe patch passed
+1 💚spotbugs1m 44sthe patch passed
+1 💚hadoopcheck12m 9sPatch does not cause any errors with Hadoop 3.3.6 3.4.0.
+1 💚spotless0m 49spatch has no errors when running spotless:check.
_ Other Tests _
+1 💚asflicense0m 11sThe patch does not generate ASF License warnings.
40m 38s
SubsystemReport/Notes
DockerClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6901/1/artifact/yetus-general-check/output/Dockerfile
GITHUB PR#6901
JIRA IssueHBASE-29253
Optional Testsdupname asflicense javac spotbugs checkstyle codespell detsecrets compile hadoopcheck hbaseanti spotless
unameLinux 03a4f2bba85c 5.4.0-1103-aws #111~18.04.1-Ubuntu SMP Tue May 23 20:04:10 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
Build toolmaven
Personalitydev-support/hbase-personality.sh
git revisionmaster / 75faaa7
Default JavaEclipse Adoptium-17.0.11+9
Max. process+thread count86 (vs. ulimit of 30000)
modulesC: hbase-server U: hbase-server
Console outputhttps://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6901/1/console
versionsgit=2.34.1 maven=3.9.8 spotbugs=4.7.3
Powered byApache Yetus 0.15.0 https://yetus.apache.org

This message was automatically generated.

@Apache-HBase

Apache-HBase commented Apr 11, 2025

Copy link
Copy Markdown

🎊 +1 overall

VoteSubsystemRuntimeLogfileComment
+0 🆗reexec0m 30sDocker mode activated.
-0 ⚠️yetus0m 2sUnprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --author-ignore-list --blanks-eol-ignore-file --blanks-tabs-ignore-file --quick-hadoopcheck
_ Prechecks _
_ master Compile Tests _
+1 💚mvninstall3m 15smaster passed
+1 💚compile0m 57smaster passed
+1 💚javadoc0m 28smaster passed
+1 💚shadedjars5m 57sbranch has no errors when building our shaded downstream artifacts.
_ Patch Compile Tests _
+1 💚mvninstall3m 3sthe patch passed
+1 💚compile0m 57sthe patch passed
+1 💚javac0m 57sthe patch passed
+1 💚javadoc0m 27sthe patch passed
+1 💚shadedjars5m 49spatch has no errors when building our shaded downstream artifacts.
_ Other Tests _
+1 💚unit241m 39shbase-server in the patch passed.
267m 55s
SubsystemReport/Notes
DockerClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6901/1/artifact/yetus-jdk17-hadoop3-check/output/Dockerfile
GITHUB PR#6901
JIRA IssueHBASE-29253
Optional Testsjavac javadoc unit compile shadedjars
unameLinux 8f9ea8ed4c0a 5.4.0-1103-aws #111~18.04.1-Ubuntu SMP Tue May 23 20:04:10 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
Build toolmaven
Personalitydev-support/hbase-personality.sh
git revisionmaster / 75faaa7
Default JavaEclipse Adoptium-17.0.11+9
Test Resultshttps://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6901/1/testReport/
Max. process+thread count4968 (vs. ulimit of 30000)
modulesC: hbase-server U: hbase-server
Console outputhttps://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6901/1/console
versionsgit=2.34.1 maven=3.9.8
Powered byApache Yetus 0.15.0 https://yetus.apache.org

This message was automatically generated.

@Apache9

Apache9 commented Apr 12, 2025

Copy link
Copy Markdown
Contributor

The java compiler will not do it automatically? Do you have microbenmark to show the improvement?

Thanks.

@charlesconnell

charlesconnell commented Apr 12, 2025•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Sure, here's a microbenchmark:


@State(Scope.Benchmark)
public class ClosureBenchmark {
@Param({ "10000" })
public int loops;
@Param({ "true", "false" })
public boolean createNewClosure;
@Benchmark
public void test(Blackhole blackhole) {
IntConsumer savedClosure = x -> {
// Do some work that captures a reference to a variable from outside the lambda, in this case blackhole,
// thus forcing the allocation of a closure.
blackhole.consume(x);
};
for (int i = 0; i < loops; i++) {
if (createNewClosure) {
IntConsumer newClosure = x -> {
// Do some work that captures a reference to a variable from outside the lambda, in this case blackhole,
// thus forcing the allocation of a closure.
// But the work inside the lambda doesn't change based on the value of i,
// so in theory the compiler could avoid creating a new closure on each loop iteration
blackhole.consume(x);
};
blackhole.consume(newClosure);
} else {
blackhole.consume(savedClosure);
}
}
}
public static void main(String[] args) throws RunnerException, IOException {
org.openjdk.jmh.Main.main(args);
}
}

which produced these results for me:


Benchmark (createNewClosure) (loops) Mode Cnt Score Error Units
ClosureBenchmark.test true 10000 thrpt 25 33571.747 ± 331.844 ops/s
ClosureBenchmark.test:·gc.alloc.rate true 10000 thrpt 25 4876.137 ± 48.299 MB/sec
ClosureBenchmark.test:·gc.alloc.rate.norm true 10000 thrpt 25 160003.792 ± 0.577 B/op
ClosureBenchmark.test:·gc.churn.G1_Eden_Space true 10000 thrpt 25 4875.828 ± 55.586 MB/sec
ClosureBenchmark.test:·gc.churn.G1_Eden_Space.norm true 10000 thrpt 25 159993.999 ± 945.061 B/op
ClosureBenchmark.test:·gc.churn.G1_Survivor_Space true 10000 thrpt 25 0.006 ± 0.001 MB/sec
ClosureBenchmark.test:·gc.churn.G1_Survivor_Space.norm true 10000 thrpt 25 0.201 ± 0.030 B/op
ClosureBenchmark.test:·gc.count true 10000 thrpt 25 1423.000 counts
ClosureBenchmark.test:·gc.time true 10000 thrpt 25 865.000 ms
ClosureBenchmark.test false 10000 thrpt 25 37243.796 ± 233.865 ops/s
ClosureBenchmark.test:·gc.alloc.rate false 10000 thrpt 25 0.541 ± 0.004 MB/sec
ClosureBenchmark.test:·gc.alloc.rate.norm false 10000 thrpt 25 16.015 ± 0.022 B/op
ClosureBenchmark.test:·gc.churn.G1_Eden_Space false 10000 thrpt 25 0.761 ± 1.164 MB/sec
ClosureBenchmark.test:·gc.churn.G1_Eden_Space.norm false 10000 thrpt 25 22.485 ± 34.382 B/op
ClosureBenchmark.test:·gc.churn.G1_Survivor_Space false 10000 thrpt 25 0.142 ± 0.217 MB/sec
ClosureBenchmark.test:·gc.churn.G1_Survivor_Space.norm false 10000 thrpt 25 4.192 ± 6.411 B/op
ClosureBenchmark.test:·gc.count false 10000 thrpt 25 5.000 counts
ClosureBenchmark.test:·gc.time false 10000 thrpt 25 11.000 ms

The test runs somewhat faster when it doesn't create a new lambda on each loop iteration (33571 op/sec versus 37243 op/sec). The allocation rate is vastly lower (4876 MB/sec versus 0.5 MB/sec).

My Java version:

openjdk version "21.0.6" 2025-01-21 LTS
OpenJDK Runtime Environment Temurin-21.0.6+7 (build 21.0.6+7-LTS)
OpenJDK 64-Bit Server VM Temurin-21.0.6+7 (build 21.0.6+7-LTS, mixed mode, sharing)

@Apache9

Apache9 commented Apr 12, 2025

Copy link
Copy Markdown
Contributor

Better put the if (createNewClosure) outside the for loop? Like

if (createNewClosure) {
for (int i = 0; i < loops; i++) {
blackhole.consume(x -> {
blackhole.consume(x);
});
}
} else {
IntConsumer savedClosure = x -> {
blackhole.consume(x);
};
for (int i = 0; i < loops; i++) {
blackhole.consume(savedClosure);
}
}

@charlesconnell

charlesconnell commented Apr 12, 2025

Copy link
Copy Markdown
ContributorAuthor

Sure. The full code is now

@State(Scope.Benchmark)
public class ClosureBenchmark {
@Param({ "10000" })
public int loops;
@Param({ "true", "false" })
public boolean createNewClosure;
@Benchmark
public void test(Blackhole blackhole) {
if (createNewClosure) {
for (int i = 0; i < loops; i++) {
IntConsumer newClosure = x -> {
blackhole.consume(x);
};
blackhole.consume(newClosure);
}
} else {
IntConsumer savedClosure = x -> {
blackhole.consume(x);
};
for (int i = 0; i < loops; i++) {
blackhole.consume(savedClosure);
}
}
}
public static void main(String[] args) throws RunnerException, IOException {
org.openjdk.jmh.Main.main(args);
}
}

and the results are similar:

Benchmark (createNewClosure) (loops) Mode Cnt Score Error Units
ClosureBenchmark.test true 10000 thrpt 25 33490.255 ± 278.683 ops/s
ClosureBenchmark.test:·gc.alloc.rate true 10000 thrpt 25 4863.875 ± 40.472 MB/sec
ClosureBenchmark.test:·gc.alloc.rate.norm true 10000 thrpt 25 160003.763 ± 0.593 B/op
ClosureBenchmark.test:·gc.churn.G1_Eden_Space true 10000 thrpt 25 4863.175 ± 43.757 MB/sec
ClosureBenchmark.test:·gc.churn.G1_Eden_Space.norm true 10000 thrpt 25 159981.872 ± 719.752 B/op
ClosureBenchmark.test:·gc.churn.G1_Survivor_Space true 10000 thrpt 25 0.006 ± 0.001 MB/sec
ClosureBenchmark.test:·gc.churn.G1_Survivor_Space.norm true 10000 thrpt 25 0.204 ± 0.040 B/op
ClosureBenchmark.test:·gc.count true 10000 thrpt 25 1411.000 counts
ClosureBenchmark.test:·gc.time true 10000 thrpt 25 852.000 ms
ClosureBenchmark.test false 10000 thrpt 25 37070.851 ± 195.065 ops/s
ClosureBenchmark.test:·gc.alloc.rate false 10000 thrpt 25 0.539 ± 0.003 MB/sec
ClosureBenchmark.test:·gc.alloc.rate.norm false 10000 thrpt 25 16.016 ± 0.022 B/op
ClosureBenchmark.test:·gc.churn.G1_Eden_Space false 10000 thrpt 25 0.761 ± 1.164 MB/sec
ClosureBenchmark.test:·gc.churn.G1_Eden_Space.norm false 10000 thrpt 25 22.643 ± 34.623 B/op
ClosureBenchmark.test:·gc.churn.G1_Survivor_Space false 10000 thrpt 25 0.142 ± 0.216 MB/sec
ClosureBenchmark.test:·gc.churn.G1_Survivor_Space.norm false 10000 thrpt 25 4.212 ± 6.441 B/op
ClosureBenchmark.test:·gc.count false 10000 thrpt 25 5.000 counts
ClosureBenchmark.test:·gc.time false 10000 thrpt 25 10.000 ms

@Apache9

Apache9 commented Apr 13, 2025

Copy link
Copy Markdown
Contributor

Please use this form

 blackhole.consume(x -> {
blackhole.consume(x);
});

Not

 IntConsumer newClosure = x -> {
blackhole.consume(x);
};
blackhole.consume(newClosure);

If compile can not pass, use casting instead of storing it to a local variable.

Apache9
Apache9 approved these changes Apr 13, 2025
View reviewed changes

@Apache9Apache9 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

OK, asked chatgpt, if a lambda captures external variables, even if these vars are not changed during the whole loop, it will still create a instance every time...

@charlesconnell

charlesconnell commented Apr 13, 2025

Copy link
Copy Markdown
ContributorAuthor

Yeah. To be sure, I just tried your suggestion like so:

@State(Scope.Benchmark)
public class ClosureBenchmark {
@Param({ "10000" })
public int loops;
@Param({ "true", "false" })
public boolean createNewClosure;
@Benchmark
public void test(Blackhole blackhole) {
if (createNewClosure) {
for (int i = 0; i < loops; i++) {
blackhole.consume(
(IntConsumer) (x -> {
blackhole.consume(x);
})
);
}
} else {
IntConsumer savedClosure = x -> {
blackhole.consume(x);
};
for (int i = 0; i < loops; i++) {
blackhole.consume(savedClosure);
}
}
}
public static void main(String[] args) throws RunnerException, IOException {
org.openjdk.jmh.Main.main(args);
}
}

and the results stay the same.

@Apache9
Apache9 merged commit abc8b43 into apache:masterApr 20, 2025
Apache9 pushed a commit that referenced this pull request Apr 20, 2025
@charlesconnell@Apache9
HBASE-29253Avoid allocating a new closure on every row processed by …
996a522
…StoreScanner (#6901)
Signed-off-by: Duo Zhang <zhangduo@apache.org>
(cherry picked from commit abc8b43)
Apache9 pushed a commit that referenced this pull request Apr 20, 2025
@charlesconnell@Apache9
HBASE-29253Avoid allocating a new closure on every row processed by …
a8ff965
…StoreScanner (#6901)
Signed-off-by: Duo Zhang <zhangduo@apache.org>
(cherry picked from commit abc8b43)
Apache9 pushed a commit that referenced this pull request Apr 20, 2025
@charlesconnell@Apache9
HBASE-29253Avoid allocating a new closure on every row processed by …
e3ed41d
…StoreScanner (#6901)
Signed-off-by: Duo Zhang <zhangduo@apache.org>
(cherry picked from commit abc8b43)
mokai87 pushed a commit to mokai87/hbase that referenced this pull request Aug 7, 2025
@charlesconnell@mokai87
HBASE-29253 Avoid allocating a new closure on every row processed by …
c84b1d2
…StoreScanner (apache#6901)
Signed-off-by: Duo Zhang <zhangduo@apache.org>
(cherry picked from commit abc8b43)
@charlesconnell
charlesconnell deleted the HBASE-29253/store-scanner-lambda branch September 19, 2025 15:33
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Apache9Apache9Apache9 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@charlesconnell@Apache-HBase@Apache9