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
Draft
wants to merge 1 commit into
base: 2.13.x
Choose a base branch
from
Draft

perf: avoid double checking value0 Future. #10972

wants to merge 1 commit into from
+18 −7

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
src/library/scala/concurrent/package.scala
@@ -170,7 +173,7 @@ package concurrent {
@throws(classOf[TimeoutException])
@throws(classOf[InterruptedException])
final def ready[T](awaitable: Awaitable[T], atMost: Duration): awaitable.type = awaitable match {
case f: Future[T] if f.isCompleted => awaitable.ready(atMost)(AwaitPermission)
case f: Future[T] if f.isCompleted => awaitable
Copy link
Member

Choose a reason for hiding this comment

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

It seems we still want to validate the duration

#7767 was closed for that reason, in favor of #7768

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, just back from hometown , will check this weekend

Copy link
Member

Choose a reason for hiding this comment

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

are you still interested in this, @He-Pin?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can take a look at this , this weekend.

@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
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

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.
4 participants
@He-Pin @lrytz @SethTisue @scala-jenkins