Skip to content

Cache suspensions#37

Merged
kelunik merged 6 commits into
mainfrom
cache-suspensions
Jan 14, 2022
Merged

Cache suspensions#37
kelunik merged 6 commits into
mainfrom
cache-suspensions

Conversation

@kelunik

@kelunik kelunik commented Jan 10, 2022

Copy link
Copy Markdown
Member

Not sure whether this is useful. If we go that route, we should rename createSuspension into getSuspension.

@trowski trowski left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

createSuspension I think is a fine name. Decorating drivers may still return a new object implementing Suspension to implement some fiber-locals, etc.

@kelunik

kelunik commented Jan 10, 2022

Copy link
Copy Markdown
Member Author

create sounds like it always returns a new object, which it doesn't do anymore then.

@trowski

trowski commented Jan 11, 2022

Copy link
Copy Markdown
Member

create sounds like it always returns a new object, which it doesn't do anymore then.

Yes, I suppose you're right. My initial thinking was create would better indicate the method should be called rather than reusing a prior Suspension object by the consumer (which may have been associated with a different fiber). However, create certainly doesn't make this obvious and will need to be documented anyway. get likely does make more sense then. I'm fine if you want to change that now as part of this PR.

@azjezz

azjezz commented Jan 11, 2022

Copy link
Copy Markdown
Member

getX also gives the assumption that it always returns the same suspension.

DriverInterface::suspension(): SuspensionInterface - return, or create suspension for the current fiber

@kelunik

kelunik commented Jan 11, 2022

Copy link
Copy Markdown
Member Author

It does return the same instance, per fiber. It's similar to a ThreadLocal in Java.

@azjezz

azjezz commented Jan 11, 2022

Copy link
Copy Markdown
Member

yea, but I'm talking generally ( as between different fibers ), maybe DriverInterface::currentSuspension()?

@kelunik

kelunik commented Jan 11, 2022

Copy link
Copy Markdown
Member Author

@trowski @WyriHaximus Renamed and added a documentation comment that the driver must always return the same instance for each fiber.

@trowski

trowski commented Jan 11, 2022

Copy link
Copy Markdown
Member

+1 for keeping the old method and deprecating it during 0.2.x.

@kelunik

kelunik commented Jan 11, 2022

Copy link
Copy Markdown
Member Author

I've added EventLoop::createSuspension again to allow for a smoother transition from 0.1 to 0.2, so libraries can mostly require ^0.1 || ^0.2 instead of directly jumping to ^0.2.

Comment thread src/EventLoop.php Outdated
Comment thread composer.json Outdated
* main:
  Also avoid re-creating callback fiber for throwing microtasks
  Avoid re-creating callback fiber on exceptions
  Remove fiber switches on callback fiber creation
@kelunik kelunik merged commit ab79812 into main Jan 14, 2022
@kelunik kelunik deleted the cache-suspensions branch January 14, 2022 11:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants