Repository navigation
crypto streams #109
Description
there is an effort to remove use of _writableState and _readableState nodejs/node#445 the crypto streams are a bit of an interesting case as they are lazy and use _writableState and _readableState to defer stream initialization. Are there a set of public api methods we can substitute for '_readableState', '_writableState','_transformState'?
Activity
chrisdickinson commentedon Jan 30, 2015
To amend the intent of that issue a bit – the goal is not to absolutely remove all use of
_{{writ,read}able,transform}State, but to remove all places where:Ultimately, the places where the subclasses access those state objects should be catalogued and we should look into the best way to deal with that access – whether that's promotion into a public API or a protected API for subclasses.
That said, I'd like to get the backstory on why the crypto streams lazily initialize the way they do before we make changes there. Maybe @bnoordhuis or @indutny could share the history of that API a bit?
bnoordhuis commentedon Jan 30, 2015
I believe that was done to fix a performance regression in v0.10. (Maybe "fix" is not the right word; IIRC, it went from 8x to 1-2x slower than v0.8.)
The commit is 855caa8. The commit log is a little light on details but I think it's called out in the v0.10.1 changelog.
indutny commentedon Jan 30, 2015
@chrisdickinson it was too expensive to operate on a streams level. Thus we opted-out of it by default for
crypto.createHash().update().digest()which does not really require any of this machinery.calvinmetcalf commentedon Jan 30, 2015
This would be pretty helpful in user land streams as well (if no where else
then crypto browserify) so having a way to do this through the public api
would be very nice.
We could make this internal to streams, a lazy option to initiation which
defers actual initiation until something actually uses the stream aspects.
This would create some flexibility too so things like sign/verify could be
lazy write streams.
On Fri, Jan 30, 2015, 6:14 AM Fedor Indutny notifications@github.com
wrote:
calvinmetcalf commentedon Jan 30, 2015
threw together an quick idea of how just making them lazy by default might look calvinmetcalf/io.js@bf06dd0
calvinmetcalf commentedon Jan 30, 2015
due to how duplex streams inherit from both readable and writable you can't really define _writable state on the prototype
indutny commentedon Jan 30, 2015
@calvinmetcalf and performance? :)
calvinmetcalf commentedon Jan 30, 2015
benching it as we speak
On Fri Jan 30 2015 at 9:30:27 AM Fedor Indutny notifications@github.com
wrote:
calvinmetcalf commentedon Jan 30, 2015
https://gist.github.com/calvinmetcalf/06760d8fd7a530533db4
seems slower overall but there is a lot of noise and certain things are faster. Can probably make write streams work better.
calvinmetcalf commentedon Jan 30, 2015
ok looks like fixing Writable gives it a more reasonable perf, I'm going to open up a pull to better discuss.
chrisdickinson commentedon Jan 30, 2015
Using
Object.definePropertymight prevent that functionality from being implemented in browserify for legacy browsers.domenic commentedon Jan 30, 2015
OK, at some point we need to have a serious discussion as to whether readable-stream and the streams WG is actually considering IE8 support within its charter.
calvinmetcalf commentedon Jan 30, 2015
there already is one here
chrisdickinson commentedon Jan 30, 2015
@calvinmetcalf I know, I put it there (in joyent/node streams) before it occurred to me that browserify might have issues with it.
@domenic Yes, I'll add that issue to wg-agenda for next time. It'd be good to get @substack to weigh in as well.
sonewman commentedon Feb 24, 2015
I'm on the fence with this one. There are good reasons to make the internal state access lazy it would be useful, particularly if we introduced a
startmethod for readable and if it doesn't introduce any perf issues.However I am with @domenic on flagging browser support, so we should discuss @iojs/streams and decide what our status on browser support is since it would effect Browserify and Webpack users supporting IE8.
chrisdickinson commentedon Feb 24, 2015
I'm -1 on making these properties lazily transform objects into full streams. The best reason we have is that one API wanted to bake-in streams support after the fact without incurring a perf hit for non-streams users. Our goal should be to drive down the overhead of stream support, while also encouraging folks to build stream-specific APIs – e.g.,
crypto.createHashStream(), orcrypto.createCipherStream().We should open a separate issue for IE8 support considerations.
calvinmetcalf commentedon Feb 24, 2015
I guess the key is whether there is any inclination to do that in crypto
On Tue, Feb 24, 2015, 5:16 PM Chris Dickinson notifications@github.com
wrote:
mcollina commentedon May 19, 2017
I'm closing this as there never was an agreement on what to be done.