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

Fix CallbackStack leak, restore specialized IODeferred #3403

Merged
merged 7 commits into from
Feb 5, 2023

Conversation

armanbilge
Copy link
Member

@armanbilge armanbilge commented Feb 5, 2023

Closes #3366.

This PR

[info] Benchmark                   (count)   Mode  Cnt     Score    Error  Units
[info] DeferredBenchmark.cancel         10  thrpt   20    38.933 ±  0.777  ops/s
[info] DeferredBenchmark.cancel        100  thrpt   20     5.323 ±  0.401  ops/s
[info] DeferredBenchmark.cancel       1000  thrpt   20     0.395 ±  0.028  ops/s
[info] DeferredBenchmark.complete       10  thrpt   20    35.443 ±  4.403  ops/s
[info] DeferredBenchmark.complete      100  thrpt   20    10.200 ±  2.016  ops/s
[info] DeferredBenchmark.complete     1000  thrpt   20     1.687 ±  0.058  ops/s
[info] DeferredBenchmark.get            10  thrpt   20  1880.096 ± 12.066  ops/s
[info] DeferredBenchmark.get           100  thrpt   20   277.012 ±  2.158  ops/s
[info] DeferredBenchmark.get          1000  thrpt   20    28.974 ±  0.091  ops/s

3.4.x

[info] Benchmark                   (count)   Mode  Cnt    Score    Error  Units
[info] DeferredBenchmark.cancel         10  thrpt   20   42.868 ±  1.431  ops/s
[info] DeferredBenchmark.cancel        100  thrpt   20    4.399 ±  0.254  ops/s
[info] DeferredBenchmark.cancel       1000  thrpt   20    0.361 ±  0.025  ops/s
[info] DeferredBenchmark.complete       10  thrpt   20   25.492 ±  2.345  ops/s
[info] DeferredBenchmark.complete      100  thrpt   20    8.211 ±  1.625  ops/s
[info] DeferredBenchmark.complete     1000  thrpt   20    1.216 ±  0.271  ops/s
[info] DeferredBenchmark.get            10  thrpt   20  906.738 ± 83.481  ops/s
[info] DeferredBenchmark.get           100  thrpt   20  166.403 ±  4.774  ops/s
[info] DeferredBenchmark.get          1000  thrpt   20   19.388 ±  1.687  ops/s

@armanbilge armanbilge changed the base branch from series/3.x to series/3.4.x February 5, 2023 03:26
@armanbilge armanbilge closed this Feb 5, 2023
@armanbilge armanbilge reopened this Feb 5, 2023
@armanbilge armanbilge marked this pull request as ready for review February 5, 2023 03:51
djspiewak
djspiewak previously approved these changes Feb 5, 2023
Copy link
Member

@djspiewak djspiewak left a comment

Choose a reason for hiding this comment

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

image

@durban
Copy link
Contributor

durban commented Feb 5, 2023

It is not closely related to this PR, but I think the CallbackStack in IOFiber will also need to do this. (If I understand it correctly, cancelling a lot of joins would cause the same leak.)

@djspiewak
Copy link
Member

It is not closely related to this PR, but I think the CallbackStack in IOFiber will also need to do this. (If I understand it correctly, cancelling a lot of joins would cause the same leak.)

There's a space/time tradeoff here. In order to do that, IOFiber would need to track some additional state (the AtomicInteger isn't free!), and join cancelation would become proportionally more expensive. In essence, we would need to deoptimize the common case of IOFiber (which is to say, not having many joiners, and not canceling them very often when we do) in order to optimize the uncommon case. I think we can be pretty confident in the uncommonness by virtue of the fact that no one has complained about this in the last two and a half years, despite the fact that it's been like this the whole time.

Really it's the observation that Deferred#get and Fiber#join are just used in slightly different patterns, and so Deferred needs to be a bit more space-efficient in the face of cancelation, while IOFiber can trade off some wasted space in order to get a more efficient fiber footprint and faster cancel.

@armanbilge armanbilge merged commit f6adf0f into typelevel:series/3.4.x Feb 5, 2023
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.

Restore specialized Deferred
3 participants