Conversation
When constructing a Fiber in support of Enumerator#next, we know it will be immediately resumed, so we can feed that initial resume value directly into the first execution of the virtual thread, avoiding a potentially costly and unnecessary vthread suspension at create time. This avoids such create-and-resume cases triggering the initial allocation of vthread state (StackChunk etc) based on a much shallower stack than they will actually need, and a subsequent throwing out of that initial state. This patch was created with assistance from an agent and has a few unresolved issues: * Repeatedly checking for null initialBlock and initialRequest even for cases that are not using lazy initialization (such as typical Fiber.new usage). This could be eliminated by abstracting the first exchange into a function object, replaced with the simple version on first resume, but my first attempt to clean it up got messy. * The method is private and must be called with #send, but it remains potentially visible to user code; a cleaner implementation would move this internal method to an internal location that will only be called from internal code. * This only improves the performance of an initial Enumerator#next, which would be dwarfed by subsequent #next calls whenever there's more than one. This is a severe edge case which has many other strikes against it, so the extra complexity here may not be worth speeding up this rare and discouraged case. The positive improvements, for reference: * Because the new vthread does not suspend until it actually yields its first result, the StackChunk allocated at that point may be exactly as large as it needs to be (assuming that yield lives at the same level as later yields, which is the case for the Fiber created by Enumerator#next). * It proves a fused create-and-resume case can perform better than creating and resuming a Fiber separately to get a single object. This may be an interesting case combined with fiber schedulers, since that scenario may have many single-shot fibers expecting to be called only once (and presumably created to encapsulate a single blocking call). Further cleanup and experimentation are needed to move forward with this change. This relates to ruby/csv#361 and improves the performance of the Enumerator#next benchmark there by roughly 30-40%.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When constructing a Fiber in support of Enumerator#next, we know it will be immediately resumed, so we can feed that initial resume value directly into the first execution of the virtual thread, avoiding a potentially costly and unnecessary vthread suspension at create time. This avoids such create-and-resume cases triggering the initial allocation of vthread state (StackChunk etc) based on a much shallower stack than they will actually need, and a subsequent throwing out of that initial state.
This patch was created with assistance from an agent and has a few unresolved issues:
The positive improvements, for reference:
Further cleanup and experimentation are needed to move forward with this change.
This relates to ruby/csv#361 and improves the performance of the Enumerator#next benchmark there by roughly 30-40%.