-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
Proposal: Move isaacs/readable-stream to iojs and make it the authoratative source #55
Comments
+1 I'm hugely in favor of this. |
@feross be warned, you'd be on my list of people to pull in to a maintainer team if this goes ahead |
+1 |
+1. @rvagg do you need me to do the switch? |
+1 |
@isaacs at this stage I'm more looking for agreement from TC to reverse the flow of code so that all the work is done in readable-stream rather than core, but there's no harm in moving readable-stream over to the org in the meantime. |
Oh, I see, so the plan would be to have the module be the "upstream" and then iojs consumes it? If so, that makes a lot of sense, and moving it to the org also seems sensible. I think this should be the exception rather than the norm, however. Stuff like |
👍 |
1 similar comment
+1 |
Finally, +1 |
+1 happy to help |
+1 as long as I don't have to triage bugs. :-) |
I'm a little worried about making readable-stream (the package) the authoritative source for streams in Node/iojs -- though at the moment I'm having difficulty putting the "why" down into words. I'll revisit this later today and see if I can't explain better, in the meantime I'm "-0" for that change (it could just turn out that I'm being silly about this, though.) I'm totally +1 putting the package under the iojs org, though! |
Okay, here goes: Issues with and PRs against streams will be opened against this repository. The latter can be solved with a big ole' disclaimer comment at the top of the vendored file, but the former splits issues between "those who know where
As one of node's core primitives, I'm not sure loosening or breaking streams the integration of streams with core is feasible; even if it is feasible, I am not sure that it would be a good thing. Streams are one of the few things that packages must agree on. Anecdotally, js-git is a good example of what a world without standard stream primitives would look like -- where the stream primitive is a local agreement between cooperating packages. In order to have my streams1-based git packages work with js-git, I rewrote them into min-streams and added translation modules. The min-streams only depended on other min-streams), and exposed streams1 to other consumers on the side. This took a lot of time, and while it made the module usable from min-streams or streams1, it still wouldn't necessarily work with pull-streams, or RxJS; and it only worked in a single package because I had a verbal agreement with the consumers (all two of us) of these packages -- I knew that the min-stream version would be available at One of the really lovely things about streams as a primitive is that modules are casually pluggable. There's not so much specification as to get in the way of the problems being solved, or so little specification that more time is spent providing shim modules at edges between ecosystems, but just enough agreement to foster cooperation and enable the community to do some truly cool things. My experience integrating streams1 with min-streams for js-git led me to believe that local, ecosystem-based agreements for the stream primitive are far less effective than global agreement. Min-streams aren't bad -- they're really neat, and for the constraints js-git works within they make total sense -- but both streams1 and min-streams lost a lot of effectiveness by not being the globally agreed-upon standard interface. (And, given all that, it follows that I believe that the readable-stream module and this repo would have to be upgraded in lockstep for most changes to readable-stream, which would create more work, and thus more potential for human error than currently exists.)
Oof, I definitely agree with this. I have a tiny stack of hand drawn state machine diagrams with a lot of frustrated scribbles in the margins that I consult regularly. We need to decrease the bus factor, and forming an impromptu team of people that can field issues with streams is a great idea. I'm a little concerned about having the option to "solely" work in streams. Having streams in core provides JS developers who are aspiring contributors a nice "ladder" to climb up into different subsystems (and C++). The design of streams, on the other hand, benefits from informed use in core -- and I think there are still gains to be made there (uniting resource/handle-backed streams under a common subclass, or otherwise addressing pain points with the existing integrations). Also of note: stream documentation -- due to the complexity of streams, they hit a lot of pain points with the way documentation is currently done in Node (is it a guide? is it reference material? who is the audience?) Moving the symptom out of Node proper makes it less likely that the underlying problem will get addressed satisfactorily -- when docs are revisited, they should be revisited with the pain of streams docs fresh in mind, and with solutions in hand. Sorry for the unstructured stream of concerns. To recap, in numbered list form:
|
This is my main concern with using
I'm willing to bet that everyone here has spent countless hours digging through that code, searching for obscure bugs and trying to fix them. Can we please, please simplify? I'm more than happy to help with the effort. I wrote BufferedStream in the days of node 0.4 to normalize behavior of the different "streams" that existed at that time, and I'm familiar with the complexity of this particular problem. |
This sounds like something obvious but it turns out not to be true. Streams has to straddle old-style and new-style support in one object and be as performant as possible. It is very common in Node that code is obfuscated in crazy looking ways for performance reasons. Simplicity and clarity is certainly the top priority for API design but I'm afraid it falls below performance and compatibility in the actual implementation. |
I don't see any evidence to suggest that performance must suffer for the sake of clarity and brevity. For some reason node has always assumed that the inverse is true though, and it's an opinion that has severely hampered its usability. You can have good performance and a clean implementation. |
@mjackson there is a long history of patches from @trevnorris that are designed to hit v8 performance paths and are much less readable than they would be if performance was not a consideration. |
I'm +1 on simplifying streams where possible, but it might be best to open up that point up as a separate issue so the conversations don't get intermingled too much.
Compatibility is a big reason things streams are complex, but performance is not a prerequisite contributor to their complexity. At a micro level, there's all sorts of deoptimizations in streams; at a slightly larger level, some of the backing data structures can be swapped out to improve performance for common cases, like logging. In any case I'm getting ahead of myself -- it would be good to discuss this in a separate issue where we can discuss concrete pain points with streams at present before putting out solutions. |
I should have been clearer. I think there is a lot of room for streams to get faster, and that the work that goes in to making them faster may not make them less complex. |
Original commit message: Merged: [wasm-simd] Fix loading fp pair registers We were incorrectly clearing the high reg from the list of regs to load. The intention was to prevent double (and incorrect) loading - loading 128 bits from the low fp and the loading 128 bits from the high fp. But this violates the assumption that the two regs in a pair would be set or unset at the same time. The fix here is to introduce a new enum for register loads, a nop, which does nothing. The high fp of the fp pair will be tied to this nop, so as we iterate down the reglist, we load 128 bits using the low fp, then don't load anything for the high fp. Bug: chromium:1161654 (cherry picked from commit 8c698702ced0de085aa91370d8cb44deab3fcf54) (cherry picked from commit ffd6ff5a61b9343ccc62e6c03b71a33682c6084d) Change-Id: Ib8134574b24f74f24ca9efd34b3444173296d8f1 No-Try: true No-Presubmit: true No-Tree-Checks: true Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2619416 Commit-Queue: Zhi An Ng <[email protected]> Reviewed-by: Clemens Backes <[email protected]> Cr-Original-Commit-Position: refs/branch-heads/8.8@{nodejs#28} Cr-Original-Branched-From: 2dbcdc105b963ee2501c82139eef7e0603977ff0-refs/heads/8.8.278@{#1} Cr-Original-Branched-From: 366d30c99049b3f1c673f8a93deb9f879d0fa9f0-refs/heads/master@{#71094} Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2649176 Reviewed-by: Victor-Gabriel Savu <[email protected]> Commit-Queue: Achuith Bhandarkar <[email protected]> Cr-Commit-Position: refs/branch-heads/8.6@{nodejs#55} Cr-Branched-From: a64aed2333abf49e494d2a5ce24bbd14fff19f60-refs/heads/8.6.395@{#1} Cr-Branched-From: a626bc036236c9bf92ac7b87dc40c9e538b087e3-refs/heads/master@{#69472} Refs: v8/v8@482e5c7
Original commit message: Merged: [wasm-simd] Fix loading fp pair registers We were incorrectly clearing the high reg from the list of regs to load. The intention was to prevent double (and incorrect) loading - loading 128 bits from the low fp and the loading 128 bits from the high fp. But this violates the assumption that the two regs in a pair would be set or unset at the same time. The fix here is to introduce a new enum for register loads, a nop, which does nothing. The high fp of the fp pair will be tied to this nop, so as we iterate down the reglist, we load 128 bits using the low fp, then don't load anything for the high fp. Bug: chromium:1161654 (cherry picked from commit 8c698702ced0de085aa91370d8cb44deab3fcf54) (cherry picked from commit ffd6ff5a61b9343ccc62e6c03b71a33682c6084d) Change-Id: Ib8134574b24f74f24ca9efd34b3444173296d8f1 No-Try: true No-Presubmit: true No-Tree-Checks: true Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2619416 Commit-Queue: Zhi An Ng <[email protected]> Reviewed-by: Clemens Backes <[email protected]> Cr-Original-Commit-Position: refs/branch-heads/8.8@{nodejs#28} Cr-Original-Branched-From: 2dbcdc105b963ee2501c82139eef7e0603977ff0-refs/heads/8.8.278@{#1} Cr-Original-Branched-From: 366d30c99049b3f1c673f8a93deb9f879d0fa9f0-refs/heads/master@{#71094} Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2649176 Reviewed-by: Victor-Gabriel Savu <[email protected]> Commit-Queue: Achuith Bhandarkar <[email protected]> Cr-Commit-Position: refs/branch-heads/8.6@{nodejs#55} Cr-Branched-From: a64aed2333abf49e494d2a5ce24bbd14fff19f60-refs/heads/8.6.395@{#1} Cr-Branched-From: a626bc036236c9bf92ac7b87dc40c9e538b087e3-refs/heads/master@{#69472} Refs: v8/v8@482e5c7
Original commit message: Merged: [wasm-simd] Fix loading fp pair registers We were incorrectly clearing the high reg from the list of regs to load. The intention was to prevent double (and incorrect) loading - loading 128 bits from the low fp and the loading 128 bits from the high fp. But this violates the assumption that the two regs in a pair would be set or unset at the same time. The fix here is to introduce a new enum for register loads, a nop, which does nothing. The high fp of the fp pair will be tied to this nop, so as we iterate down the reglist, we load 128 bits using the low fp, then don't load anything for the high fp. Bug: chromium:1161654 (cherry picked from commit 8c698702ced0de085aa91370d8cb44deab3fcf54) (cherry picked from commit ffd6ff5a61b9343ccc62e6c03b71a33682c6084d) Change-Id: Ib8134574b24f74f24ca9efd34b3444173296d8f1 No-Try: true No-Presubmit: true No-Tree-Checks: true Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2619416 Commit-Queue: Zhi An Ng <[email protected]> Reviewed-by: Clemens Backes <[email protected]> Cr-Original-Commit-Position: refs/branch-heads/8.8@{#28} Cr-Original-Branched-From: 2dbcdc105b963ee2501c82139eef7e0603977ff0-refs/heads/8.8.278@{#1} Cr-Original-Branched-From: 366d30c99049b3f1c673f8a93deb9f879d0fa9f0-refs/heads/master@{#71094} Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2649176 Reviewed-by: Victor-Gabriel Savu <[email protected]> Commit-Queue: Achuith Bhandarkar <[email protected]> Cr-Commit-Position: refs/branch-heads/8.6@{#55} Cr-Branched-From: a64aed2333abf49e494d2a5ce24bbd14fff19f60-refs/heads/8.6.395@{#1} Cr-Branched-From: a626bc036236c9bf92ac7b87dc40c9e538b087e3-refs/heads/master@{#69472} Refs: v8/v8@482e5c7 PR-URL: #38275 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Shelley Vohr <[email protected]>
#53 and all of the other obscure streams issues that keep coming up keep on giving me headaches. Even though I "maintain" readable-stream as an extraction of joyent/node streams I still don't have a complete handle on the inner workings of the streams code and I imagine this is the same for many core contributors. I'm willing to bet that even now, nobody understands the code better than @isaacs because it's grown so complex.
As the most obvious source of JavaScript complexity in core, I propose that we move readable-stream to iojs and make it the place to file streams issues and where the code in core is extraced from rather than the other way around. It can also have its own core team, separate but potentially overlapping with iojs TC, that cares about and understands how they work and maybe the tight integration with core can be loosened (and perhaps broken completely) over time. As a separate project we can also pursue better testing (including browserify) of the code.
The text was updated successfully, but these errors were encountered: