Skip to content
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

Idea for solving the writev problem #27

Open
domenic opened this issue Nov 5, 2013 · 9 comments
Open

Idea for solving the writev problem #27

domenic opened this issue Nov 5, 2013 · 9 comments

Comments

@domenic
Copy link
Member

domenic commented Nov 5, 2013

CorkableWritableStream (what a name!) inherits from WritableStream (or from BaseWritableStream? How do I mix them?). It adds public cork and uncork methods, and a constructor option writev. Thus calling cork switches you into batch-write mode where calls to write get batched, and calling uncork switches you out of it.

Not entirely sure about the compositionality of this approach---I feel like I'm trying to solve too many problems with inheritance, perhaps---but in general, it should be a fairly simple modification of a writable stream to make it corkable.

@domenic
Copy link
Member Author

domenic commented Feb 23, 2014

Moving this out of the readme as I'm not sure it's going to make it:

CorkableWritableStream

class CorkableWritableStream extends WritableStream {
    // Adds a writev argument.
    constructor({
        function start = () => {},
        function write = () => {},
        function writev = () => {},
        function close = () => {},
        function abort = close,
        strategy: { function count, function needsMoreData }
    })

    // New methods
    void cork()
    void uncork()

    // Overriden to take into account corking
    Promise<undefined> write(data)
    Promise<undefined> close()

    // TODO: internal properties and overrides to support corking
}

@tyoshino
Copy link
Member

Thinking of real use case, for example, if the sink is network and the WritableStream to the sink accepts ArrayBuffers. We can make the sink accept also an array of ArrayBuffers for batch writing. strategy can be defined to recognize such an array as well.

It might be a problem for the case the data type is also ArrayBuffer for non batch write, but can still do type check or fail on encountering unexpected structure.

You need to build an array by yourself but I don't feel like it's so cumbersome compared to calling cork/uncork.

@domenic
Copy link
Member Author

domenic commented Feb 25, 2014

@tyoshino hmm I think I see what you're saying: something like

var polymorphicStream = new WritableStream({
  write(data, done, error) {
    if (Array.isArray(data)) {
      sink.writev(data, () => { /* use done/error */ });
    } else {
      sink.write(data, () => { /* use done/error */ })
    }
  }
});

?

In general I kind of hate overloading but you're right that it's a much easier and simpler solution, that only uses existing primitives.

Recalling the Node discussion it seems kind of like their option C/D, except we thankfully don't deal with encoding at this level, so that nightmare is gone, and they did writev instead of overloading write.

@tyoshino
Copy link
Member

@domenic Right. I'm not against cork/uncork. Just tried to look for alternative. Have you considered adding one method say control that accepts a string (or structured data?) to send some command to the sink? When I was discussing Aymeric's requests for stop/resume/etc., I thought it might be good to provide this kind of data path whose interpretation is opaque to the streams primitive and is distinguished from the main data stream. cork/uncork could be implemented as commands the sink understands.

var stream = new WritableStream({
  write(data, done, error) {
    ...
  },
  control(opcode, done, error) {
    if (opcode === "cork") {
      ...
    } else ...
  },

IIUC, this is still efficient (not introducing any significant delay), and the only change is how cork-ed data is considered to be consumed. I.e., this sends out the I/O vectors to the sink before uncorking happens, so the WritableStreamWithControl will notify the writer of "the data has been consumed" by resolving the promise even while the set of data is actually still cork-ed inside the sink. If we consider dam-ing cork-ed data as a role of Streams API, this is bad, but it seems not.

@tyoshino
Copy link
Member

Hmm, sorry. This means that we stop utilizing Streams for buffering cork-ed data. That's not desirable basically. I need to think more.

@tyoshino
Copy link
Member

Having corkedWrite might be simpler? N-1 corkedWrite + write result in writev with N items. Kinda variant of A in the thread you pointed.

@domenic
Copy link
Member Author

domenic commented Mar 1, 2014

Have you considered adding one method say control that accepts a string (or structured data?) to send some command to the sink?

This seems like the job for specific subclasses, especially if it's opaque to the primitive... I am pretty sure putting it in the base class would not gain anything.

Having corkedWrite might be simpler? N-1 corkedWrite + write result in writev with N items. Kinda variant of A in the thread you pointed.

I think we have to consider the perspective of the user... corkedWrite + write seems like it would be hard to reason about, since the behavior of write changes. Although, cork + write + uncork has similar problems.

The easiest to reason about is probably just a writev([...array...]) form, but then it's tough to use if you want to queue up multiple writes e.g. in a loop before committing them...

It's probably important to remember that the primary consumer of this is pipeTo. If pipeTo notices the destination has a writev (or cork + uncork, or whatever) then it will drain the source as much as possible before passing the writes as a batch. I am not sure what approach this favors though, haha.

@tyoshino
Copy link
Member

tyoshino commented Mar 1, 2014

I am pretty sure putting it in the base class would not gain anything.

Yeah, it was kinda suggestion to delegate cork stuff to each impl.

The easiest to reason about is probably just a writev([...array...]) form

Agreed but yeah, to reduce the work by user, I tried to figure out something.

@othiym23
Copy link

othiym23 commented Apr 4, 2014

/cc @Gozala

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants