Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10244001:
Creation of ByteStream class. (Closed)
|
Created:
8 years, 8 months ago by Randy Smith (Not in Mondays) Modified:
8 years, 7 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, rdsmith+dwatch_chromium.org, satorux1, zel Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionCreation of ByteStream class.
ByteStream is an abstraction of a zero copy transfer of bytes between
threads, along with an error indicator upon completion. Data is
explicitly pushed into or pulled out of the stream, and source and
destination may register for callbacks to be called when there is
room/data in the pipe. A ByteStream object is owned in common by the
source and destination of the stream, and is destroyed when both
source and destination drop references to it.
BUG=125250
R=willchan@chromium.org
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=138504
Patch Set 1 #Patch Set 2 : Cleaned up header comments. #
Total comments: 2
Patch Set 3 : #Patch Set 4 : Rewrite using message passing and not exposing RefCount. #Patch Set 5 : Added stats, tweaked names, and made buffer owned in transfer. #Patch Set 6 : Finished and polished rewrite. #
Total comments: 7
Patch Set 7 : Incorporated (some) comments. #
Total comments: 4
Patch Set 8 : Incorporated comments and fixed a few callback problems. #
Total comments: 48
Patch Set 9 : Incorporated comments. #
Total comments: 6
Patch Set 10 : Incorporated comments. #Patch Set 11 : Fixed bug with zero length writes. #
Total comments: 6
Patch Set 12 : Incorporated non-template suggestions. #Patch Set 13 : Rebased to LKGR. #
Messages
Total messages: 51 (0 generated)
Will, would you be willing to take a look at this? I'm asking you primarily because I'm consciously with hopefully-not-malicious intent creating a RefCountedThreadSafe<> class and I figure you're the person likeliest to give me a hard time about it. This is purely the new class and its unit tests; context for the use of this class can be found in the bug (and arch document linked from there) and in http://codereview.chromium.org/10074001 , which is the larger CL I'm working on that uses this (which isn't quite ready for review yet).
I think I see why you are using refcounting. You are sharing the same object between the producer and consumer, although they use two different interfaces. I do not believe you should use refcounting. I consider it very likely that you will run into the problem where someone else is. I suggest you create two separate classes. A producer and a consumer class. These two can share the same internal core if you want to refcount internally. But the interfaces should not expose refcounting. And it sounds to me like there are two separate interfaces here. It's important to separate the interfaces too, because the separate interfaces are not individually threadsafe. I think they assume there's one consumer and one producer. The ownership is well defined and should use assertions if possible to prevent others from accessing on the wrong thread. Also, be very careful with the return values, especially for the IsFull() member function. The code is multithreaded, so returning a value that is supposed to make a statement about the current condition of the multithreaded object is dangerous. It is not obvious to users of these return values that it is incorrect to rely on it being true for any period of time at all. I'd also like to see an explanation of why locking was used here rather than message passing. http://codereview.chromium.org/10244001/diff/2001/content/browser/download/by... File content/browser/download/byte_stream.h (right): http://codereview.chromium.org/10244001/diff/2001/content/browser/download/by... content/browser/download/byte_stream.h:139: virtual size_t num_source_callbacks() const { return num_source_callbacks_; } There's some chromium-dev thread on unix_hacker_naming() and virtuals. Check it out. http://codereview.chromium.org/10244001/diff/2001/content/browser/download/by... File content/browser/download/byte_stream_unittest.cc (right): http://codereview.chromium.org/10244001/diff/2001/content/browser/download/by... content/browser/download/byte_stream_unittest.cc:10: #include "content/browser/download/byte_stream.h" goes first, see google c++ style guide's include ordering example of basictypes_unittest.cc and basictypes.h.
On 2012/04/27 21:22:29, willchan wrote: > I think I see why you are using refcounting. You are sharing the same object > between the producer and consumer, although they use two different interfaces. I > do not believe you should use refcounting. I consider it very likely that you > will run into the problem where someone else is. I suggest you create two > separate classes. A producer and a consumer class. These two can share the same > internal core if you want to refcount internally. But the interfaces should not > expose refcounting. And it sounds to me like there are two separate interfaces > here. Fair enough. I'll refactor into two interfaces. I had considered that and had decided that the complexity wasn't worth it, but I hadn't considered the point you raise that I could avoid exposing refcounting to the consumers if I did so--that makes it worth it. I'm not certain what I'm going to do about creation semantics, though--I'll have to think about that. > It's important to separate the interfaces too, because the separate interfaces > are not individually threadsafe. I think they assume there's one consumer and > one producer. I should have been explicit, but I think that they *are* individually threadsafe (though the ordering of data is obviously not guaranteed). On the other hand, the use case for which these are targeted is a single producer and a single consumer, so I have no investment in guaranteeing that at the interface level until some use case requires that. > The ownership is well defined and should use assertions if > possible to prevent others from accessing on the wrong thread. So this is a bit tricky, and I'd love to have you share another level of detail. The problem is that while these are currently for use on single threads, at some point I'm going to want to move away from the file thread and towards a SequencedTaskRunner (side question: I've spec'd these to use TaskRunners, but maybe I should spec them to use SequencedTaskRunners; WDYT?). I don't think I have any way of querying or asserting which TaskRunner I am on; is there such a way? If I can't do assertions based on TaskRunner, I'm reluctant to do them based on thread and then later have to yank them back out--I'd rather target the eventual clean, complete interface now. > Also, be very careful with the return values, especially for the IsFull() member > function. The code is multithreaded, so returning a value that is supposed to > make a statement about the current condition of the multithreaded object is > dangerous. It is not obvious to users of these return values that it is > incorrect to rely on it being true for any period of time at all. That is completely fair, and I'm personally disgruntled that I needed IsFull() at all; it has to do with the (IMO, broken) semantics of the relationship between ResourceDispatcherHost and ResourceHandler, specifically that there isn't a way to pause requests from within OnReadCompleted() or OnWillRead(). I'm aware of the point you raise, and I'll update the comment to make it really clear. I believe my use in the consuming CL does not trip over this problem, but I can ask you to review that use if you're concerned about it. To pop up another level of abstraction: If you are on the producing thread, and IsFull() returns false, you are guaranteed that it will not return true in the future if you don't do anything. That's the semantics for which IsFull() is used in the consuming CL. > I'd also like to see an explanation of why locking was used here rather than > message passing. So to my mind message passing isn't really a good fit. Whether through one interface or two, I'm exposing an underlying object that is a pipe. The bytes are pushed into the pipe in one place (thread/task runner) and pulled out in another; neither the push nor the pull actually transfers them between threads. If the producer or consumer shifts threads (which is explicitly allowed by the interface and which I expect to use in a future use case) I don't want to have to be forwarding the information onwards. I'm sure there's some way to do the underlying implementation with message passing, but at the moment I'm really not seeing it. If you can suggest an implementation strategy, I'm happy to consider it, but I'm worried that the mismatch between concept and implementation will trip me up. Does that make sense, or am I just being inordinantly invested in my own creation :-} :-|? I'll respond to the nits (which are fine) when I do the next upload. Thanks for the review--these comments are exactly the kinds of things I was looking for in asking you to review it. http://codereview.chromium.org/10244001/diff/2001/content/browser/download/by... > File content/browser/download/byte_stream.h (right): > > http://codereview.chromium.org/10244001/diff/2001/content/browser/download/by... > content/browser/download/byte_stream.h:139: virtual size_t > num_source_callbacks() const { return num_source_callbacks_; } > There's some chromium-dev thread on unix_hacker_naming() and virtuals. Check it > out. > > http://codereview.chromium.org/10244001/diff/2001/content/browser/download/by... > File content/browser/download/byte_stream_unittest.cc (right): > > http://codereview.chromium.org/10244001/diff/2001/content/browser/download/by... > content/browser/download/byte_stream_unittest.cc:10: #include > "content/browser/download/byte_stream.h" > goes first, see google c++ style guide's include ordering example of > basictypes_unittest.cc and basictypes.h.
On Fri, Apr 27, 2012 at 2:57 PM, <rdsmith@chromium.org> wrote: > On 2012/04/27 21:22:29, willchan wrote: > >> I think I see why you are using refcounting. You are sharing the same >> object >> between the producer and consumer, although they use two different >> interfaces. >> > I > >> do not believe you should use refcounting. I consider it very likely that >> you >> will run into the problem where someone else is. I suggest you create two >> separate classes. A producer and a consumer class. These two can share the >> > same > >> internal core if you want to refcount internally. But the interfaces >> should >> > not > >> expose refcounting. And it sounds to me like there are two separate >> interfaces >> here. >> > > Fair enough. I'll refactor into two interfaces. I had considered that > and had > decided that the complexity wasn't worth it, but I hadn't considered the > point > you raise that I could avoid exposing refcounting to the consumers if I did > so--that makes it worth it. I'm not certain what I'm going to do about > creation > semantics, though--I'll have to think about that. I was sort of imagining you'd have something like source, sink = CreatePipe(); Of course, since C++ sucks and can't return tuples like this, you need to use output params. Or a return a std::pair<Source*,Sink*>. > > > It's important to separate the interfaces too, because the separate >> interfaces >> are not individually threadsafe. I think they assume there's one consumer >> and >> one producer. >> > > I should have been explicit, but I think that they *are* individually > threadsafe > (though the ordering of data is obviously not guaranteed). On the other > hand, > the use case for which these are targeted is a single producer and a single > consumer, so I have no investment in guaranteeing that at the interface > level > until some use case requires that. While I don't think it'll crash if concurrently called, the return values are obviously misleading. Take GetData() for example. How would you know to call GetSourceResult() if you miss the STREAM_COMPLETE return value which may have been returned to another sink? That's more of what I meant by not threadsafe. > > The ownership is well defined and should use assertions if >> possible to prevent others from accessing on the wrong thread. >> > > So this is a bit tricky, and I'd love to have you share another level of > detail. > The problem is that while these are currently for use on single threads, > at > some point I'm going to want to move away from the file thread and towards > a > SequencedTaskRunner (side question: I've spec'd these to use TaskRunners, > but > maybe I should spec them to use SequencedTaskRunners; WDYT?). I don't > think I > have any way of querying or asserting which TaskRunner I am on; is there > such a > way? If I can't do assertions based on TaskRunner, I'm reluctant to do > them > based on thread and then later have to yank them back out--I'd rather > target the > eventual clean, complete interface now. TaskRunner::RunsTasksOnCurrentThread() > > > Also, be very careful with the return values, especially for the IsFull() >> > member > >> function. The code is multithreaded, so returning a value that is >> supposed to >> make a statement about the current condition of the multithreaded object >> is >> dangerous. It is not obvious to users of these return values that it is >> incorrect to rely on it being true for any period of time at all. >> > > That is completely fair, and I'm personally disgruntled that I needed > IsFull() > at all; it has to do with the (IMO, broken) semantics of the relationship > between ResourceDispatcherHost and ResourceHandler, specifically that there > isn't a way to pause requests from within OnReadCompleted() or > OnWillRead(). > I'm aware of the point you raise, and I'll update the comment to make it > really > clear. I believe my use in the consuming CL does not trip over this > problem, > but I can ask you to review that use if you're concerned about it. > > To pop up another level of abstraction: If you are on the producing > thread, and > IsFull() returns false, you are guaranteed that it will not return true in > the > future if you don't do anything. That's the semantics for which IsFull() > is > used in the consuming CL. You've explained the context in which this doesn't cause bugs, but there's nothing in the interface that would prevent people from abusing this code and introduce bugs. I feel that rather than introducing an API that can be badly abused, it's better to fix the buggy code that requires you to add this hack. That said, there are time/productivity tradeoffs that may mean you need this API, at least temporarily. In that case, you should loudly admit this API is horrible by naming the API in a horrible manner that will draw the attention of any author/reviewer. > > > I'd also like to see an explanation of why locking was used here rather >> than >> message passing. >> > > So to my mind message passing isn't really a good fit. Whether through one > interface or two, I'm exposing an underlying object that is a pipe. The > bytes > are pushed into the pipe in one place (thread/task runner) and pulled out > in > another; neither the push nor the pull actually transfers them between > threads. > If the producer or consumer shifts threads (which is explicitly allowed by > the > interface and which I expect to use in a future use case) I don't want to > have > to be forwarding the information onwards. I'm sure there's some way to do > the > underlying implementation with message passing, but at the moment I'm > really not > seeing it. If you can suggest an implementation strategy, I'm happy to > consider > it, but I'm worried that the mismatch between concept and implementation > will > trip me up. Does that make sense, or am I just being inordinantly > invested in > my own creation :-} :-|? > I think I don't have the context to make a good call here. But in general, I argue strongly against locking. Perhaps I should read your other changelist? Oh, btw, I plan to be in CAM in about 2 weeks. If there's no hurry on this CL, then I'd be very happy to discuss this in person. VC would also be wonderful. > > I'll respond to the nits (which are fine) when I do the next upload. > Thanks for > the review--these comments are exactly the kinds of things I was looking > for in > asking you to review it. > > > http://codereview.chromium.**org/10244001/diff/2001/** > content/browser/download/byte_**stream.h<http://codereview.chromium.org/10244001/diff/2001/content/browser/download/byte_stream.h> > >> File content/browser/download/byte_**stream.h (right): >> > > > http://codereview.chromium.**org/10244001/diff/2001/** > content/browser/download/byte_**stream.h#newcode139<http://codereview.chromium.org/10244001/diff/2001/content/browser/download/byte_stream.h#newcode139> > >> content/browser/download/byte_**stream.h:139: virtual size_t >> num_source_callbacks() const { return num_source_callbacks_; } >> There's some chromium-dev thread on unix_hacker_naming() and virtuals. >> Check >> > it > >> out. >> > > > http://codereview.chromium.**org/10244001/diff/2001/** > content/browser/download/byte_**stream_unittest.cc<http://codereview.chromium.org/10244001/diff/2001/content/browser/download/byte_stream_unittest.cc> > >> File content/browser/download/byte_**stream_unittest.cc (right): >> > > > http://codereview.chromium.**org/10244001/diff/2001/** > content/browser/download/byte_**stream_unittest.cc#newcode10<http://codereview.chromium.org/10244001/diff/2001/content/browser/download/byte_stream_unittest.cc#newcode10> > >> content/browser/download/byte_**stream_unittest.cc:10: #include >> "content/browser/download/**byte_stream.h" >> goes first, see google c++ style guide's include ordering example of >> basictypes_unittest.cc and basictypes.h. >> > > > > http://codereview.chromium.**org/10244001/<http://codereview.chromium.org/102... >
On 2012/04/27 22:54:21, willchan wrote: > On Fri, Apr 27, 2012 at 2:57 PM, <mailto:rdsmith@chromium.org> wrote: > > > On 2012/04/27 21:22:29, willchan wrote: > > > >> I think I see why you are using refcounting. You are sharing the same > >> object > >> between the producer and consumer, although they use two different > >> interfaces. > >> > > I > > > >> do not believe you should use refcounting. I consider it very likely that > >> you > >> will run into the problem where someone else is. I suggest you create two > >> separate classes. A producer and a consumer class. These two can share the > >> > > same > > > >> internal core if you want to refcount internally. But the interfaces > >> should > >> > > not > > > >> expose refcounting. And it sounds to me like there are two separate > >> interfaces > >> here. > >> > > > > Fair enough. I'll refactor into two interfaces. I had considered that > > and had > > decided that the complexity wasn't worth it, but I hadn't considered the > > point > > you raise that I could avoid exposing refcounting to the consumers if I did > > so--that makes it worth it. I'm not certain what I'm going to do about > > creation > > semantics, though--I'll have to think about that. > > > I was sort of imagining you'd have something like source, sink = > CreatePipe(); > > Of course, since C++ sucks and can't return tuples like this, you need to > use output params. Or a return a std::pair<Source*,Sink*>. Yeah, it's more that I'm exposing a bare function rather than a class; that feels wrong somehow. But I went with that interface. You'll see it relatively soon :-}. > While I don't think it'll crash if concurrently called, the return values > are obviously misleading. Take GetData() for example. How would you know to > call GetSourceResult() if you miss the STREAM_COMPLETE return value which > may have been returned to another sink? That's more of what I meant by not > threadsafe. Good point. Multiple producers and one consumer, then :-}. (More seriously, yep, single producer, single consumer.) > > So this is a bit tricky, and I'd love to have you share another level of > > detail. > > The problem is that while these are currently for use on single threads, > > at > > some point I'm going to want to move away from the file thread and towards > > a > > SequencedTaskRunner (side question: I've spec'd these to use TaskRunners, > > but > > maybe I should spec them to use SequencedTaskRunners; WDYT?). I don't > > think I > > have any way of querying or asserting which TaskRunner I am on; is there > > such a > > way? If I can't do assertions based on TaskRunner, I'm reluctant to do > > them > > based on thread and then later have to yank them back out--I'd rather > > target the > > eventual clean, complete interface now. > > TaskRunner::RunsTasksOnCurrentThread() Ah. Sure, happy to do that. > You've explained the context in which this doesn't cause bugs, but there's > nothing in the interface that would prevent people from abusing this code > and introduce bugs. I feel that rather than introducing an API that can be > badly abused, it's better to fix the buggy code that requires you to add > this hack. That said, there are time/productivity tradeoffs that may mean > you need this API, at least temporarily. In that case, you should loudly > admit this API is horrible by naming the API in a horrible manner that will > draw the attention of any author/reviewer. I started to write an impassioned defense of the interface, then I thought about it some more. The thing that the producer really wants to rely on is that, if they get a full indication and they've registered a callback, at some point in the future they'll get a callback (presuming the consumer actually eventually consumes the data). The return value from AddData() has that guarantee. The return value from IsFull() does not. My memory is that I had to do some dancing to deal with that issue in the consuming CL, which validates your point. (I presume you compared the AddData return and IsFull() and decided that the former was ok but the latter was not?) I'll spend some time thinking about this, but I'm probably going to either wholeheartedly accept your advice with bells, or get determined/creative in the consuming CL, find a way to avoid needing IsFull(), and remove it. I'll get back to you on this :-}. > > I'd also like to see an explanation of why locking was used here rather > >> than > >> message passing. > >> > > > > So to my mind message passing isn't really a good fit. Whether through one > > interface or two, I'm exposing an underlying object that is a pipe. The > > bytes > > are pushed into the pipe in one place (thread/task runner) and pulled out > > in > > another; neither the push nor the pull actually transfers them between > > threads. > > If the producer or consumer shifts threads (which is explicitly allowed by > > the > > interface and which I expect to use in a future use case) I don't want to > > have > > to be forwarding the information onwards. I'm sure there's some way to do > > the > > underlying implementation with message passing, but at the moment I'm > > really not > > seeing it. If you can suggest an implementation strategy, I'm happy to > > consider > > it, but I'm worried that the mismatch between concept and implementation > > will > > trip me up. Does that make sense, or am I just being inordinantly > > invested in > > my own creation :-} :-|? > > > > I think I don't have the context to make a good call here. But in general, > I argue strongly against locking. Perhaps I should read your other > changelist? Oh, btw, I plan to be in CAM in about 2 weeks. If there's no > hurry on this CL, then I'd be very happy to discuss this in person. VC > would also be wonderful. Hmmm. I'm going to be in MTV in two weeks too :-}. (5/6-5/12.) But whether or not we'll overlap, I would like to resolve this, if possible, sooner than that. On the one hand, I've certainly got plenty of other stuff to do. On the other hand, I've got a chunk of refactoring I really want to get in in the next two quarters of which this is the clear first step, and ugly code that needs this + a few extra things in to allow us to clean it up. VC early next week?
Will: Willing to take a scan through this and tell me what you think? This is in nothing like final form (e.g. I don't think the tests compile ATM :-}) so I'm just looking for high level, is this a reasonable implementation pathway feedback. I'll continue to speculatively execute assuming that it is ok :-}.
Will: I've finished the rewrite and am ready for a real review. WDYT? (Side question, can be answered later: Would this work satisfy C++ readability requirements?)
Totally would satisfy readability. Sorry, I'm getting DDoS'd by code reviews. On Fri, May 4, 2012 at 11:52 AM, <rdsmith@chromium.org> wrote: > Will: I've finished the rewrite and am ready for a real review. WDYT? > > (Side question, can be answered later: Would this work satisfy C++ > readability > requirements?) > > > http://codereview.chromium.**org/10244001/<http://codereview.chromium.org/102... >
On 2012/05/04 18:53:44, willchan wrote: > Totally would satisfy readability. Sorry, I'm getting DDoS'd by code > reviews. No worries. If it takes you longer than a week, I'll be in SFO and can beat you up directly :-}. (Though truth to tell, I'm hoping it'll take you less time than that. ) One heads up: It looks as if I can actually define this in terms of scoped_array<char> rather than net::IOBuffer, which would make me very happy. I'm not going to do that in this CL, as making sure it'll work is part of the larger integration with the downloads system. But since I am planning to do it, it seemed fair to let you know as part of the initial review. > > On Fri, May 4, 2012 at 11:52 AM, <mailto:rdsmith@chromium.org> wrote: > > > Will: I've finished the rewrite and am ready for a real review. WDYT? > > > > (Side question, can be answered later: Would this work satisfy C++ > > readability > > requirements?) > > > > > > > http://codereview.chromium.**org/10244001/%3Chttp://codereview.chromium.org/1...> > >
I'm won't be in SFO this week. http://codereview.chromium.org/10244001/diff/17002/content/browser/download/b... File content/browser/download/byte_stream.h (right): http://codereview.chromium.org/10244001/diff/17002/content/browser/download/b... content/browser/download/byte_stream.h:61: public: I'm a bit surprised this interface isn't more like the sending side of a socket. Why isn't it int Write(buffer, len, callback) and Close(DownloadInterruptReason)? I suggest an |int| because perhaps we should return ERR_CONNECTION_CLOSED or something if the reading side is already closed. http://codereview.chromium.org/10244001/diff/17002/content/browser/download/b... content/browser/download/byte_stream.h:118: // Statistics. On the output side of the byte stream to indicate Why all these stats? Can't this be done in a composed object that also inherits from ByteStreamOutput, if you really want it?
> I'm won't be in SFO this week. And you're not even taking advantage of it by delaying the review :-}. (Thank you, by the way.) No code changes yet; these are agreements/clarifying questions on your comments. http://codereview.chromium.org/10244001/diff/17002/content/browser/download/b... File content/browser/download/byte_stream.h (right): http://codereview.chromium.org/10244001/diff/17002/content/browser/download/b... content/browser/download/byte_stream.h:61: public: On 2012/05/07 23:08:51, willchan wrote: > I'm a bit surprised this interface isn't more like the sending side of a socket. > Why isn't it int Write(buffer, len, callback) and > Close(DownloadInterruptReason)? I'm happy to switch over to Write/Close; that's a good suggestion. My instinct is that I'd rather keep callback separate, because it's not associated with any particular write; are you suggesting that because you envision the contract as "either return true or promise to call the callback?" > I suggest an |int| because perhaps we should return ERR_CONNECTION_CLOSED or > something if the reading side is already closed. Hmmm. I rather value the one way propagation of error information; at least for my use cases, I'm imagining the source can be written pretty simply and the sync has the error processing responsibility. But that interface would certainly allow that programming model. It just makes it a bit more complex interface, and there isn't currently a use case for the functionality. WDYT? If we go that way, I'd rather return an enum than an int (i.e. create my own namespace); that ok? http://codereview.chromium.org/10244001/diff/17002/content/browser/download/b... content/browser/download/byte_stream.h:118: // Statistics. On the output side of the byte stream to indicate On 2012/05/07 23:08:51, willchan wrote: > Why all these stats? Can't this be done in a composed object that also inherits > from ByteStreamOutput, if you really want it? Reasonable. I'll make the change. The reason for the stats is that I want to put into UMA some notes on what the amortization tradeoffs that I'm making for propagating data are costing me, but that's a download specific question rather than a general ByteStream one, so I wanted to expose information about the ByteStream so that I could implement the stats a layer up. (It was more obvious it was a download specific question when I had control knobs on the ByteStream controlling the hysteresis & etc.; possibly I could just stick the UMA into ByteStream now.)
http://codereview.chromium.org/10244001/diff/17002/content/browser/download/b... File content/browser/download/byte_stream.h (right): http://codereview.chromium.org/10244001/diff/17002/content/browser/download/b... content/browser/download/byte_stream.h:61: public: On 2012/05/07 23:26:01, rdsmith wrote: > My instinct > is that I'd rather keep callback separate, because it's not associated with any > particular write; are you suggesting that because you envision the contract as > "either return true or promise to call the callback?" A point in support of keeping the callback separate: If we associate the callback with the Write call, we have the problem of how to spec multiple write calls when the pipe is full (i.e. pick which callback we choose to call). This can be done, but sorta emphasizes that the callback is really a property of the ByteStream, not of the individual Write calls.
Will: Not having heard back from you, I suited myself as to which of your comments I took and which I didn't :-}. Specifically, what I've done is: * Changed names (AddData->Write, GetData->Read, SourceComplete->Close, GetSourceResult -> GetStatus). * Removed stat interface. Please let me know what you think. I'm going to keep my fingers crossed that we're getting close, and merge this CL into the larger CL that uses it.
Remember that what you have doesn't really have any specific reason to be tied to content/. For the best integration with Google Drive, you may want to consider how Google Drive will do file uploads. They will want an abstraction similar to this. Now, if you do a download from one cloud filesystem to upload to another cloud filesystem, I could see URLRequest wanting to take the Sink interface as a parameter for the POST, where the Source end may actually itself be a Sink in the download system. This provides an elegant chaining of pipes here. The degree to which you want to add download-isms into the code will prevent the use of this across subsystems and require conversion costs. http://codereview.chromium.org/10244001/diff/17002/content/browser/download/b... File content/browser/download/byte_stream.h (right): http://codereview.chromium.org/10244001/diff/17002/content/browser/download/b... content/browser/download/byte_stream.h:61: public: On 2012/05/07 23:26:01, rdsmith wrote: > On 2012/05/07 23:08:51, willchan wrote: > > I'm a bit surprised this interface isn't more like the sending side of a > socket. > > Why isn't it int Write(buffer, len, callback) and > > Close(DownloadInterruptReason)? > > I'm happy to switch over to Write/Close; that's a good suggestion. My instinct > is that I'd rather keep callback separate, because it's not associated with any > particular write; are you suggesting that because you envision the contract as > "either return true or promise to call the callback?" > > > I suggest an |int| because perhaps we should return ERR_CONNECTION_CLOSED or > > something if the reading side is already closed. > > Hmmm. I rather value the one way propagation of error information; at least for > my use cases, I'm imagining the source can be written pretty simply and the sync > has the error processing responsibility. But that interface would certainly > allow that programming model. It just makes it a bit more complex interface, > and there isn't currently a use case for the functionality. WDYT? I'm surprised there's no use case. What happens when the underlying sink is Google Drive or another kind of network filesystem? Or the source? Do we really want to keep reading from whatever (possibly the network) and writing to the source even though the sink has gone away? I think I would be hard pressed to find a pipe abstraction in a standard library that didn't account for this. > > If we go that way, I'd rather return an enum than an int (i.e. create my own > namespace); that ok? Sure. You may want to consider the error cases though. If sources/sinks are backed by the network, and go through the network stack, it's likely they'll return errors in net::Error form. If you want to ever want to expose that to a higher layer, you may end up having to write mapping code between your namespace and the net::Error namespace. You know your use case better than I, just be aware of this possibility. http://codereview.chromium.org/10244001/diff/17002/content/browser/download/b... content/browser/download/byte_stream.h:61: public: On 2012/05/08 05:27:04, rdsmith wrote: > On 2012/05/07 23:26:01, rdsmith wrote: > > My instinct > > is that I'd rather keep callback separate, because it's not associated with > any > > particular write; are you suggesting that because you envision the contract as > > "either return true or promise to call the callback?" > > A point in support of keeping the callback separate: If we associate the > callback with the Write call, we have the problem of how to spec multiple write > calls when the pipe is full (i.e. pick which callback we choose to call). This > can be done, but sorta emphasizes that the callback is really a property of the > ByteStream, not of the individual Write calls. We avoid this problem in net::Socket code by disallowing multiple write calls when the pipe is full. If you return an ERR_IO_PENDING type of return value, then the caller should know not to write again. This is a common design paradigm. As far as registering a callback once as opposed to per write/read, registering it once is fine. You should probably call it "setting" as opposed to "registering", since it's common to "register" multiple items, whereas you make it sound like you only want to set it once. One thing that makes me disagree with the comment about it always being a property of the source's owner and not the individual write/read call, is sometimes, for zero-copy purposes, you want to pass the pipe around so the appropriate user can read/write directly from/to a buffer. That's a more advanced use though, and if you don't want to support that, that's probably OK.
[CCing Satoru-san for his perspective on the abstraction, as the obvious other possible consumer. ] > Remember that what you have doesn't really have any specific reason to be tied > to content/. For the best integration with Google Drive, you may want to > consider how Google Drive will do file uploads. They will want an abstraction > similar to this. Now, if you do a download from one cloud filesystem to upload > to another cloud filesystem, I could see URLRequest wanting to take the Sink > interface as a parameter for the POST, where the Source end may actually itself > be a Sink in the download system. This provides an elegant chaining of pipes > here. The degree to which you want to add download-isms into the code will > prevent the use of this across subsystems and require conversion costs. [So just to be clear, I don't understand the example you give of URLRequest wanting to take a Sink as a parameter for the Post, but I think I get the basic point about the more downloadisms I have, the less generally useful the abstraction is. ] So I struggled with this when I was first designing the class--it was originally in net, and returned a net::Error (which I considered more general than the download error return), but that didn't satisfy my use case, as the DownloadResourceHandler needed to specify some errors that weren't in net::Error. I briefly thought about using "int" and letting the caller decide what that means (DownloadInterruptReason, net::Error, errno, etc.) and really didn't like that--the meanings of those codes are pretty important to how the software works, and I wanted to be specific about where they were coming from. All use cases I'm currently thinking about are in the download system. So I decided to just target my use case. It occurs to me that another alternative could be to simple add the error codes I need to net::Error, move this into net/ and make it pass a net::Error. Do you think that might fly? The non-net::Error codes I need are: * User cancelled * Server bad content * Server no range * Server precondition failed * Generic server failed I could imagine that there might be some push back about adding these; at least, the user cancel case had a use case previously but wasn't added. But from my perspective it would be a reasonable way to get around this problem, and would probably mean I could get rid of a type within the download system. > > Hmmm. I rather value the one way propagation of error information; at least > for > > my use cases, I'm imagining the source can be written pretty simply and the > sync > > has the error processing responsibility. But that interface would certainly > > allow that programming model. It just makes it a bit more complex interface, > > and there isn't currently a use case for the functionality. WDYT? > > I'm surprised there's no use case. What happens when the underlying sink is > Google Drive or another kind of network filesystem? Or the source? Do we really > want to keep reading from whatever (possibly the network) and writing to the > source even though the sink has gone away? I think I would be hard pressed to > find a pipe abstraction in a standard library that didn't account for this. Remember it's flow controlled, so we don't keep reading; we fill up the pipe and stop. But that's in some ways a minor point. The class is spec'd that error flow from sink to source must go through an external controller, which is (I believe) the right thing for the downloads system; the current downloads system have errors and cancellations going every which way, and it's not clear whether you're covering your use cases or not. It needs some regularization. That regularization doesn't need to be enforced by this class, but I'm not adverse to encouraging what I see as proper flow. OTOH, that rules out use cases in the future that want the reverse error flow. OTTH, those use cases might have the same issues as downloads and it would be useful discipline for them to have to go through the controller. I don't know, so I restrict to my current use cases. More generally, I think this is a case of "as simple as possible, and no simpler" where you and I have different instincts on whether this is too simple. Having the source know nothing about the status of the sink other than whether it can accept data keeps the source interface very simple, and for the use cases I imagine (for all of which I think there has to be some external controller structuring things), the backwards error flow isn't needed. And if we do put it in, there's the obvious temptation to add more general error codes (as you suggested) and we need to deal with the fact that that more general error code space doesn't have the values for what we need to express (pipe full) and we'd need to enhance it to do so. I hear you about inconsistency with other pipe implementations, and I'm not sure how to respond to it--being tied to my own invention despite the wisdom of the years is a standard programmer failure mode. But despite that I feel pulled towards as simple an abstraction as I can manage. Which I guess comes down to: If you push me on this, I'll yield (to the extent of a simple enum that includes "closed"--I'm not going to expand to more general error cases without more argument or a very concrete use case, as I think that's a lot of messiness for no purpose.) But if I get to make the decision I'll leave it like this until there's a need to change it. > http://codereview.chromium.org/10244001/diff/17002/content/browser/download/b... > content/browser/download/byte_stream.h:61: public: > On 2012/05/08 05:27:04, rdsmith wrote: > > On 2012/05/07 23:26:01, rdsmith wrote: > > > My instinct > > > is that I'd rather keep callback separate, because it's not associated with > > any > > > particular write; are you suggesting that because you envision the contract > as > > > "either return true or promise to call the callback?" > > > > A point in support of keeping the callback separate: If we associate the > > callback with the Write call, we have the problem of how to spec multiple > write > > calls when the pipe is full (i.e. pick which callback we choose to call). > This > > can be done, but sorta emphasizes that the callback is really a property of > the > > ByteStream, not of the individual Write calls. > > We avoid this problem in net::Socket code by disallowing multiple write calls > when the pipe is full. If you return an ERR_IO_PENDING type of return value, > then the caller should know not to write again. This is a common design > paradigm. Completely fair. I have a minor preference for allowing multiple write calls (making the flow control advisory) because we're not really allocating space in the pipe; we're passing around pointers, so it's not "really" full. It's more implementing flow control on behalf of and at the request of the consumers. But the real requirement that drives this is that pausing synchronously in resource handlers didn't look like it was doable to me, and Darin may be fixing that. If that happens (fingers crossed) I'll only be left with my minor preference, and if you felt strongly I'd yield. >> As far as registering a callback once as opposed to per write/read, registering > it once is fine. You should probably call it "setting" as opposed to > "registering", since it's common to "register" multiple items, whereas you make > it sound like you only want to set it once. Sure, though I plan to spec the interface so that you can overwrite the callback for the below requirement, and to block the callback when you don't want it anymore. > One thing that makes me disagree with the comment about it always being a > property of the source's owner and not the individual write/read call, is > sometimes, for zero-copy purposes, you want to pass the pipe around so the > appropriate user can read/write directly from/to a buffer. That's a more > advanced use though, and if you don't want to support that, that's probably OK. Good point, and I would like to support that use case (it's actually a target in GData for passing the sink from downloads system to Gdata). Given that we're single threaded (/SequencedTaskRunnerred :-}), I think that the set operator does support it, but I see the argument for associating it with a particular write. But what we're really doing is associating the callback with a particular consumer, and that's coarser granularity than a write, and finer than the whole pipe. Based on that analysis if there weren't other factors I'd go with the finer granularity, but I still have the minor preference (plus the "I don't have the code yet to drop my pause-driven requirement") for making the push back advisory. So I'm inclined to leave the callback on the ByteStream. Summary: * I have strong feelings about error code spaces, and would like to stay within a solution that keps those spaces clean and simple without pretty clear use cases that that breaks. * I need the push back to be advisory until Darin does his magic, at which case I'd like it to be as I think it targets my expected use cases best but would be willing to change it if you felt strongly. * If we make the pushback non-advisory I'd associate the callback with the write rather than with the pipe, but since I don't mind associating it with the pipe, for me this isn't an argument to make the pushback non-advisory.
Sorry, one followup which my response was based on but that I didn't say explicitly: My current understanding is that the GData use cases go through the download system, and hence we do not block those use cases by the downloadisms in this code.
Randy, thank you for adding me. I'll take a look. +achuith and +zelidrag who would be interested in this.
Quick summary for Satoru-san and anyone else who's tracking: Will and I had a long discussion during his visit to Cambridge. We decided: * Worrying too too much about making this interface general for abstract anticipated future use cases is probably not energy well spent; if it works for the set of use cases we currently have on the horizon, that's good enough, and we can revisit the issues as new use cases arise. * One particular area that might or might not be important in the GData case is the flow of error information. As currently spec'd, this class flows error information from source to sink only, and any error information that must flow from sink to source goes OOB though some other mechanism. This was done to encourage a specific type of programming that I think would be cleaner in the downloads case. I am perfectly happy to enhance this class to flow error information from sink to source, but I'd rather do that in the context of a particular use case that needs it than in the abstract. So: If GData wants to use ByteStream in a way that could use sink->source error info, I'm happy to adapt to it. * The class is currently in content so that the source->sink error information can be expressed in a DownloadInterruptReason. I have no objection, and some interest, in making it more general, but I don't currently see how without doing something like making the error info an int and leaving it up to consumers how it's interpreted, wihch is disturbing to me. So I'm open to (e.g.) moving this into net:: and (e.g.) using an expanded net::Error to cover the error cases I need, but that would require some design evaluation. * Will's still not completely comfortable with the advisory nature of the interface (in that it might encourage programming styles that court out of memory behavior), but is happy to get experience with it in the download context and possibly revisit the issue if we try to expand it outside the downloads context. Given all this, I think Will's comfortable with the interface, but he'll make that official (or not) after a quick read through this afternoon.
I'm fine with this interface given the caveats that rdsmith already listed. I'm curious to see how this plays out. http://codereview.chromium.org/10244001/diff/24003/content/browser/download/b... File content/browser/download/byte_stream.h (right): http://codereview.chromium.org/10244001/diff/24003/content/browser/download/b... content/browser/download/byte_stream.h:84: // |empty_percentage| is an integer in the range 0-100 that specifies how Is |empty_percentage| still here? http://codereview.chromium.org/10244001/diff/24003/content/browser/download/b... content/browser/download/byte_stream.h:89: virtual void RegisterCallback(base::Closure source_callback) = 0; const base::Closure&
Will's comments incorporated; Will you're welcome to re-review or not as you wish. Satoru-san: You're currently listed as a blocking reviewer. Do you want to be, or should I move you to the CC list? Ben: Will's ok'd the interface, and taken himself off the review WRT implementation and testing. Could you review those files? For context, the CL that uses this abstraction is http://codereview.chromium.org/10392111/. Please note that that CL is not yet ready for review; specifically, I haven't merged the most recent version of this CL in, and it's got several changes "in progress". http://codereview.chromium.org/10244001/diff/24003/content/browser/download/b... File content/browser/download/byte_stream.h (right): http://codereview.chromium.org/10244001/diff/24003/content/browser/download/b... content/browser/download/byte_stream.h:84: // |empty_percentage| is an integer in the range 0-100 that specifies how On 2012/05/15 19:41:32, willchan wrote: > Is |empty_percentage| still here? Whoops; no. I put in controllable hysteresis into the original class, and decided I was getting ahead of myself (implementing features without targeted use cases) and removed it in one of the rewrites. Fixed. http://codereview.chromium.org/10244001/diff/24003/content/browser/download/b... content/browser/download/byte_stream.h:89: virtual void RegisterCallback(base::Closure source_callback) = 0; On 2012/05/15 19:41:32, willchan wrote: > const base::Closure& Done.
Arggh. Really truly adding Ben as reviewer this time.
My concern about this approach for handling drive download+upload is with respect to flow control. Currently we download to file, and simultaneously upload from the file. The file acts as our intermediate buffer. The file is not just a temporary - we also want to cache the file (and we currently do) because it's very common for the user to open a file she has just downloaded, and having to re-download it would be a bad experience. The in-memory buffer ByteStream approach links the download and upload speeds. So if the download is much faster than the upload (as it is in practice), it's going to get held up by the upload. The overall rate of transfer is the slowest rate of all your sinks and sources. Currently, if the download finishes but the upload hasn't finished, we have an option (currently not implemented) of detecting this and resuming the upload. I think we lose this option when we move to the new model.
I'm swapmed by random m20 issues now, so please don't want for my comments. achuith, do you want to take a look?
On 2012/05/15 23:05:43, satorux1 wrote: > I'm swapmed by random m20 issues now, so please don't want for my comments. > achuith, do you want to take a look? Sure.
On 2012/05/15 23:05:42, achuith.bhandarkar wrote: > My concern about this approach for handling drive download+upload is with > respect to flow control. > > Currently we download to file, and simultaneously upload from the file. The file > acts as our intermediate buffer. The file is not just a temporary - we also want > to cache the file (and we currently do) because it's very common for the user to > open a file she has just downloaded, and having to re-download it would be a bad > experience. > > The in-memory buffer ByteStream approach links the download and upload speeds. > So if the download is much faster than the upload (as it is in practice), it's > going to get held up by the upload. The overall rate of transfer is the slowest > rate of all your sinks and sources. > > Currently, if the download finishes but the upload hasn't finished, we have an > option (currently not implemented) of detecting this and resuming the upload. I > think we lose this option when we move to the new model. My model for how we're going to rewrite the GData stuff with this abstraction is that we'll start out downloading to a file, then find out that we're targeting GData, and hand off both the file and the ByteStreamOutput pointer to you guys to do whatever you want with it. You are welcome to use the ByteStream abstraction within your implementation, or to have something that looks like the current implementation with the extra benefit that since you're the ones writing to the file you don't have to track updates and name changes. I can see why you'd rather use the file for buffering than an in-memory buffer such as ByteStream implies; possibly you can implement a fork that writes to the file and to a ByteStream read by the upload code, but uses the file for buffering. Or possibly that's not the right abstraction. But the bottom line is: * I don't think using ByteStream for communication from downloads system to GData imposes any restrictions on the GData implementation, and * I don't offhand see how to expand the ByteStream semantics to make it more useful for the GData internal implementation, but if you do, I'm quite interested and would be happy to consider adding new semantics. Any ideas?
nits. I probably won't have any comments about the architecture. http://codereview.chromium.org/10244001/diff/17008/content/browser/download/b... File content/browser/download/byte_stream.cc (right): http://codereview.chromium.org/10244001/diff/17008/content/browser/download/b... content/browser/download/byte_stream.cc:15: typedef std::deque<std::pair<scoped_refptr<net::IOBuffer>,size_t> > Space after comma. http://codereview.chromium.org/10244001/diff/17008/content/browser/download/b... content/browser/download/byte_stream.cc:23: // threads rather than task runners. TODO: TaskRunnerWeakPointer? http://codereview.chromium.org/10244001/diff/17008/content/browser/download/b... content/browser/download/byte_stream.cc:27: bool is_alive_; blank line between sections? http://codereview.chromium.org/10244001/diff/17008/content/browser/download/b... content/browser/download/byte_stream.cc:31: }; DISALLOW_COPY_AND_ASSIGN? http://codereview.chromium.org/10244001/diff/17008/content/browser/download/b... File content/browser/download/byte_stream.h (right): http://codereview.chromium.org/10244001/diff/17008/content/browser/download/b... content/browser/download/byte_stream.h:34: // and the sync retrieves bytes already written via |ByteStreamOutput::Read|. s/sync/sink/g? http://codereview.chromium.org/10244001/diff/17008/content/browser/download/b... content/browser/download/byte_stream.h:46: // relationship, to signal the source in some other fashion. Does this contradict the second sentence of the first paragraph? http://codereview.chromium.org/10244001/diff/17008/content/browser/download/b... content/browser/download/byte_stream.h:55: // callback strategy, callbacks will always be evaluated immediately Sorry, what does it mean to evaluate a callback? http://codereview.chromium.org/10244001/diff/17008/content/browser/download/b... content/browser/download/byte_stream.h:75: // Register a callback to be called M-q to reflow? http://codereview.chromium.org/10244001/diff/17008/content/browser/download/b... content/browser/download/byte_stream.h:90: typedef enum { STREAM_EMPTY, STREAM_HAS_DATA, STREAM_COMPLETE } StreamState; Why do you need typedef? http://codereview.chromium.org/10244001/diff/17008/content/browser/download/b... content/browser/download/byte_stream.h:106: // Register a callback to be called M-q? http://codereview.chromium.org/10244001/diff/17008/content/browser/download/b... content/browser/download/byte_stream.h:119: scoped_refptr<base::SequencedTaskRunner> input_task_runner, I thought that function inputs (task_runners, buffer_size) usually preceded outputs (byte stream halves). Maybe that's just my verilog showing.
more nits http://codereview.chromium.org/10244001/diff/17008/content/browser/download/b... File content/browser/download/byte_stream.cc (right): http://codereview.chromium.org/10244001/diff/17008/content/browser/download/b... content/browser/download/byte_stream.cc:230: No blank lines at beginning/ends of blocks. http://codereview.chromium.org/10244001/diff/17008/content/browser/download/b... content/browser/download/byte_stream.cc:234: DCHECK_GE(target->output_size_used_, bytes_consumed); Want to put the rest of this function in a method so you don't need to say target->? http://codereview.chromium.org/10244001/diff/17008/content/browser/download/b... content/browser/download/byte_stream.cc:250: // Arbitrarily, we buffer to a third of the total size before sending. Does this contradict L248? http://codereview.chromium.org/10244001/diff/17008/content/browser/download/b... content/browser/download/byte_stream.cc:251: if (input_contents_size_ > total_buffer_size_ / 3) { Invert and early-return? http://codereview.chromium.org/10244001/diff/17008/content/browser/download/b... content/browser/download/byte_stream.cc:251: if (input_contents_size_ > total_buffer_size_ / 3) { Use a kConstant instead of 3? http://codereview.chromium.org/10244001/diff/17008/content/browser/download/b... content/browser/download/byte_stream.cc:354: if (target->available_contents_size_ == buffer_size && Looks like you can get rid of available_contents_size_ if you look at available_contents_ instead. Looks like you wouldn't even need a loop anywhere. http://codereview.chromium.org/10244001/diff/17008/content/browser/download/b... content/browser/download/byte_stream.cc:401: return; Feeling philosophical?
two more ideas http://codereview.chromium.org/10244001/diff/17008/content/browser/download/b... File content/browser/download/byte_stream_unittest.cc (right): http://codereview.chromium.org/10244001/diff/17008/content/browser/download/b... content/browser/download/byte_stream_unittest.cc:61: void NullCallback() { Would something like this work just as well with less code and insulate between TEST_Fs? void CountCallbacks(int* counter) { ++*counter; } TEST_F(...) { int counter_a = 0; int counter_b = 0; int counter_c = 0; base::Bind(&CountCallbacks, &counter_a); base::Bind(&CountCallbacks, &counter_b); base::Bind(&CountCallbacks, &counter_c); ... EXPECT_EQ(42, counter_a); EXPECT_EQ(42, counter_b); EXPECT_EQ(42, counter_c); counter_a = counter_b = counter_c = 0; ... EXPECT_EQ(42, counter_a); EXPECT_EQ(42, counter_b); EXPECT_EQ(42, counter_c); } http://codereview.chromium.org/10244001/diff/17008/content/browser/download/b... content/browser/download/byte_stream_unittest.cc:76: // |seed_key| value within a single test. Can you use an internal automatic counter instead of magic numbers and fallible human counters?
Implementation architecture looks good. Are you planning on just changing all IOBuffers to scoped_ptr_array<char>s or something more interesting like pair<scoped_ptr_array<char>, size_t> or a class? Has there been any talk of importing Google's internal IOBuffer? Few more nits. http://codereview.chromium.org/10244001/diff/17008/content/browser/download/b... File content/browser/download/byte_stream.cc (right): http://codereview.chromium.org/10244001/diff/17008/content/browser/download/b... content/browser/download/byte_stream.cc:198: void ByteStreamInputImpl::Close( This seems to duplicate some of MaybePostToPeer(). Could they be merged? http://codereview.chromium.org/10244001/diff/17008/content/browser/download/b... content/browser/download/byte_stream.cc:364: if (unreported_consumed_bytes_ > total_buffer_size_ / 3) { Invert and early-return? http://codereview.chromium.org/10244001/diff/17008/content/browser/download/b... File content/browser/download/byte_stream_unittest.cc (right): http://codereview.chromium.org/10244001/diff/17008/content/browser/download/b... content/browser/download/byte_stream_unittest.cc:124: private: Blank line between sections? http://codereview.chromium.org/10244001/diff/17008/content/browser/download/b... content/browser/download/byte_stream_unittest.cc:131: TEST_F(ByteStreamTest, PushBack) { Would you mind prepending "ByteStream_" to all of the TEST_F case names so that we see "ByteStream_PushBack" etc. instead of mysteriously "PushBack" on the bot summary pages?
> Are you planning on just changing all IOBuffers to scoped_ptr_array<char>s or > something more interesting like pair<scoped_ptr_array<char>, size_t> or a > class? I had been aiming for scoped_array<char>, but I'll take a look at scoped_ptr_array. I'm disinclined to create the class unless I see more general usefulness for it. But we can have that discussion on the next CL (which is where I'm going to the the transformation). > Has there been any talk of importing Google's internal IOBuffer? Not that I'm aware of. PTAL. http://codereview.chromium.org/10244001/diff/17008/content/browser/download/b... File content/browser/download/byte_stream.cc (right): http://codereview.chromium.org/10244001/diff/17008/content/browser/download/b... content/browser/download/byte_stream.cc:15: typedef std::deque<std::pair<scoped_refptr<net::IOBuffer>,size_t> > On 2012/05/16 14:26:25, benjhayden_chromium wrote: > Space after comma. Done. http://codereview.chromium.org/10244001/diff/17008/content/browser/download/b... content/browser/download/byte_stream.cc:23: // threads rather than task runners. On 2012/05/16 14:26:25, benjhayden_chromium wrote: > TODO: TaskRunnerWeakPointer? Done. http://codereview.chromium.org/10244001/diff/17008/content/browser/download/b... content/browser/download/byte_stream.cc:27: bool is_alive_; On 2012/05/16 14:26:25, benjhayden_chromium wrote: > blank line between sections? Done. http://codereview.chromium.org/10244001/diff/17008/content/browser/download/b... content/browser/download/byte_stream.cc:31: }; On 2012/05/16 14:26:25, benjhayden_chromium wrote: > DISALLOW_COPY_AND_ASSIGN? Done. http://codereview.chromium.org/10244001/diff/17008/content/browser/download/b... content/browser/download/byte_stream.cc:198: void ByteStreamInputImpl::Close( On 2012/05/16 19:17:21, benjhayden_chromium wrote: > This seems to duplicate some of MaybePostToPeer(). Could they be merged? Done. http://codereview.chromium.org/10244001/diff/17008/content/browser/download/b... content/browser/download/byte_stream.cc:230: On 2012/05/16 15:41:13, benjhayden_chromium wrote: > No blank lines at beginning/ends of blocks. Done. http://codereview.chromium.org/10244001/diff/17008/content/browser/download/b... content/browser/download/byte_stream.cc:234: DCHECK_GE(target->output_size_used_, bytes_consumed); On 2012/05/16 15:41:13, benjhayden_chromium wrote: > Want to put the rest of this function in a method so you don't need to say > target->? Done. http://codereview.chromium.org/10244001/diff/17008/content/browser/download/b... content/browser/download/byte_stream.cc:250: // Arbitrarily, we buffer to a third of the total size before sending. On 2012/05/16 15:41:13, benjhayden_chromium wrote: > Does this contradict L248? Yep; oops. Fixed. http://codereview.chromium.org/10244001/diff/17008/content/browser/download/b... content/browser/download/byte_stream.cc:251: if (input_contents_size_ > total_buffer_size_ / 3) { On 2012/05/16 15:41:13, benjhayden_chromium wrote: > Invert and early-return? Obsolete because of refactor done for another comment. http://codereview.chromium.org/10244001/diff/17008/content/browser/download/b... content/browser/download/byte_stream.cc:251: if (input_contents_size_ > total_buffer_size_ / 3) { On 2012/05/16 15:41:13, benjhayden_chromium wrote: > Use a kConstant instead of 3? Done. I'm a bit hesitant because if I change this in the future I'll want to give control on a percentage of buffer basis (i.e. not just an integer fraction) but this'll call out the location in the code where that needs to be done pretty clearly. http://codereview.chromium.org/10244001/diff/17008/content/browser/download/b... content/browser/download/byte_stream.cc:354: if (target->available_contents_size_ == buffer_size && On 2012/05/16 15:41:13, benjhayden_chromium wrote: > Looks like you can get rid of available_contents_size_ if you look at > available_contents_ instead. Looks like you wouldn't even need a loop anywhere. How do you figure? I agree available_contents_size_ is redundant with available_contents_, but I think I'd need to loop over AC_ getting the size of each buffer to recreate ACS_. Do you think that's worth it to eliminate the redundancy? Or is there some way to avoid the loop I'm missing? http://codereview.chromium.org/10244001/diff/17008/content/browser/download/b... content/browser/download/byte_stream.cc:364: if (unreported_consumed_bytes_ > total_buffer_size_ / 3) { On 2012/05/16 19:17:21, benjhayden_chromium wrote: > Invert and early-return? Done. http://codereview.chromium.org/10244001/diff/17008/content/browser/download/b... content/browser/download/byte_stream.cc:401: return; On 2012/05/16 15:41:13, benjhayden_chromium wrote: > Feeling philosophical? You know, it took me a good fifteen seconds to figure out what you meant. Chuckle. Done. http://codereview.chromium.org/10244001/diff/17008/content/browser/download/b... File content/browser/download/byte_stream.h (right): http://codereview.chromium.org/10244001/diff/17008/content/browser/download/b... content/browser/download/byte_stream.h:34: // and the sync retrieves bytes already written via |ByteStreamOutput::Read|. On 2012/05/16 14:26:25, benjhayden_chromium wrote: > s/sync/sink/g? Done. http://codereview.chromium.org/10244001/diff/17008/content/browser/download/b... content/browser/download/byte_stream.h:46: // relationship, to signal the source in some other fashion. On 2012/05/16 14:26:25, benjhayden_chromium wrote: > Does this contradict the second sentence of the first paragraph? Modified the text a bit. My intention is that the source and sink have no direct knowledge of each other; there may be indirect links (and will be in the downloads system use case). http://codereview.chromium.org/10244001/diff/17008/content/browser/download/b... content/browser/download/byte_stream.h:55: // callback strategy, callbacks will always be evaluated immediately On 2012/05/16 14:26:25, benjhayden_chromium wrote: > Sorry, what does it mean to evaluate a callback? Modified the text; let me know what you think. http://codereview.chromium.org/10244001/diff/17008/content/browser/download/b... content/browser/download/byte_stream.h:75: // Register a callback to be called On 2012/05/16 14:26:25, benjhayden_chromium wrote: > M-q to reflow? Done. http://codereview.chromium.org/10244001/diff/17008/content/browser/download/b... content/browser/download/byte_stream.h:90: typedef enum { STREAM_EMPTY, STREAM_HAS_DATA, STREAM_COMPLETE } StreamState; On 2012/05/16 14:26:25, benjhayden_chromium wrote: > Why do you need typedef? Because I'm an old programmer who isn't used to the new fangled compilers and language specs :-}. Done. http://codereview.chromium.org/10244001/diff/17008/content/browser/download/b... content/browser/download/byte_stream.h:106: // Register a callback to be called On 2012/05/16 14:26:25, benjhayden_chromium wrote: > M-q? Done. http://codereview.chromium.org/10244001/diff/17008/content/browser/download/b... content/browser/download/byte_stream.h:119: scoped_refptr<base::SequencedTaskRunner> input_task_runner, On 2012/05/16 14:26:25, benjhayden_chromium wrote: > I thought that function inputs (task_runners, buffer_size) usually preceded > outputs (byte stream halves). Maybe that's just my verilog showing. If so, the folks who wrote the style guide also knew verilog :-}. Done. http://codereview.chromium.org/10244001/diff/17008/content/browser/download/b... File content/browser/download/byte_stream_unittest.cc (right): http://codereview.chromium.org/10244001/diff/17008/content/browser/download/b... content/browser/download/byte_stream_unittest.cc:61: void NullCallback() { On 2012/05/16 15:55:57, benjhayden_chromium wrote: > Would something like this work just as well with less code and insulate between > TEST_Fs? > > void CountCallbacks(int* counter) { ++*counter; } > > TEST_F(...) { > int counter_a = 0; > int counter_b = 0; > int counter_c = 0; > base::Bind(&CountCallbacks, &counter_a); > base::Bind(&CountCallbacks, &counter_b); > base::Bind(&CountCallbacks, &counter_c); > ... > EXPECT_EQ(42, counter_a); > EXPECT_EQ(42, counter_b); > EXPECT_EQ(42, counter_c); > counter_a = counter_b = counter_c = 0; > ... > EXPECT_EQ(42, counter_a); > EXPECT_EQ(42, counter_b); > EXPECT_EQ(42, counter_c); > } D'oh. Good idea. Done. http://codereview.chromium.org/10244001/diff/17008/content/browser/download/b... content/browser/download/byte_stream_unittest.cc:76: // |seed_key| value within a single test. On 2012/05/16 15:55:57, benjhayden_chromium wrote: > Can you use an internal automatic counter instead of magic numbers and fallible > human counters? I'm a little reluctant to do that, as it implies that we're always going to evaluate buffers in the order that they were allocated. Having said that, that assumption is valid for this entire file, and it would make for simpler code, so ok. http://codereview.chromium.org/10244001/diff/17008/content/browser/download/b... content/browser/download/byte_stream_unittest.cc:124: private: On 2012/05/16 19:17:21, benjhayden_chromium wrote: > Blank line between sections? Done. http://codereview.chromium.org/10244001/diff/17008/content/browser/download/b... content/browser/download/byte_stream_unittest.cc:131: TEST_F(ByteStreamTest, PushBack) { On 2012/05/16 19:17:21, benjhayden_chromium wrote: > Would you mind prepending "ByteStream_" to all of the TEST_F case names so that > we see "ByteStream_PushBack" etc. instead of mysteriously "PushBack" on the bot > summary pages? It's annoyingly redundant :-{, but I hear your pain. Done.
http://codereview.chromium.org/10244001/diff/26009/content/browser/download/b... File content/browser/download/byte_stream.cc (right): http://codereview.chromium.org/10244001/diff/26009/content/browser/download/b... content/browser/download/byte_stream.cc:142: void TransferDataMethod( Sounds like you're transferring something called a "data method". TransferDataInternal()? http://codereview.chromium.org/10244001/diff/26009/content/browser/download/b... content/browser/download/byte_stream.cc:359: scoped_ptr<ContentVector> xfer_buffer, Want to spell out "transfer_buffer"? http://codereview.chromium.org/10244001/diff/26009/content/browser/download/b... content/browser/download/byte_stream.cc:376: if (available_contents_size_ == buffer_size && bool was_empty = available_contents_.empty(); ... copy xfer_buffer to available_contents_ if (was_empty && !available_contents_.empty() && !data_available_callback_.is_null()) ...
http://codereview.chromium.org/10244001/diff/26009/content/browser/download/b... File content/browser/download/byte_stream.cc (right): http://codereview.chromium.org/10244001/diff/26009/content/browser/download/b... content/browser/download/byte_stream.cc:142: void TransferDataMethod( On 2012/05/16 21:11:04, benjhayden_chromium wrote: > Sounds like you're transferring something called a "data method". > TransferDataInternal()? Done. http://codereview.chromium.org/10244001/diff/26009/content/browser/download/b... content/browser/download/byte_stream.cc:359: scoped_ptr<ContentVector> xfer_buffer, On 2012/05/16 21:11:04, benjhayden_chromium wrote: > Want to spell out "transfer_buffer"? Done. http://codereview.chromium.org/10244001/diff/26009/content/browser/download/b... content/browser/download/byte_stream.cc:376: if (available_contents_size_ == buffer_size && On 2012/05/16 21:11:04, benjhayden_chromium wrote: > bool was_empty = available_contents_.empty(); > ... copy xfer_buffer to available_contents_ > if (was_empty && !available_contents_.empty() && > !data_available_callback_.is_null()) ... Done.
Uploaded a bug fix.
Avi: Can I have a rubberstamp for the .gypi files? (I don't yet have the LGTM for the base change, but I expect to soon, so trying to get the ducks in a row.)
[Apologies for the duplication. ] Avi, may I have a rubberstamp content OWNER ok for the .gypi files? I don't yet have the LGTM for the base change, but I expect to soon, so I'm getting my ducks in a row.
Probably last round. http://codereview.chromium.org/10244001/diff/34003/content/browser/download/b... File content/browser/download/byte_stream.cc (right): http://codereview.chromium.org/10244001/diff/34003/content/browser/download/b... content/browser/download/byte_stream.cc:49: // happen in the context of their SequencedTaskRunner. Is there any way to DCHECK this? I find that kind of DCHECK to be very helpful when reading the code. http://codereview.chromium.org/10244001/diff/34003/content/browser/download/b... content/browser/download/byte_stream.cc:319: DCHECK_GE(available_contents_size_, *length); Did you mean to delete |available_contents_size_|, or is this DCHECK alone necessary enough to be worth keeping the member variable? http://codereview.chromium.org/10244001/diff/34003/content/browser/download/b... File content/browser/download/byte_stream.h (right): http://codereview.chromium.org/10244001/diff/34003/content/browser/download/b... content/browser/download/byte_stream.h:74: virtual void Close(DownloadInterruptReason status) = 0; Did you want to templatize the error type?
lgtm for gypi
http://codereview.chromium.org/10244001/diff/34003/content/browser/download/b... File content/browser/download/byte_stream.cc (right): http://codereview.chromium.org/10244001/diff/34003/content/browser/download/b... content/browser/download/byte_stream.cc:49: // happen in the context of their SequencedTaskRunner. On 2012/05/18 14:21:06, benjhayden_chromium wrote: > Is there any way to DCHECK this? I find that kind of DCHECK to be very helpful > when reading the code. Agreed. Done. http://codereview.chromium.org/10244001/diff/34003/content/browser/download/b... content/browser/download/byte_stream.cc:319: DCHECK_GE(available_contents_size_, *length); On 2012/05/18 14:21:06, benjhayden_chromium wrote: > Did you mean to delete |available_contents_size_|, or is this DCHECK alone > necessary enough to be worth keeping the member variable? Good point--deleted. http://codereview.chromium.org/10244001/diff/34003/content/browser/download/b... File content/browser/download/byte_stream.h (right): http://codereview.chromium.org/10244001/diff/34003/content/browser/download/b... content/browser/download/byte_stream.h:74: virtual void Close(DownloadInterruptReason status) = 0; On 2012/05/18 14:21:06, benjhayden_chromium wrote: > Did you want to templatize the error type? I've played around a little bit and concluded that a) I do indeed want to do this (but I want Will's perspective first as that's a fairly large change to the interface), but b) it's not trivial, it's not clear what the right .h/.cc balance is, and I'd rather not delay further refactors on it. So if it's ok with you I'd like to delay this change (and a probable concurrent move out of content/) for a different CL.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rdsmith@chromium.org/10244001/39002
Try job failure for 10244001-39002 (retry) on win for step "update" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number... Step "update" is always a major failure. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rdsmith@chromium.org/10244001/40004
Try job failure for 10244001-40004 (retry) on win for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rdsmith@chromium.org/10244001/40004
Try job failure for 10244001-40004 on win for step "update". http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number... Step "update" is always a major failure. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rdsmith@chromium.org/10244001/40004
Try job failure for 10244001-40004 (retry) on win_rel for steps "base_unittests, unit_tests, sync_unit_tests" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rdsmith@chromium.org/10244001/40004
The commit queue went berserk retrying too often for a seemingly flaky test. Builder is win_rel, revision is 138404, job name was 10244001-40004 (retry) (retry) (retry) (previous was lost).
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rdsmith@chromium.org/10244001/40004
Change committed as 138504 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
