crypto streams

[calvinmetcalf]

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'?

[chrisdickinson]

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:

• subclasses directly manipulate private state, or...
• state is being preserved in the base classes for the exclusive use of a subclass.

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]

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]

@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]

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:

@chrisdickinson https://github.com/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.

—
Reply to this email directly or view it on GitHub
#109 (comment)
.

[calvinmetcalf]

threw together an quick idea of how just making them lazy by default might look calvinmetcalf/io.js@bf06dd0

[calvinmetcalf]

due to how duplex streams inherit from both readable and writable you can't really define _writable state on the prototype

[indutny]

@calvinmetcalf and performance? :)

[calvinmetcalf]

benching it as we speak

On Fri Jan 30 2015 at 9:30:27 AM Fedor Indutny notifications@github.com
wrote:

@calvinmetcalf https://github.com/calvinmetcalf and performance? :)

—
Reply to this email directly or view it on GitHub
#109 (comment)
.

[calvinmetcalf]

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]

ok looks like fixing Writable gives it a more reasonable perf, I'm going to open up a pull to better discuss.

[chrisdickinson]

Using Object.defineProperty might prevent that functionality from being implemented in browserify for legacy browsers.

[domenic]

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]

there already is one here