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

[chrisdickinson]

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

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 start method 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]

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(), or crypto.createCipherStream().

We should open a separate issue for IE8 support considerations.

[calvinmetcalf]

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:

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(), or crypto.createCipherStream().

We should open a separate issue for IE8 support considerations.

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

[mcollina]

I'm closing this as there never was an agreement on what to be done.