implement helper function for blocking reads

RESOLVED WONTFIX

Status

()

enhancement
P1
normal
RESOLVED WONTFIX
20 years ago
3 years ago

People

(Reporter: waterson, Assigned: alecf)

Tracking

(Blocks 1 bug, {perf})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Here's a bugper our discussion today. Implement a helper function that opens a
URL and a stream listener, reads the contents of the stream, and then returns
once the read is complete.

Basically, take the functionality of rdf_BlockingParse() and move it into a
necko utility.
QA Contact: paulmac → waterson
Set self as QA contact.
Status: NEW → ASSIGNED
Target Milestone: M10
This may or may not relate to the CopyFrom stuff dougt is doing. Cc'ing him.
Spoke with warren at bug triage today.  Not an M10 blocker...moving to M11.
Target Milestone: M11 → M12
Not essential for m11.
Target Milestone: M12 → M15
I'm moving this to m15. If we get to it, we get to it, but it doesn't seem like
a big deal.
Bulk move of all Necko (to be deleted component) bugs to new Networking

component.
Severity: normal → enhancement
Target Milestone: M15 → M20
Target Milestone: M20 → Future
I found 4 places tonight where the same silly helper class was cut&paste in the 
code:

nsXULKeyListener
nsXULDocument
nsRDFXMLDataSource
nsXBLService

You guys should know better than that.
I discovered that we can save some space as well as time here too by 
eliminating this ProxyStream class in favor of an nsIInputStreamObserver that 
notifies BlockingParse when more data is available. It will save us a copy, and 
the copy buffer. Dependent on fixing bug 46777.
Depends on: 46777
I think we should do this now, since I know how to do it and save space and 
time.
Target Milestone: Future → M18
assigning to myself... let me know if this bug is still relavent.  thx!

setting target = future
Assignee: warren → darin
Status: ASSIGNED → NEW
Target Milestone: M18 → Future
waterson: there is a version of NS_OpenURI in nsNetUtil.h that takes a nsIURI
and a nsIStreamListener.  would this not suffice?  is asyncOpen inappropriate
for this?
In the places where this is used, we actually need to *synchronously* read the 
files in (for example, on startup). Is that the case with the functions you're 
referring to?
if you are only reading synchronously, then i am confused as to why a stream
listener is involved?  is that you "have time" to block your thread to read from
the stream, but that you still need to pass the data to an asynchronous-type
listener?  if so, then NS_OpenURI wouldn't be the same.
In the case of RDF, it ``supports'' (*cough*) either blocking or non-blocking
reads. So yeah, I have an async interface that I need to pump synchronously
sometimes.
QA Contact: waterson → benc
Summary: [rfe] implement helper function for blocking reads → implement helper function for blocking reads
Status: NEW → ASSIGNED
Brain dump from darin (darin, feel free to correct)

Use the stream transfer service, and create an input transport for the stream.
Using that, create a input stream. this input stream will be a pipe which brings
the data over from the thread that's actually loading the data.

We can't just loop over calls to Available()/OnDataAvailable(), because we can't
call Available() on the pipe. Instead we should call ReadSegments() on the pipe.
In the callback, we create a nsStringInputStream for each chunk, and notify the
stream listener about data being available on that buffer.

My one thought is that is seems like now the consumer (the nsIStreamListener)
will need to call ReadSegments() in order to avoid the extra read out of the
nsStringInputStream, if it wants to avoid the copy out of the pipe. Darin,
thoughts on that?

I've started an initial implementation of the loader service. The one minor
oddity I've run into is that we still have to pass in the nsIChannel for the
stream that's being loaded, so that we have something to pass into
OnDataAvailable() and friends. I guess that isn't a huge deal, but it assumes
there is an nsIChannel available... I guess that's fine for existing
implementations who are already using nsIStreamListener, because they are
getting a channel in anyway.. but I wonder if there are other potential users of
this who have an nsIInputStream created outside of necko.
> Use the stream transfer service, and create an input transport for the stream.
> Using that, create a input stream. this input stream will be a pipe which 
> brings the data over from the thread that's actually loading the data.

you'll want to open a blocking, buffered input stream on the nsITransport 
returned from nsIStreamTransportService::CreateInputTransport.

  transport->OpenInputStream(nsITransport::OPEN_BLOCKING, ...);

> We can't just loop over calls to Available()/OnDataAvailable(), because we 
> can't call Available() on the pipe. Instead we should call ReadSegments() on 
> the pipe. In the callback, we create a nsStringInputStream for each chunk, and 
> notify the stream listener about data being available on that buffer.

we can allocate the nsStringInputStream once and call the ShareData method to
configure the stream before calling OnDataAvailable each time.

> My one thought is that is seems like now the consumer (the nsIStreamListener)
> will need to call ReadSegments() in order to avoid the extra read out of the
> nsStringInputStream, if it wants to avoid the copy out of the pipe. Darin,
> thoughts on that?

Yes, the consumer should call ReadSegments.

> I've started an initial implementation of the loader service. The one minor
> oddity I've run into is that we still have to pass in the nsIChannel for the
> stream that's being loaded, so that we have something to pass into
> OnDataAvailable() and friends. I guess that isn't a huge deal, but it assumes
> there is an nsIChannel available... I guess that's fine for existing
> implementations who are already using nsIStreamListener, because they are
> getting a channel in anyway.. but I wonder if there are other potential users 
> of this who have an nsIInputStream created outside of necko.

don't think in terms of nsIChannel... think in terms of nsIRequest.  the API
for OnDataAvailable just passes a nsIRequest.  the API for this helper object
should allow the user to configure the nsIRequest and nsISupports for the
OnDataAvailable calls ;-)

darin
ok, I've got a working implementation, over to me. I think I found a tiny bug in
nsPipe3.cpp, and I needed to tweak nsStringStream in order to make it reusable.
Assignee: darin → alecf
Status: ASSIGNED → NEW
nsStringStream: this is easy - mOffset wasn't being reset when shareData() was
called the 2nd time. This meant that, for instance, if shareData() was first
filled with 4096 bytes, mOffset would be 4096 - when we reused shareData() with
another 999 bytes (for example), then the stream would tell the listener that
it was at offset 4096 bytes even though there were only 999 bytes in the
stream. In the case of the parser, it would try to allocate (length - offset)
which would be 999 - 4096. -3097, which would get translated to some massive
unsigned number making nsMemory::Alloc() fail.

nsPipe3.cpp: The problem I ran into is that when you have a blocking pipe, it
only blocks in ReadSegments when reading the currently available buffers... we
were returning NS_OK even when we hadn't completely filled out the buffers. 

Unfortunately, at some level this seems like the right behavior. In this case
however, we have this stack: (I'm doing this from memory, so bear with me)

nsStringInputStream::ReadSegments()
nsParser::OnDataAvailable()
nsSyncStreamLoader::WritePipeSegments()
nsPipeInputStream::ReadSegments()
nsSyncStreamLoader::PushStreamToListener()

The problem is that when there isn't any more data in the pipe,
nsPipeInputStream::ReadSegments returns early and thus does not completely read
the whole stream, then nsParser::OnDataAvailable() has no way to tell its
caller that it did not completely consume the data... 

So the simple fix was this tweak to nsPipe3.cpp, but I'm wondering if I should
instead be fixing the parser to keep calling ReadSegments() until all the data
is consumed. Help!
here's the implementation... look in the callback, I've put the comments 'How
do we know that everything was consumed?' where I'm confused.
actually, it wouldn't be the parser that would need to be tweaked, the problem
is in the call to 
bufferedStream->ReadSegments(WritePipeSegment, &closure, -1, &bytesRead)

since we don't know how many bytes are actually available, we don't know if the
number of bytes read out of the pipe is all of them or not. (hence the -1)  We
can't call Available() on the pipe because the pipe may or may not know how many
bytes will eventually be available. 

So the problem is basically that I need this:

while (someCondition) {
  bufferedStream->ReadSegments(...);
}

but I don't know how to know "someCondition"


note that my hacked nsPipe3 seems to be working quite well. With that, I've
hooked the loader up to .rdf parsing, among other things, and it really makes
life easy.
Status: NEW → ASSIGNED
Keywords: perf
Priority: P3 → P1
Target Milestone: Future → mozilla1.4alpha
alec: the pipe changes aren't necessary if you sit in a loop waiting for
ReadSegments to return NS_OK and zero bytes read.  that is the EOF indicator.

like this:

  nsresult rv;
  PRUint32 n;
  do {
    rv = stream->ReadSegments(myWriterFunc, this, PRUint32(-1), &n);
  }
  while (NS_SUCCEEDED(rv) && n > 0);
also, "stream loader" is probably the wrong term for this.  we already have
something in necko called a "stream loader" ... there's the nsIStreamLoader and
the nsIUnicharStreamLoader.  these take a channel and give the user a buffer.

perhaps something like nsISyncInputStreamPump instead??  not great either :(
cool! I'll try that.
 
I'm totally flexible on the name - so nsISyncInputStreamPump works.. let me know
if you have any other names you'd prefer.
oh the pump is stateless too, and I'm using it as a service because of that.

Would there be a use for this within necko? If so, I can make the actual methods
be non-virtual so other consumers can call it directly (rather than going
through the service) and then just use the interface to wrap it... in fact yeah
I'll do that anyway.

no consumers for this within necko.  also, given that this is a service,
probably good to give it a name ending with "Service" ;-)

nsSyncLoaderService.. hey, now that sounds familiar!  let me ponder the name for
a bit :)
Posted patch NS_NewBlockingInputStream (obsolete) — Splinter Review
Ok, darin and I talked about this a bit more.. this is now split into two
things, and here's the first part. 

Here's the split:
1) provide a mechanism similar to nsBufferedInputStream that creates an
intermediary pipe and starts the other input stream on a seperate thread. This
allows you to simply take an nsIInputStream and run it on a seperate thread, no
nsIRequest/etc needed.
2) provide another mechanism to pump an nsIInputStream into a nsIStreamListener
until it is empty. 

Between these two mechanisms, we'll have what we need..
Attachment #113111 - Attachment is obsolete: true
Attachment #113118 - Attachment is obsolete: true
Blocks: 113529
Blocks: 190730
Comment on attachment 113226 [details] [diff] [review]
NS_NewBlockingInputStream

>+NS_NewBlockingInputStream(nsIInputStream** aResult,
>+                          nsIInputStream* aStream,
>+                          PRUint32 aSegmentSize = nsIUnicharStreamLoader::DEFAULT_SEGMENT_SIZE)

ok, so aStream had better be a "blocking" input stream already per
the contract of nsIStreamTransportService::CreateInputTransport, so
the name of this helper function isn't quite right.  was a nice stab
at a name though... at least it looks nice ;-)

also, this function should allow the caller to pass aSegmentSize and
aSegmentCount.	the default values should be 0 and 0.  necko treats
0 as a flag to say "use the default buffer parameters."

otherwise, this is right on!
thanks - so any suggestions as to the name? NS_NewBackgroundInputStream?
NS_NewTransportInputStream? I'm pretty flexible...

maybe use "Background" more as an action than an adjective??

NS_BackgroundInputStream(nsIInputStream **result,
                         nsIInputStream *source,
                         PRUint32 segmentSize = 0,
                         PRUint32 segmentCount = 0);

NS_BackgroundOutputStream(nsIOutputStream **result,
                          nsIOutputStream *sink,
                          PRUint32 segmentSize = 0,
                          PRUint32 segmentCount = 0);

IOW these functions take a stream and operate on it in the background ;-)

Be sure to include big scary comments stating that the stream being backgrounded
 must be threadsafe!  in fact, the implementation of the stream should not count
on Read being called on any particular thread, but Read will never be called
re-entrantly.
ok, here's a patch for NS_BackgroundInputStream and one for OutputStreams as
well.
Attachment #113226 - Attachment is obsolete: true
now the other thing I've forgotten was where we would put the
nsIInputStream->nsIStreamListener pump? an inline version would work, but I
think it would be large. Is there an existing service that I can hang this off of?
alec: before implementing that / decided on where this should live, can you see
if this approach is even worthwhile? ;-)
Comment on attachment 113440 [details] [diff] [review]
NS_Background[Input|Output]Stream

well at the very least I'd like to get these functions into the tree so we can
experiment with them.
Doug/Darin, can I get a review?

Note that I forgot the "= 0" in NS_BackgroundOutputStream in the patch, but
have the = 0 in my tree now.
Attachment #113440 - Flags: superreview?(darin)
Attachment #113440 - Flags: review?(dougt)
Attachment #113440 - Flags: review?(dougt) → review+
Comment on attachment 113440 [details] [diff] [review]
NS_Background[Input|Output]Stream

sr=darin
Attachment #113440 - Flags: superreview?(darin) → superreview+
bumping these to 1.5alpha, I just don't have time in 1.4beta...
Target Milestone: mozilla1.4alpha → mozilla1.5alpha
This bug has a patch with r+/sr+. Is it going to make it for 1.5alpha?
mc: hmm, the patch landed a while back actually... alec, you still need this bug
open for something?
(In reply to comment #26)
> 2) provide another mechanism to pump an nsIInputStream into a nsIStreamListener
> until it is empty. 

It looks like the part 2 never happened. Alec? NS_Background*Stream looks unused right now.
You're asking me 4 and a half years later? I have no idea, sorry :)
Target Milestone: mozilla1.5alpha → ---
Version: Trunk → unspecified
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.