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.
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
lib: performance improvement on readline async iterator #41276
lib: performance improvement on readline async iterator #41276
Changes from 1 commit
c467ffe
05db8ff
3ecee3c
83919d2
03ac26d
870b5cf
ae3325c
bb6f6ab
221e239
b536144
31efb2d
0e04a18
7a8c3fa
156837b
87da1a2
e7f358a
d5917c9
0143cdf
917c51b
4e8be80
610a275
e1dd9d3
26d066b
835bb5f
e83baea
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part wasn't needed because this method is inherited from the parent prototype
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pauseThreshold -> highWaterMark?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, Could it be pauseThreshold -> highWaterMark and resumeThreshold -> lowWaterMark? I understood that I got the best result in my testing controlling the moment to pause and to resume with two different thresholds
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ronag what do you think (about naming)?
(to tl;dr; this PR improves the performance of readline's async iterator and
events.on
by making the implementation faster using a FixedQueue and exposing machinery to deal with backpressure inon
that isn't exposed now but will be in a future PR)(ideally readline would just be a stream but that probably can't work)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since these params won't be exposed to the public can you please use symbols for them? e.g.
And then pass that instead:
Since these aren't exposed to users?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can do that.
But maybe it's a good thing to expose it to users, it can be useful in other situations too.
Regardless, it can be discussed in a further moment, so I'll change it to a internal symbol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@benjamingr the problem here is that I'm using the firstEventParam on the interface.js, so, even if I create an internal symbol into events.js, I would need to export it. Is there some special place you folks use to put internal symbols shared between files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Farenheith the thing is we are touching a public API here (on) without a lot of feedback and since you've already been working on this PR for a while I don't want to endanger delaying it further so I would rather not expose any public API changes to
on
here and do it in a follow up PR.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@benjamingr okay, I created a new file to keep this symbol inside the internal folder. I think it's enough to attend that.
Please take a look if it solves the issue and if there's anything else to fix, just let me know!