Skip to content

perf: avoid double checking value0 Future. #10972

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 3 commits into
base: 2.13.x
Choose a base branch
from
Open

perf: avoid double checking value0 Future. #10972

wants to merge 3 commits into from
+18 −6

Conversation

He-Pin
Copy link
Contributor

@He-Pin He-Pin commented Jan 3, 2025

Motivation:

The current Future.isComplete check the value0 once, and then it will checked it again to extract the value.

Modification:

Use .value to extract the value at the same time.

Result:
Better performance when the future is already completed.

[info] Benchmark                                                 (pool)  (recursion)  (threads)   Mode  Cnt        Score        Error   Units
[info] CompleteFutureBenchmark.success                              fix         1024          1  thrpt    5   990403.006 ±   6431.624  ops/ms
[info] CompleteFutureBenchmark.success:gc.alloc.rate                fix         1024          1  thrpt    5        0.001 ±      0.001  MB/sec
[info] CompleteFutureBenchmark.success:gc.alloc.rate.norm           fix         1024          1  thrpt    5       ≈ 10⁻⁶                 B/op
[info] CompleteFutureBenchmark.success:gc.count                     fix         1024          1  thrpt    5          ≈ 0               counts
[info] CompleteFutureBenchmark.successOption                        fix         1024          1  thrpt    5  1265272.145 ± 336165.695  ops/ms
[info] CompleteFutureBenchmark.successOption:gc.alloc.rate          fix         1024          1  thrpt    5        0.001 ±      0.001  MB/sec
[info] CompleteFutureBenchmark.successOption:gc.alloc.rate.norm     fix         1024          1  thrpt    5       ≈ 10⁻⁶                 B/op
[info] CompleteFutureBenchmark.successOption:gc.count               fix         1024          1  thrpt    5          ≈ 0               counts
[info] CompleteFutureBenchmark.successUnwrap                        fix         1024          1  thrpt    5  1322735.278 ±  25844.808  ops/ms
[info] CompleteFutureBenchmark.successUnwrap:gc.alloc.rate          fix         1024          1  thrpt    5        0.001 ±      0.001  MB/sec
[info] CompleteFutureBenchmark.successUnwrap:gc.alloc.rate.norm     fix         1024          1  thrpt    5       ≈ 10⁻⁶                 B/op
[info] CompleteFutureBenchmark.successUnwrap:gc.count               fix         1024          1  thrpt    5          ≈ 0               counts`

The result if successOption with this modification.

I still checked adding an Future#unwrap = value0() method to avoid the lifting to Some/None ,but seems not help much.

@scala-jenkins scala-jenkins added this to the 2.13.17 milestone Jan 3, 2025
@He-Pin He-Pin mentioned this pull request Jan 3, 2025
Open
@He-Pin He-Pin force-pushed the futureAwait branch 2 times, most recently from fbd326d to dc8956b Compare January 3, 2025 09:41
@He-Pin
Copy link
Contributor Author

He-Pin commented Jan 3, 2025 

[error] -- [E149] Syntax Error: /home/travis/build/scala/scala/target/library/src_managed/main/scala3-library-src/scala/Matchable.scala:7:6 
[error] 7 |trait Matchable
[error]   |      ^^^^^^^^^
[error]   |      illegal redefinition of standard class Matchable
[error]   |
[error]   | longer explanation available when compiling with `-explain`
[error] -- [E149] Syntax Error: /home/travis/build/scala/scala/target/library/src_managed/main/scala3-library-src/scala/AnyKind.scala:7:21 
[error] 7 |final abstract class AnyKind
[error]   |                     ^^^^^^^
[error]   |                     illegal redefinition of standard class AnyKind
[error]   |
[error]   | longer explanation available when compiling with `-explain`
[error] -- [E007] Type Mismatch Error: /home/travis/build/scala/scala/src/library/scala/concurrent/package.scala:203:45 
[error] 203 |      case CompletedFuture(result) => result.get
[error]     |                                      ^^^^^^^^^^
[error]     |                                      Found:    Any
[error]     |                                      Required: T
[error]     |

@He-Pin
Copy link
Contributor Author

He-Pin commented Jan 3, 2025

refs: scala/scala3#22304

@SethTisue SethTisue added the library:concurrent Changes to the concurrency support in stdlib label Jan 6, 2025
lrytz
lrytz reviewed Jan 31, 2025
View reviewed changes
@SethTisue SethTisue marked this pull request as draft February 4, 2025 04:39
@SethTisue SethTisue added the performance the need for speed. usually compiler performance, sometimes runtime performance. label Sep 3, 2025
@He-Pin

This comment was marked as resolved.

Sign in to view
@He-Pin He-Pin closed this Sep 5, 2025
@He-Pin He-Pin reopened this Sep 5, 2025
@He-Pin
Copy link
Contributor Author

He-Pin commented Sep 5, 2025

I just checked the CompletableFuture's get method again, I think we can follow that.

@He-Pin He-Pin marked this pull request as ready for review September 5, 2025 15:16
@He-Pin He-Pin requested a review from a team as a code owner September 5, 2025 15:16
sjrd
sjrd reviewed Sep 5, 2025
View reviewed changes
He-Pin and others added 2 commits September 5, 2025 23:36
@He-Pin @sjrd
Update src/library/scala/concurrent/package.scala
0ca9173 
Co-authored-by: Sébastien Doeraene <sjrdoeraene@gmail.com>
@He-Pin
x
a86956d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lrytz lrytz lrytz left review comments

@sjrd sjrd sjrd left review comments

Assignees
No one assigned
Labels
library:concurrent Changes to the concurrency support in stdlib performance the need for speed. usually compiler performance, sometimes runtime performance.
Projects
None yet
Milestone
2.13.17
Development

Successfully merging this pull request may close these issues.
5 participants
@He-Pin @lrytz @sjrd @SethTisue @scala-jenkins