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

Remove final fields startIO and startEC #1773

Merged
merged 9 commits into from
Mar 19, 2021
Merged

Remove final fields startIO and startEC #1773

merged 9 commits into from
Mar 19, 2021

Conversation

vasilmkd
Copy link
Member

@vasilmkd vasilmkd commented Mar 10, 2021

Because we used startIO and startEC as part of execR, this escaped the constructor of IOFiber and the constructor arguments became (final) fields after compilation, resulting in unnecessary bloat to each fiber. On top of that, we were unnecessarily holding onto the root IO object, for the whole lifetime of the fiber.

Before:

Compiled from "IOFiber.scala"
public final class cats.effect.IOFiber<A> extends cats.effect.IOFiberPlatform<A> implements cats.effect.kernel.Fiber<cats.effect.IO, java.lang.Throwable, A>, java.lang.Runnable {
  private final int initMask;

  private final cats.effect.IO<A> startIO;

  private final scala.concurrent.ExecutionContext startEC;

  private final cats.effect.unsafe.IORuntime runtime;

  private cats.effect.ByteStack conts;

  private final cats.effect.ArrayStack<java.lang.Object> objectState;

  private scala.concurrent.ExecutionContext currentCtx;

  private cats.effect.ArrayStack<scala.concurrent.ExecutionContext> ctxs;

  private boolean canceled;

  private int masks;

  private boolean finalizing;

  private final int childMask;

...

After:

Compiled from "IOFiber.scala"
public final class cats.effect.IOFiber<A> extends cats.effect.IOFiberPlatform<A> implements cats.effect.kernel.Fiber<cats.effect.IO, java.lang.Throwable, A>, java.lang.Runnable {
  private final int initMask;

  private final cats.effect.unsafe.IORuntime runtime;

  private cats.effect.ByteStack conts;

  private final cats.effect.ArrayStack<java.lang.Object> objectState;

  private scala.concurrent.ExecutionContext currentCtx;

  private cats.effect.ArrayStack<scala.concurrent.ExecutionContext> ctxs;

  private boolean canceled;

  private int masks;

  private boolean finalizing;

  private final int childMask;

...

Then I figured, why not use it for autoCede as well. Here are the results.

Benchmark results (very marginal improvements, but this is to be expected):

Benchmark series/3.x error unit this PR error unit
AsyncBenchmark.async 16944.240 ± 260.687 ops/s 17251.533 ± 232.521 ops/s
AsyncBenchmark.race 21521.173 ± 410.881 ops/s 21687.002 ± 306.729 ops/s
AsyncBenchmark.racePair 20644.814 ± 1180.355 ops/s 21109.900 ± 1268.308 ops/s
AsyncBenchmark.start 5888.046 ± 356.687 ops/s 5943.158 ± 74.453 ops/s
DeepBindBenchmark.async 1313.741 ± 76.735 ops/s 1389.603 ± 33.490 ops/s
DeepBindBenchmark.delay 3128.832 ± 112.297 ops/s 3175.600 ± 131.373 ops/s
DeepBindBenchmark.pure 4164.669 ± 116.216 ops/s 4184.599 ± 112.218 ops/s
ShallowBindBenchmark.async 976.105 ± 6.784 ops/s 979.814 ± 16.856 ops/s
ShallowBindBenchmark.delay 2792.907 ± 78.072 ops/s 2911.944 ± 287.622 ops/s
ShallowBindBenchmark.pure 3115.989 ± 30.956 ops/s 3528.893 ± 102.307 ops/s

Comment on lines -784 to +794
objectState.push(cur.ioa)
resumeIO = cur.ioa
Copy link
Member

Choose a reason for hiding this comment

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

Wait… what? How? Doesn't this break down if you nest multiple evalOns?

Copy link
Member Author

Choose a reason for hiding this comment

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

Each evalOn relinquishes the runloop immediately with execute(ec)(this), so the pushing to the objectState is immediately followed by a pop in evalOnR. It's the exact same logic.

@djspiewak djspiewak merged commit 9b4164d into typelevel:series/3.x Mar 19, 2021
@vasilmkd vasilmkd deleted the resumeio branch March 21, 2021 10:57
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.

None yet

2 participants