Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1165903002:
Multi reader/writer cache/buffer (Closed)
DescriptionMulti reader/writer cache/buffer
MultiBuffer is meant to be integrated into buffered_resource_loader
to provide more efficient cacheing.
Unit tests are in a separate CL:
https://codereview.chromium.org/1420883004
Depends on:
https://codereview.chromium.org/1427433012/
https://codereview.chromium.org/1422523007/
Media cache design doc:
https://docs.google.com/document/d/15q6LTG0iDUe30QcoMtj4XNmKCa_7W_Q2uUIPFsJhS1E/edit
BUG=514719
Committed: https://crrev.com/89d5cdfa35dad6711cc0177e3eaae75f5e08d83a
Cr-Commit-Position: refs/heads/master@{#359627}
Patch Set 1 #Patch Set 2 : rangemap done and tested #Patch Set 3 : lru unit tests #
Total comments: 21
Patch Set 4 : more tests + lots of fixes #Patch Set 5 : writers refactored #Patch Set 6 : major integration done (in separate cl) #Patch Set 7 : missed waiter_index.cc #Patch Set 8 : more tests #
Total comments: 52
Patch Set 9 : more tests, comments adressed, minor refactoring + a little code cleanup #
Total comments: 8
Patch Set 10 : comments addressed #Patch Set 11 : compile fixes #Patch Set 12 : use int64_t more consistently #Patch Set 13 : fix variable initialization ordering #Patch Set 14 : compile fixes #Patch Set 15 : win compile workaround #Patch Set 16 : win compile fix #Patch Set 17 : export symbols #Patch Set 18 : oops #Patch Set 19 : export more symbols #Patch Set 20 : compile fixes #Patch Set 21 : one more compile fix #
Total comments: 27
Patch Set 22 : some comments addressed #Patch Set 23 : refactor #
Total comments: 37
Patch Set 24 : commments addressed #
Total comments: 8
Patch Set 25 : comments addressed + minor cleanup & compile fixes #Patch Set 26 : compile fixes #
Total comments: 4
Patch Set 27 : compile fixes #Patch Set 28 : use intptr_t #
Total comments: 12
Patch Set 29 : split out helper classes + added more comments #Patch Set 30 : split out reader #Patch Set 31 : merged #Patch Set 32 : mac workaround #Patch Set 33 : merged #
Total comments: 26
Patch Set 34 : comments addressed + merged in rangemap -> intervalmap name change #Patch Set 35 : save a few cycles on 32 bit platforms #Patch Set 36 : formatted #
Dependent Patchsets: Messages
Total messages: 66 (14 generated)
hubbe@chromium.org changed reviewers: + dalecurtis@chromium.org
First look at MultiBuffer. The code is not ready to be checked in. It is in fact completely untested so far. Next I'm going to make sure it's properly unit tested. In the mean time I wanted to make sure that MultiBuffer functionally does what we want, so don't worry about reviewing for style or bugs yet, just sanity. :)
dalecurtis@chromium.org changed reviewers: + liberato@chromium.org
+liberato to help with high level design since he's looked through our network level cache.
Can you start a doc with how you intend these components to hook up and the intent of each one? We should have a design doc for all this work that we can share with the net/ team to ensure we're all on the same page. Thanks! It's exciting to see the start of something shiny here :)
On 2015/06/04 00:02:49, DaleCurtis wrote: > Can you start a doc with how you intend these components to hook up and the > intent of each one? We should have a design doc for all this work that we can > share with the net/ team to ensure we're all on the same page. > > Thanks! It's exciting to see the start of something shiny here :) I did start a doc, but found it hard to describe the functionality in a non-confusing way, so I thought it would be more clearly described as code. Anyways, the doc is here: https://docs.google.com/a/google.com/document/d/1hDkMV-bCTZm-Sf6pQj-v5PRqdSSl...
On 2015/06/04 19:40:59, hubbe wrote: > On 2015/06/04 00:02:49, DaleCurtis wrote: > > Can you start a doc with how you intend these components to hook up and the > > intent of each one? We should have a design doc for all this work that we can > > share with the net/ team to ensure we're all on the same page. > > > > Thanks! It's exciting to see the start of something shiny here :) > > I did start a doc, but found it hard to describe the functionality in > a non-confusing way, so I thought it would be more clearly described as code. > > Anyways, the doc is here: > > https://docs.google.com/a/google.com/document/d/1hDkMV-bCTZm-Sf6pQj-v5PRqdSSl... That's a great doc, can you elaborate on expiration of data as we reach cache size limits?
On 2015/06/04 21:39:34, DaleCurtis wrote: > On 2015/06/04 19:40:59, hubbe wrote: > > On 2015/06/04 00:02:49, DaleCurtis wrote: > > > Can you start a doc with how you intend these components to hook up and the > > > intent of each one? We should have a design doc for all this work that we > can > > > share with the net/ team to ensure we're all on the same page. > > > > > > Thanks! It's exciting to see the start of something shiny here :) > > > > I did start a doc, but found it hard to describe the functionality in > > a non-confusing way, so I thought it would be more clearly described as code. > > > > Anyways, the doc is here: > > > > > https://docs.google.com/a/google.com/document/d/1hDkMV-bCTZm-Sf6pQj-v5PRqdSSl... > > That's a great doc, can you elaborate on expiration of data as we reach cache > size limits? Sure, done.
really sorry for the delay. missed that i had an incoming review. will check my chromium forward settings, since i seem to have caused them to be wrong. -fl https://codereview.chromium.org/1165903002/diff/40001/media/blink/multibuffer.cc File media/blink/multibuffer.cc (right): https://codereview.chromium.org/1165903002/diff/40001/media/blink/multibuffer... media/blink/multibuffer.cc:167: multibuffer_->AddWriter(pos_, this); it seems like there are many calls to AddWriter() and only one call to RemoveWriter() -- is this okay? https://codereview.chromium.org/1165903002/diff/40001/media/blink/multibuffer... media/blink/multibuffer.cc:177: delete this; who calls Write()? how would they know that it chose to delete itself? i'm a little unclear on how you're picturing MultiBufferWriter being used. it might be a good idea to have a thing that "copies a range into the cache", and manages interacting with net/ to do so. it wouldn't do any of the cache management logic (want now / want soon). then, the job of picking the ranges can be moved elsewhere. https://codereview.chromium.org/1165903002/diff/40001/media/blink/multibuffer... media/blink/multibuffer.cc:241: block_ciel(std::min(pos_ + max_buffer_forward_, end_)), nit: s/ciel/ceil/ https://codereview.chromium.org/1165903002/diff/40001/media/blink/multibuffer... media/blink/multibuffer.cc:275: ++i; i'm not sure that this doesn't sometimes walk right out of the consecutive region of the map, possibly off the end of the map. https://codereview.chromium.org/1165903002/diff/40001/media/blink/multibuffer.h File media/blink/multibuffer.h (right): https://codereview.chromium.org/1165903002/diff/40001/media/blink/multibuffer... media/blink/multibuffer.h:28: class MultiBuffer { the name "MultiBuffer" is a little confusing. maybe something that contains the word "cache", else it sounds like it should be a thing with getStart(), getLength(), and little else. https://codereview.chromium.org/1165903002/diff/40001/media/blink/multibuffer... media/blink/multibuffer.h:38: void AddWriter(int32_t pos, Waiter* writer); are all the positions in byte offsets or block offsets? also not sure that int32 is the right type for it. might be a good place for a typedef to make its interpretation clearer. https://codereview.chromium.org/1165903002/diff/40001/media/blink/multibuffer... media/blink/multibuffer.h:51: void DeferredWaitFor(int32_t pos, Waiter* reader); why do we need WaitFor vs DeferredWaitFor? shouldn't the preload / max / min specs cover this? https://codereview.chromium.org/1165903002/diff/40001/media/blink/multibuffer... media/blink/multibuffer.h:89: virtual void StartWriter(int32_t pos) = 0; MultiBuffer::StartWriter => new Writer => MultiBuffer::AddWriter(). maybe multibuffer is doing too much -- it seems like it's a complicated cache data structure, and also trying to control who's accessing it. maybe this can be split into two parts: the cache data structure (add / contains / remove) and the cache manager (start writers / prune). plus, we might want to experiment with different eviction or loading strategies, for example. not sure where 'pin' goes. https://codereview.chromium.org/1165903002/diff/40001/media/blink/multibuffer... media/blink/multibuffer.h:129: class MultiBufferWriter : public Waiter { this might be better off in another .h
I've added some more tests, cleaned up a bunch of things that didn't work (because I didn't have tests yet) and addressed some/most of the comments. I'm going to go work on the design doc now, feel free to take another look at the code, or wait until the design doc is more complete. https://codereview.chromium.org/1165903002/diff/40001/media/blink/multibuffer.cc File media/blink/multibuffer.cc (right): https://codereview.chromium.org/1165903002/diff/40001/media/blink/multibuffer... media/blink/multibuffer.cc:167: multibuffer_->AddWriter(pos_, this); On 2015/06/09 15:00:53, liberato wrote: > it seems like there are many calls to AddWriter() and only one call to > RemoveWriter() -- is this okay? They seem evenly matched to me... https://codereview.chromium.org/1165903002/diff/40001/media/blink/multibuffer... media/blink/multibuffer.cc:177: delete this; On 2015/06/09 15:00:53, liberato wrote: > who calls Write()? how would they know that it chose to delete itself? > > i'm a little unclear on how you're picturing MultiBufferWriter being used. > > it might be a good idea to have a thing that "copies a range into the cache", > and manages interacting with net/ to do so. it wouldn't do any of the cache > management logic (want now / want soon). then, the job of picking the ranges > can be moved elsewhere. Updated comments in header file, hope it explains. https://codereview.chromium.org/1165903002/diff/40001/media/blink/multibuffer... media/blink/multibuffer.cc:241: block_ciel(std::min(pos_ + max_buffer_forward_, end_)), On 2015/06/09 15:00:53, liberato wrote: > nit: s/ciel/ceil/ Done. https://codereview.chromium.org/1165903002/diff/40001/media/blink/multibuffer... media/blink/multibuffer.cc:275: ++i; On 2015/06/09 15:00:53, liberato wrote: > i'm not sure that this doesn't sometimes walk right out of the consecutive > region of the map, possibly off the end of the map. Hmm, why are you not sure? What can I do to make you sure? https://codereview.chromium.org/1165903002/diff/40001/media/blink/multibuffer.h File media/blink/multibuffer.h (right): https://codereview.chromium.org/1165903002/diff/40001/media/blink/multibuffer... media/blink/multibuffer.h:28: class MultiBuffer { On 2015/06/09 15:00:54, liberato wrote: > the name "MultiBuffer" is a little confusing. maybe something that contains the > word "cache", else it sounds like it should be a thing with getStart(), > getLength(), and little else. It's meant to replace multiple preloading buffers. I couldn't come up with a GOOD name, so I picked a short unique one. Perhaps StreamingBlockCache would be better though? https://codereview.chromium.org/1165903002/diff/40001/media/blink/multibuffer... media/blink/multibuffer.h:38: void AddWriter(int32_t pos, Waiter* writer); On 2015/06/09 15:00:53, liberato wrote: > are all the positions in byte offsets or block offsets? Yes. (the new typedef makes that clear) > > also not sure that int32 is the right type for it. might be a good place for a > typedef to make its interpretation clearer. Done. When you say "I'm not sure...", can you please tell me why you're not sure so that I can address it better? (With a block size of 32kb, this class can handle 64Tb files....) https://codereview.chromium.org/1165903002/diff/40001/media/blink/multibuffer... media/blink/multibuffer.h:51: void DeferredWaitFor(int32_t pos, Waiter* reader); On 2015/06/09 15:00:53, liberato wrote: > why do we need WaitFor vs DeferredWaitFor? shouldn't the preload / max / min > specs cover this? This class doesn't know about any preload specs. https://codereview.chromium.org/1165903002/diff/40001/media/blink/multibuffer... media/blink/multibuffer.h:89: virtual void StartWriter(int32_t pos) = 0; On 2015/06/09 15:00:53, liberato wrote: > MultiBuffer::StartWriter => new Writer => MultiBuffer::AddWriter(). > > maybe multibuffer is doing too much -- it seems like it's a complicated cache > data structure, and also trying to control who's accessing it. > > maybe this can be split into two parts: the cache data structure (add / contains > / remove) and the cache manager (start writers / prune). plus, we might want to > experiment with different eviction or loading strategies, for example. not sure > where 'pin' goes. I could break out the cache itself into something like "PinnableBlockCache" which would contain pinned_, lru_, data_ and PinRange(). That doesn't provide a whole lot of simplification, and makes it harder to explain why PinRange traverses the cache backwards. I'm not sure what you mean by "control who's access it". The idea was to create a cache where consumers simply ask for blocks and the cache decides how to fill those requests by instantiating streaming writers. However, I wanted to abstract out the writers themselves for testing and possible re-use. https://codereview.chromium.org/1165903002/diff/40001/media/blink/multibuffer... media/blink/multibuffer.h:129: class MultiBufferWriter : public Waiter { On 2015/06/09 15:00:54, liberato wrote: > this might be better off in another .h They really are meant to be used as a set. Originally this was an internal class in MultiBuffer, but I moved it out because it seemed easier to read that way. However, I'll move it if you feel strongly about it.
thanks for the clarifications. i'll hang out and wait for the design doc updates before diving in. thanks -fl https://codereview.chromium.org/1165903002/diff/40001/media/blink/multibuffer.cc File media/blink/multibuffer.cc (right): https://codereview.chromium.org/1165903002/diff/40001/media/blink/multibuffer... media/blink/multibuffer.cc:167: multibuffer_->AddWriter(pos_, this); On 2015/06/09 21:23:48, hubbe wrote: > On 2015/06/09 15:00:53, liberato wrote: > > it seems like there are many calls to AddWriter() and only one call to > > RemoveWriter() -- is this okay? > > They seem evenly matched to me... Indeed, you're right. i missed the first line of the function! https://codereview.chromium.org/1165903002/diff/40001/media/blink/multibuffer... media/blink/multibuffer.cc:275: ++i; On 2015/06/09 21:23:48, hubbe wrote: > On 2015/06/09 15:00:53, liberato wrote: > > i'm not sure that this doesn't sometimes walk right out of the consecutive > > region of the map, possibly off the end of the map. > > Hmm, why are you not sure? > What can I do to make you sure? i'm not sure. :) the available() test is supposed to make this safe, i think, but it's pretty intricate to see if preload_pos_ is always up to date. if it's not, for whatever reason, then kaboom and/or silently grab noncontiguous data from the cache. Some sanity checks would probably go a long way. or, one might not depend on preload_pos_ at all here, and just check what's available. if we're not asking for many blocks, it's fine.
Design doc updated... https://codereview.chromium.org/1165903002/diff/40001/media/blink/multibuffer.cc File media/blink/multibuffer.cc (right): https://codereview.chromium.org/1165903002/diff/40001/media/blink/multibuffer... media/blink/multibuffer.cc:275: ++i; On 2015/06/09 21:50:47, liberato wrote: > On 2015/06/09 21:23:48, hubbe wrote: > > On 2015/06/09 15:00:53, liberato wrote: > > > i'm not sure that this doesn't sometimes walk right out of the consecutive > > > region of the map, possibly off the end of the map. > > > > Hmm, why are you not sure? > > What can I do to make you sure? > > i'm not sure. :) > > the available() test is supposed to make this safe, i think, but it's pretty > intricate to see if preload_pos_ is always up to date. if it's not, for > whatever reason, then kaboom and/or silently grab noncontiguous data from the > cache. > > Some sanity checks would probably go a long way. or, one might not depend on > preload_pos_ at all here, and just check what's available. if we're not asking > for many blocks, it's fine. What you're describing should never happen because: block(pos_) .. preload_pos_ is pinned we've iterated over all the blocks already and made sure they were there (in IncrementPreloadPos) The sanity check you're talking about is the DCHECK_EQ() inside the loop. I suppose I could change the DCHECK_EQ() to a CHECK_EQ() to make SURE it never happens :)
Refactored writers using inversion-of-control, which puts all the writer logic, construction and destruction in one place.
On 2015/06/12 22:53:36, hubbe wrote: > Refactored writers using inversion-of-control, which puts all the writer logic, > construction and destruction in one place. PTAL This should be more or less the final form of this code. I have another CL that does all the integration to use the code.
mostly cosmetic stuff. overall looks nice. https://codereview.chromium.org/1165903002/diff/140001/media/blink/multibuffe... File media/blink/multibuffer.cc (right): https://codereview.chromium.org/1165903002/diff/140001/media/blink/multibuffe... media/blink/multibuffer.cc:27: block_num_(-1) { why not MultiBufferBlockId::min()? https://codereview.chromium.org/1165903002/diff/140001/media/blink/multibuffe... media/blink/multibuffer.cc:45: static MultiBuffer::BlockId ClosestPreviousEntry( previous or equal? https://codereview.chromium.org/1165903002/diff/140001/media/blink/multibuffe... media/blink/multibuffer.cc:197: if (closest_writer == MultiBufferBlockId(kUnknownUrlId, -1)) might want to have an explicit unknown block id for this, here and elsewhere. https://codereview.chromium.org/1165903002/diff/140001/media/blink/multibuffe... media/blink/multibuffer.cc:261: if (i.range_begin() == freed_iter.range_begin()) { perhaps comment to the effect of "notify that a range was truncated" https://codereview.chromium.org/1165903002/diff/140001/media/blink/multibuffe... media/blink/multibuffer.cc:463: for (auto i = tmp.first_range(); i != tmp.last_range(); ++i) { nice, though this code appears twice and should probably be its own function AdjustPinning(outgoing, incoming). https://codereview.chromium.org/1165903002/diff/140001/media/blink/multibuffe... media/blink/multibuffer.cc:559: 1); -1? https://codereview.chromium.org/1165903002/diff/140001/media/blink/multibuffe... media/blink/multibuffer.cc:567: multibuffer_->CleanupWriters(old_preload_pos); would this be better in the multibuffer, maybe in response to StopWaitFor()? seems like an implementation detail of the cache that the readers shouldn't care about. https://codereview.chromium.org/1165903002/diff/140001/media/blink/multibuffer.h File media/blink/multibuffer.h (right): https://codereview.chromium.org/1165903002/diff/140001/media/blink/multibuffe... media/blink/multibuffer.h:31: // refcounted UrlData pointer and a block ID. if !url_id has special meaning, might want to mention it here. https://codereview.chromium.org/1165903002/diff/140001/media/blink/multibuffe... media/blink/multibuffer.h:38: ~MultiBufferBlockId(); why declare this? https://codereview.chromium.org/1165903002/diff/140001/media/blink/multibuffe... media/blink/multibuffer.h:95: // minimum as {0, max_int64}. This helper function lets maximum https://codereview.chromium.org/1165903002/diff/140001/media/blink/multibuffe... media/blink/multibuffer.h:124: numeric_limits<media::MultiBufferUrlId>::max(), should this be 0 rather than ::max? (probably also in min() too). if not, then i don't see what the special case in url_id_cmp() is for. https://codereview.chromium.org/1165903002/diff/140001/media/blink/multibuffe... media/blink/multibuffer.h:138: virtual ~MultiBufferDataProvider(); why not {}? https://codereview.chromium.org/1165903002/diff/140001/media/blink/multibuffe... media/blink/multibuffer.h:155: virtual void SetAvailableCallback(base::Closure cb) = 0; const base::Closure& ? https://codereview.chromium.org/1165903002/diff/140001/media/blink/multibuffe... media/blink/multibuffer.h:157: // Ask the data provider to stop giving us data. does this mean that it may no longer call the available callback, or just best effort? https://codereview.chromium.org/1165903002/diff/140001/media/blink/multibuffe... media/blink/multibuffer.h:161: class Reader { comment here would help. also, MultiBufferReader? Reader is pretty generic. alternatively, move this into MultiBuffer. https://codereview.chromium.org/1165903002/diff/140001/media/blink/multibuffe... media/blink/multibuffer.h:163: Reader() {} why? https://codereview.chromium.org/1165903002/diff/140001/media/blink/multibuffe... media/blink/multibuffer.h:172: virtual void UpdateURLId(MultiBufferUrlId old_url_id, might want to include what a typical reader action would be. i suspect it's to call stopwaitfor(old) and then waitfor(new). https://codereview.chromium.org/1165903002/diff/140001/media/blink/multibuffe... media/blink/multibuffer.h:184: class ReaderIndex { please comment. Also, MultiBufferReaderIndex? https://codereview.chromium.org/1165903002/diff/140001/media/blink/multibuffe... media/blink/multibuffer.h:226: // Users should inherit this class and implement StartWriter(). s/Start/Create/ also, this might be better if a MultiBufferWriterFactory was provided, rather than inheriting. https://codereview.chromium.org/1165903002/diff/140001/media/blink/multibuffe... media/blink/multibuffer.h:243: // with the current level of buffering. "ok with the current level of buffering" isn't very clear. maybe something like "provides a hint to pause rather than destroy data providers when they've filled all outstanding WaitFor requests.". https://codereview.chromium.org/1165903002/diff/140001/media/blink/multibuffe... media/blink/multibuffer.h:256: // blocks before or after |pos| changes, we'll call NotifyAvailableRange on this isn't clear. all blocks are either before, after, or equal to |pos|. https://codereview.chromium.org/1165903002/diff/140001/media/blink/multibuffe... media/blink/multibuffer.h:353: class MultiBufferReader : public Reader { this is a confusing name. StreamingMultiBufferReaderImpl? also, this should get its own header. https://codereview.chromium.org/1165903002/diff/140001/media/blink/multibuffe... media/blink/multibuffer.h:357: // origin checks will become insecure. please document processs_callback args. https://codereview.chromium.org/1165903002/diff/140001/media/blink/url_index.h File media/blink/url_index.h (right): https://codereview.chromium.org/1165903002/diff/140001/media/blink/url_index.... media/blink/url_index.h:96: // It keeps a weak reference to UrlData that it has returned. i don't see any weak references. https://codereview.chromium.org/1165903002/diff/140001/media/blink/waiter_ind... File media/blink/waiter_index.h (right): https://codereview.chromium.org/1165903002/diff/140001/media/blink/waiter_ind... media/blink/waiter_index.h:1: // Copyright 2013 The Chromium Authors. All rights reserved. 2015 https://codereview.chromium.org/1165903002/diff/140001/media/blink/waiter_ind... media/blink/waiter_index.h:24: class WaiterIndex { why is WaiterIndex included in this CL?
PTAL https://codereview.chromium.org/1165903002/diff/140001/media/blink/multibuffe... File media/blink/multibuffer.cc (right): https://codereview.chromium.org/1165903002/diff/140001/media/blink/multibuffe... media/blink/multibuffer.cc:27: block_num_(-1) { On 2015/10/09 17:36:09, liberato wrote: > why not MultiBufferBlockId::min()? I changed it to 0 actually. Also, I added some more comment to explain what MultiBufferBlockIds with url_id_ = null are used for. https://codereview.chromium.org/1165903002/diff/140001/media/blink/multibuffe... media/blink/multibuffer.cc:45: static MultiBuffer::BlockId ClosestPreviousEntry( On 2015/10/09 17:36:09, liberato wrote: > previous or equal? Comment added. https://codereview.chromium.org/1165903002/diff/140001/media/blink/multibuffe... media/blink/multibuffer.cc:197: if (closest_writer == MultiBufferBlockId(kUnknownUrlId, -1)) On 2015/10/09 17:36:08, liberato wrote: > might want to have an explicit unknown block id for this, here and elsewhere. Changed it to MultiBufferBlockId() https://codereview.chromium.org/1165903002/diff/140001/media/blink/multibuffe... media/blink/multibuffer.cc:261: if (i.range_begin() == freed_iter.range_begin()) { On 2015/10/09 17:36:09, liberato wrote: > perhaps comment to the effect of "notify that a range was truncated" Done. https://codereview.chromium.org/1165903002/diff/140001/media/blink/multibuffe... media/blink/multibuffer.cc:463: for (auto i = tmp.first_range(); i != tmp.last_range(); ++i) { On 2015/10/09 17:36:09, liberato wrote: > nice, though this code appears twice and should probably be its own function > AdjustPinning(outgoing, incoming). Made a MultiBuffer::PinRanges() convenience function instead. https://codereview.chromium.org/1165903002/diff/140001/media/blink/multibuffe... media/blink/multibuffer.cc:559: 1); On 2015/10/09 17:36:09, liberato wrote: > -1? Good catch, guess I need more tests. https://codereview.chromium.org/1165903002/diff/140001/media/blink/multibuffe... media/blink/multibuffer.cc:567: multibuffer_->CleanupWriters(old_preload_pos); On 2015/10/09 17:36:09, liberato wrote: > would this be better in the multibuffer, maybe in response to StopWaitFor()? > seems like an implementation detail of the cache that the readers shouldn't care > about. Unfortunately it's not that easy, because sometimes we like to stop waiting for something, then start waiting for something nearby, *without* cleaning up the writers. Originally I didn't call CleanupWriters at all, I just let the writers continue their merry way, and then cleaned them up when they tried to add data to the cache that we didn't need. However, that is probably not good since the http cache will bypass the disk/memory cache for one request if two requests are going at the same time. Adding this to the documentation for CleanupWriters now... Hmm, as I think about this, I'm realizing that StopWaitfor() could possibly do the trick if I change the coding pattern in MultiBufferReader so that WaitFor()/DeferredWaitFor() is called *before* StopWaitFor(), but that's not the most intuitive thing in the world either... https://codereview.chromium.org/1165903002/diff/140001/media/blink/multibuffer.h File media/blink/multibuffer.h (right): https://codereview.chromium.org/1165903002/diff/140001/media/blink/multibuffe... media/blink/multibuffer.h:31: // refcounted UrlData pointer and a block ID. On 2015/10/09 17:36:09, liberato wrote: > if !url_id has special meaning, might want to mention it here. Done. https://codereview.chromium.org/1165903002/diff/140001/media/blink/multibuffe... media/blink/multibuffer.h:38: ~MultiBufferBlockId(); On 2015/10/09 17:36:10, liberato wrote: > why declare this? Because chrome-style demands it? https://codereview.chromium.org/1165903002/diff/140001/media/blink/multibuffe... media/blink/multibuffer.h:95: // minimum as {0, max_int64}. This helper function lets On 2015/10/09 17:36:09, liberato wrote: > maximum Done. https://codereview.chromium.org/1165903002/diff/140001/media/blink/multibuffe... media/blink/multibuffer.h:124: numeric_limits<media::MultiBufferUrlId>::max(), On 2015/10/09 17:36:09, liberato wrote: > should this be 0 rather than ::max? (probably also in min() too). if not, then > i don't see what the special case in url_id_cmp() is for. Hmm, not sure what is best here. Originally, MultiBufferUrlId was an integer, but now it's a scoped_refptr<> Unfortunately std::numeric_limits<T>::min() and std::numeric_limits<T>::max() default to returning T(), which is confusing and error-inducing, but I kept this code the way it was in case I wanted to go back to an integer. The reason min and max are both represented using kUnknownUrlID is because scoped_refptr<UrlData>((UrlData*)-1); will crash, so having a real "largest value" is not possible. https://codereview.chromium.org/1165903002/diff/140001/media/blink/multibuffe... media/blink/multibuffer.h:138: virtual ~MultiBufferDataProvider(); On 2015/10/09 17:36:09, liberato wrote: > why not {}? Done. https://codereview.chromium.org/1165903002/diff/140001/media/blink/multibuffe... media/blink/multibuffer.h:155: virtual void SetAvailableCallback(base::Closure cb) = 0; On 2015/10/09 17:36:09, liberato wrote: > const base::Closure& ? Done. https://codereview.chromium.org/1165903002/diff/140001/media/blink/multibuffe... media/blink/multibuffer.h:157: // Ask the data provider to stop giving us data. On 2015/10/09 17:36:09, liberato wrote: > does this mean that it may no longer call the available callback, or just best > effort? Clarified. https://codereview.chromium.org/1165903002/diff/140001/media/blink/multibuffe... media/blink/multibuffer.h:161: class Reader { On 2015/10/09 17:36:10, liberato wrote: > comment here would help. also, MultiBufferReader? Reader is pretty generic. > alternatively, move this into MultiBuffer. Comment added, moved into MultiBuffer. https://codereview.chromium.org/1165903002/diff/140001/media/blink/multibuffe... media/blink/multibuffer.h:163: Reader() {} On 2015/10/09 17:36:09, liberato wrote: > why? because: ../../media/blink/multibuffer.cc:398:20: error: constructor for 'media::MultiBufferReader' must explicitly initialize the base class 'MultiBuffer::Reader' which does not have a default constructor https://codereview.chromium.org/1165903002/diff/140001/media/blink/multibuffe... media/blink/multibuffer.h:172: virtual void UpdateURLId(MultiBufferUrlId old_url_id, On 2015/10/09 17:36:09, liberato wrote: > might want to include what a typical reader action would be. i suspect it's to > call stopwaitfor(old) and then waitfor(new). Done. https://codereview.chromium.org/1165903002/diff/140001/media/blink/multibuffe... media/blink/multibuffer.h:184: class ReaderIndex { On 2015/10/09 17:36:09, liberato wrote: > please comment. Also, MultiBufferReaderIndex? Gone. https://codereview.chromium.org/1165903002/diff/140001/media/blink/multibuffe... media/blink/multibuffer.h:226: // Users should inherit this class and implement StartWriter(). On 2015/10/09 17:36:10, liberato wrote: > s/Start/Create/ Fixed. > > also, this might be better if a MultiBufferWriterFactory was provided, rather > than inheriting. Why? https://codereview.chromium.org/1165903002/diff/140001/media/blink/multibuffe... media/blink/multibuffer.h:243: // with the current level of buffering. On 2015/10/09 17:36:09, liberato wrote: > "ok with the current level of buffering" isn't very clear. maybe something like > "provides a hint to pause rather than destroy data providers when they've filled > all outstanding WaitFor requests.". DeferredWaitFor is gone. https://codereview.chromium.org/1165903002/diff/140001/media/blink/multibuffe... media/blink/multibuffer.h:256: // blocks before or after |pos| changes, we'll call NotifyAvailableRange on On 2015/10/09 17:36:10, liberato wrote: > this isn't clear. all blocks are either before, after, or equal to |pos|. Updated comment, better? https://codereview.chromium.org/1165903002/diff/140001/media/blink/multibuffe... media/blink/multibuffer.h:353: class MultiBufferReader : public Reader { On 2015/10/09 17:36:10, liberato wrote: > this is a confusing name. StreamingMultiBufferReaderImpl? > > also, this should get its own header. I like the shorter name, but I moved it to a separate file. https://codereview.chromium.org/1165903002/diff/140001/media/blink/multibuffe... media/blink/multibuffer.h:357: // origin checks will become insecure. On 2015/10/09 17:36:10, liberato wrote: > please document processs_callback args. Done. https://codereview.chromium.org/1165903002/diff/140001/media/blink/url_index.h File media/blink/url_index.h (right): https://codereview.chromium.org/1165903002/diff/140001/media/blink/url_index.... media/blink/url_index.h:96: // It keeps a weak reference to UrlData that it has returned. On 2015/10/09 17:36:10, liberato wrote: > i don't see any weak references. Discussed offline. https://codereview.chromium.org/1165903002/diff/140001/media/blink/waiter_ind... File media/blink/waiter_index.h (right): https://codereview.chromium.org/1165903002/diff/140001/media/blink/waiter_ind... media/blink/waiter_index.h:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2015/10/09 17:36:10, liberato wrote: > 2015 Gone. https://codereview.chromium.org/1165903002/diff/140001/media/blink/waiter_ind... media/blink/waiter_index.h:24: class WaiterIndex { On 2015/10/09 17:36:10, liberato wrote: > why is WaiterIndex included in this CL? Gone.
lgtm. nice! https://codereview.chromium.org/1165903002/diff/160001/media/blink/multibuffe... File media/blink/multibuffer_reader.cc (right): https://codereview.chromium.org/1165903002/diff/160001/media/blink/multibuffe... media/blink/multibuffer_reader.cc:21: preload_low_(), (0)? https://codereview.chromium.org/1165903002/diff/160001/media/blink/multibuffe... media/blink/multibuffer_reader.cc:141: IncrementPreloadPos(); check return? https://codereview.chromium.org/1165903002/diff/160001/media/blink/multibuffe... media/blink/multibuffer_reader.cc:201: return false; super nitpicky: "return false else fall through to return true" is kinda weird. maybe move |still_alive| outside the outer if(), and have just one return at the end of the function? https://codereview.chromium.org/1165903002/diff/160001/media/blink/multibuffe... File media/blink/multibuffer_reader.h (right): https://codereview.chromium.org/1165903002/diff/160001/media/blink/multibuffe... media/blink/multibuffer_reader.h:72: // Adjusts max_buffer_forward and max_buffer_backsard. one will ask "what is max_buffer_*". might as well mention it here rather than making them find the comment for |max_buffer_forward_|. also, s/sard/ward/
Lots of revisions to make it compile. Going to check in once it builds everywhere. https://codereview.chromium.org/1165903002/diff/160001/media/blink/multibuffe... File media/blink/multibuffer_reader.cc (right): https://codereview.chromium.org/1165903002/diff/160001/media/blink/multibuffe... media/blink/multibuffer_reader.cc:21: preload_low_(), On 2015/10/14 14:51:34, liberato wrote: > (0)? Done. https://codereview.chromium.org/1165903002/diff/160001/media/blink/multibuffe... media/blink/multibuffer_reader.cc:141: IncrementPreloadPos(); On 2015/10/14 14:51:34, liberato wrote: > check return? Done. https://codereview.chromium.org/1165903002/diff/160001/media/blink/multibuffe... media/blink/multibuffer_reader.cc:201: return false; On 2015/10/14 14:51:34, liberato wrote: > super nitpicky: "return false else fall through to return true" is kinda weird. > maybe move |still_alive| outside the outer if(), and have just one return at the > end of the function? Done. https://codereview.chromium.org/1165903002/diff/160001/media/blink/multibuffe... File media/blink/multibuffer_reader.h (right): https://codereview.chromium.org/1165903002/diff/160001/media/blink/multibuffe... media/blink/multibuffer_reader.h:72: // Adjusts max_buffer_forward and max_buffer_backsard. On 2015/10/14 14:51:34, liberato wrote: > one will ask "what is max_buffer_*". might as well mention it here rather than > making them find the comment for |max_buffer_forward_|. > > also, s/sard/ward/ Comment updated.
The CQ bit was checked by hubbe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from liberato@chromium.org Link to the patchset: https://codereview.chromium.org/1165903002/#ps400001 (title: "one more compile fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1165903002/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1165903002/400001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
hubbe@chromium.org changed reviewers: + ddorwin@chromium.org
Need OWNERS approval.
ddorwin@chromium.org changed reviewers: - ddorwin@chromium.org
On 2015/10/16 20:23:20, hubbe wrote: > Need OWNERS approval. I don't have any experience with this project. Let's wait until Dale returns Monday.
Is your design doc for this up to date? If so, you should share with chromium-dev, or at the least with the flywheel team. As mentioned a couple times, there are too many random tests in this code. It's okay for a couple explicit random tests which will print enough information to reconstruct failures on any platform, however it's not okay to use implicitly random tests as a proxy for coverage -- as you get exactly what you expect, random coverage :) Please convert most of the tests to be deterministic and only leave a few explicitly random tests if you feel that's adding strong value. https://codereview.chromium.org/1165903002/diff/400001/media/blink/buffered_r... File media/blink/buffered_resource_loader.cc (right): https://codereview.chromium.org/1165903002/diff/400001/media/blink/buffered_r... media/blink/buffered_resource_loader.cc:230: DVLOG(2) << "READ " << read_size << " @ " << position; Necessary? https://codereview.chromium.org/1165903002/diff/400001/media/blink/lru_unitte... File media/blink/lru_unittest.cc (right): https://codereview.chromium.org/1165903002/diff/400001/media/blink/lru_unitte... media/blink/lru_unittest.cc:129: int value = rand() % kTestSize; Rand tests are generally discouraged: https://code.google.com/p/chromium/issues/detail?id=440255 https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/fTK4QvZ851w... I'd prefer not to use them. Even using a seed will tie this test to a specific platform, which hurts reproduction of failures. If you insist, you need to set/print the enough information on failure to repeat the test locally. https://codereview.chromium.org/1165903002/diff/400001/media/blink/multibuffe... File media/blink/multibuffer.cc (right): https://codereview.chromium.org/1165903002/diff/400001/media/blink/multibuffe... media/blink/multibuffer.cc:153: } else { Don't use else w/ return. Ternary? https://codereview.chromium.org/1165903002/diff/400001/media/blink/multibuffe... media/blink/multibuffer.cc:332: const MultiBufferUrlData& new_url_data) { Alignment? https://codereview.chromium.org/1165903002/diff/400001/media/blink/multibuffe... media/blink/multibuffer.cc:350: const BlockId& from, const BlockId& to, int32_t howmuch) { how_much https://codereview.chromium.org/1165903002/diff/400001/media/blink/multibuffer.h File media/blink/multibuffer.h (right): https://codereview.chromium.org/1165903002/diff/400001/media/blink/multibuffe... media/blink/multibuffer.h:26: const int kMaxFreesPerAdd = 10; Needs comment. https://codereview.chromium.org/1165903002/diff/400001/media/blink/multibuffe... media/blink/multibuffer.h:69: bool operator<(const MultiBufferBlockId& other) const { Chrome style typically frowns on using operator overloading. Is there a way to reuse a std class without these overrides? Do you need all the properties exposed by these overloads? Could you condense to a single type via hashing? Overloaded operators make it hard to reason about data structures without first spending some time understanding their axioms. https://codereview.chromium.org/1165903002/diff/400001/media/blink/multibuffe... media/blink/multibuffer.h:141: namespace std { Forbidden via style guide. http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Namespaces https://codereview.chromium.org/1165903002/diff/400001/media/blink/multibuffe... File media/blink/multibuffer_reader.cc (right): https://codereview.chromium.org/1165903002/diff/400001/media/blink/multibuffe... media/blink/multibuffer_reader.cc:199: clear_on_delete_ = &still_alive; This is really messy, can you remove this in favor weakptrs? https://codereview.chromium.org/1165903002/diff/400001/media/blink/multibuffe... File media/blink/multibuffer_unittest.cc (right): https://codereview.chromium.org/1165903002/diff/400001/media/blink/multibuffe... media/blink/multibuffer_unittest.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. Overall there's too much use of rand() in this file. It's okay to have a single test or two which explicitly exercise random behavior, but most tests should be straight forward and not have implicit random results. https://codereview.chromium.org/1165903002/diff/400001/media/blink/multibuffe... media/blink/multibuffer_unittest.cc:75: blocks_until_deferred_ = rand() % max_blocks_after_defer_; Again, don't use rand unless you're prepared to debug this on all platforms or print out enough information to reconstruct the test on all platforms. https://codereview.chromium.org/1165903002/diff/400001/media/blink/multibuffe... media/blink/multibuffer_unittest.cc:216: media::writers[rand() % media::writers.size()]->Advance(); Ditto. https://codereview.chromium.org/1165903002/diff/400001/media/blink/rangemap.h File media/blink/rangemap.h (right): https://codereview.chromium.org/1165903002/diff/400001/media/blink/rangemap.h... media/blink/rangemap.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. Is this just a std::map<x, Range> ? https://code.google.com/p/chromium/codesearch#chromium/src/media/base/ranges.h https://codereview.chromium.org/1165903002/diff/400001/media/blink/url_index.h File media/blink/url_index.h (right): https://codereview.chromium.org/1165903002/diff/400001/media/blink/url_index.... media/blink/url_index.h:112: Extra line.
Still working on it, but I have a few questions.... I'm working on adding deterministic tests, but I would prefer to keep my random tests as I think they are adding a fair amount of value. I did some looking in google3, and there seems to be a fair number of random tests in container implementations: https://cs.corp.google.com/#search/&q=file:util/gtl.*test%20acmrandom.h&type=cs Most of them use acmrandom.h though, which currently is not available in chrome. So the question becomes; if there was a random generator which generated the same sequence on all platforms, that would solve the debuggability problems, right? (But it does not address the concern of using random instead of explicit coverage of course.) I should point out that I try to make my random tests in such a way that most or all corner cases are encountered on all platforms, but I have no way of guaranteeing that of course. https://codereview.chromium.org/1165903002/diff/400001/media/blink/buffered_r... File media/blink/buffered_resource_loader.cc (right): https://codereview.chromium.org/1165903002/diff/400001/media/blink/buffered_r... media/blink/buffered_resource_loader.cc:230: DVLOG(2) << "READ " << read_size << " @ " << position; On 2015/10/19 21:45:25, DaleCurtis wrote: > Necessary? Nope, not sure how that got in there. https://codereview.chromium.org/1165903002/diff/400001/media/blink/multibuffe... File media/blink/multibuffer.cc (right): https://codereview.chromium.org/1165903002/diff/400001/media/blink/multibuffe... media/blink/multibuffer.cc:153: } else { On 2015/10/19 21:45:25, DaleCurtis wrote: > Don't use else w/ return. Ternary? I'm not a big fan of terniary. Is this better? https://codereview.chromium.org/1165903002/diff/400001/media/blink/multibuffe... media/blink/multibuffer.cc:332: const MultiBufferUrlData& new_url_data) { On 2015/10/19 21:45:25, DaleCurtis wrote: > Alignment? Done. https://codereview.chromium.org/1165903002/diff/400001/media/blink/multibuffe... media/blink/multibuffer.cc:350: const BlockId& from, const BlockId& to, int32_t howmuch) { On 2015/10/19 21:45:25, DaleCurtis wrote: > how_much Done. https://codereview.chromium.org/1165903002/diff/400001/media/blink/multibuffer.h File media/blink/multibuffer.h (right): https://codereview.chromium.org/1165903002/diff/400001/media/blink/multibuffe... media/blink/multibuffer.h:26: const int kMaxFreesPerAdd = 10; On 2015/10/19 21:45:25, DaleCurtis wrote: > Needs comment. Done. https://codereview.chromium.org/1165903002/diff/400001/media/blink/multibuffe... media/blink/multibuffer.h:69: bool operator<(const MultiBufferBlockId& other) const { On 2015/10/19 21:45:25, DaleCurtis wrote: > Chrome style typically frowns on using operator overloading. Is there a way to > reuse a std class without these overrides? Not that I know of. What I have is similar to a std::pair<MultiBufferUrlData, MultiBufferBlockNum>, but it's a major step down in readability to use .first instead of .url_data(), and I'm not sure how to support a "max value" properly in that case. > Do you need all the properties > exposed by these overloads? I'm not sure if I use all the comparison functions, I could try minimizing that, but I don't think that's your main complaint here, is it? > Could you condense to a single type via hashing? > No, I need something sortable for my algorithms to work. > Overloaded operators make it hard to reason about data structures without first > spending some time understanding their axioms. Hmm, the google3 style guide used to have a phrase like "unless you're implementing something an arithmetic type", guess that's gone now... Chromium doesn't seem to follow the google3 guide very well though, there are no less than 252 implementations of operator< (not counting third_party_ but that's neither here nor there really... I can replace all the operators with .LessThan(), .GreaterThan() etc. if you prefer, but I think it will make the code harder to read unfortunately. https://codereview.chromium.org/1165903002/diff/400001/media/blink/multibuffe... media/blink/multibuffer.h:141: namespace std { On 2015/10/19 21:45:25, DaleCurtis wrote: > Forbidden via style guide. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Namespaces I see a section that say not to forward-declare anything in std::, but it's not entirely clear if that also means not to define anything in std::. I can work around the numeric_limits<> stuff, but the ostream operator << has to be in std:: to work. https://codereview.chromium.org/1165903002/diff/400001/media/blink/multibuffe... File media/blink/multibuffer_reader.cc (right): https://codereview.chromium.org/1165903002/diff/400001/media/blink/multibuffe... media/blink/multibuffer_reader.cc:199: clear_on_delete_ = &still_alive; On 2015/10/19 21:45:25, DaleCurtis wrote: > This is really messy, can you remove this in favor weakptrs? I could have a weak pointer factory, get a pointer to myself and then check if it's still valid when I return, but it doesn't really change anything except add more overhead I think. I tried really hard to think up ways to not have to do something like this, but failed I'm afraid. :( https://codereview.chromium.org/1165903002/diff/400001/media/blink/rangemap.h File media/blink/rangemap.h (right): https://codereview.chromium.org/1165903002/diff/400001/media/blink/rangemap.h... media/blink/rangemap.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2015/10/19 21:45:25, DaleCurtis wrote: > Is this just a std::map<x, Range> ? > > https://code.google.com/p/chromium/codesearch#chromium/src/media/base/ranges.h no Comment added to explain this class. https://codereview.chromium.org/1165903002/diff/400001/media/blink/url_index.h File media/blink/url_index.h (right): https://codereview.chromium.org/1165903002/diff/400001/media/blink/url_index.... media/blink/url_index.h:112: On 2015/10/19 21:45:26, DaleCurtis wrote: > Extra line. Done.
On 2015/10/20 00:31:39, hubbe wrote: > Still working on it, but I have a few questions.... > > I'm working on adding deterministic tests, but I would prefer to keep my random > tests as I think they are adding a fair amount of value. I did some looking in > google3, and there seems to be a fair number of random tests in container > implementations: I'm okay with some explicit random tests so long as you log enough information such that tests can be precisely rerun using text from the error log. I'm not okay with the implicit randomness in regular tests though, please make those tests deterministic or explicitly random with additional deterministic coverage. > > https://cs.corp.google.com/#search/&q=file:util/gtl.*test%20acmrandom.h&type=cs > > Most of them use acmrandom.h though, which currently is not available in chrome. > So the question becomes; if there was a random generator which generated the > same sequence on all platforms, that would solve the debuggability problems, > right? Kind of. You'd still need to log enough information that the random sequence can be precisely reconstructed. I'm not familiar with acmrandom, but presumably it has a printable seed. Even with that, if your code has run time variations of sequencing, a precise reconstruction can be hard/impossible to achieve with random tests. > > (But it does not address the concern of using random instead of explicit > coverage of course.) > > I should point out that I try to make my random tests in such a way that most or > all corner cases are encountered on all platforms, but I have no way of > guaranteeing that of course. > Yes, it'd be hard to guarantee I think. It'd be easier not to have to worry about it :)
https://codereview.chromium.org/1165903002/diff/400001/media/blink/multibuffer.h File media/blink/multibuffer.h (right): https://codereview.chromium.org/1165903002/diff/400001/media/blink/multibuffe... media/blink/multibuffer.h:69: bool operator<(const MultiBufferBlockId& other) const { On 2015/10/20 00:31:39, hubbe wrote: > On 2015/10/19 21:45:25, DaleCurtis wrote: > > Chrome style typically frowns on using operator overloading. Is there a way to > > reuse a std class without these overrides? > > Not that I know of. > What I have is similar to a std::pair<MultiBufferUrlData, MultiBufferBlockNum>, > but it's a major step down in readability to use .first instead of .url_data(), > and I'm not sure how to support a "max value" properly in that case. > > > > Do you need all the properties > > exposed by these overloads? > > I'm not sure if I use all the comparison functions, I could try minimizing that, > but I don't think that's your main complaint here, is it? > > > Could you condense to a single type via hashing? > > > > No, I need something sortable for my algorithms to work. > > > Overloaded operators make it hard to reason about data structures without > first > > spending some time understanding their axioms. > > Hmm, the google3 style guide used to have a phrase like "unless you're > implementing something an arithmetic type", guess that's gone now... > > Chromium doesn't seem to follow the google3 guide very well though, there are no > less than 252 implementations of operator< (not counting third_party_ but that's > neither here nor there really... > > I can replace all the operators with .LessThan(), .GreaterThan() etc. if you > prefer, but I think it will make the code harder to read unfortunately. Reducing to only the operators you need to override is a good idea, removing those you're not using is acceptable. You'll need some of them to use this in std data structures I think; which methods fall outside of that list? https://codereview.chromium.org/1165903002/diff/400001/media/blink/multibuffe... media/blink/multibuffer.h:141: namespace std { On 2015/10/20 00:31:39, hubbe wrote: > On 2015/10/19 21:45:25, DaleCurtis wrote: > > Forbidden via style guide. > > > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Namespaces > > I see a section that say not to forward-declare anything in std::, but it's not > entirely clear if that also means not to define anything in std::. > > I can work around the numeric_limits<> stuff, but the ostream operator << has to > be in std:: to work. The ostream operator is just for printing, right? Can't you do it with a ToString() on the type instead? https://codereview.chromium.org/1165903002/diff/400001/media/blink/multibuffe... File media/blink/multibuffer_reader.cc (right): https://codereview.chromium.org/1165903002/diff/400001/media/blink/multibuffe... media/blink/multibuffer_reader.cc:199: clear_on_delete_ = &still_alive; On 2015/10/20 00:31:39, hubbe wrote: > On 2015/10/19 21:45:25, DaleCurtis wrote: > > This is really messy, can you remove this in favor weakptrs? > > I could have a weak pointer factory, get a pointer to myself and then check if > it's still valid when I return, but it doesn't really change anything except add > more overhead I think. > > I tried really hard to think up ways to not have to do something like this, but > failed I'm afraid. :( Hmm, that usage of WeakPtr wouldn't fly either. Lets discuss on Wednesday so I can better understand what you're trying to do here.
Also, please update the description with some more details and add a test=/bug= stanza
On 2015/10/20 00:49:44, DaleCurtis wrote: > https://codereview.chromium.org/1165903002/diff/400001/media/blink/multibuffer.h > File media/blink/multibuffer.h (right): > > https://codereview.chromium.org/1165903002/diff/400001/media/blink/multibuffe... > media/blink/multibuffer.h:69: bool operator<(const MultiBufferBlockId& other) > const { > On 2015/10/20 00:31:39, hubbe wrote: > > On 2015/10/19 21:45:25, DaleCurtis wrote: > > > Chrome style typically frowns on using operator overloading. Is there a way > to > > > reuse a std class without these overrides? > > > > Not that I know of. > > What I have is similar to a std::pair<MultiBufferUrlData, > MultiBufferBlockNum>, > > but it's a major step down in readability to use .first instead of > .url_data(), > > and I'm not sure how to support a "max value" properly in that case. > > > > > > > Do you need all the properties > > > exposed by these overloads? > > > > I'm not sure if I use all the comparison functions, I could try minimizing > that, > > but I don't think that's your main complaint here, is it? > > > > > Could you condense to a single type via hashing? > > > > > > > No, I need something sortable for my algorithms to work. > > > > > Overloaded operators make it hard to reason about data structures without > > first > > > spending some time understanding their axioms. > > > > Hmm, the google3 style guide used to have a phrase like "unless you're > > implementing something an arithmetic type", guess that's gone now... > > > > Chromium doesn't seem to follow the google3 guide very well though, there are > no > > less than 252 implementations of operator< (not counting third_party_ but > that's > > neither here nor there really... > > > > I can replace all the operators with .LessThan(), .GreaterThan() etc. if you > > prefer, but I think it will make the code harder to read unfortunately. > > Reducing to only the operators you need to override is a good idea, removing > those you're not using is acceptable. You'll need some of them to use this in > std data structures I think; which methods fall outside of that list? > > https://codereview.chromium.org/1165903002/diff/400001/media/blink/multibuffe... > media/blink/multibuffer.h:141: namespace std { > On 2015/10/20 00:31:39, hubbe wrote: > > On 2015/10/19 21:45:25, DaleCurtis wrote: > > > Forbidden via style guide. > > > > > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Namespaces > > > > I see a section that say not to forward-declare anything in std::, but it's > not > > entirely clear if that also means not to define anything in std::. > > > > I can work around the numeric_limits<> stuff, but the ostream operator << has > to > > be in std:: to work. > > The ostream operator is just for printing, right? Can't you do it with a > ToString() on the type instead? > > https://codereview.chromium.org/1165903002/diff/400001/media/blink/multibuffe... > File media/blink/multibuffer_reader.cc (right): > > https://codereview.chromium.org/1165903002/diff/400001/media/blink/multibuffe... > media/blink/multibuffer_reader.cc:199: clear_on_delete_ = &still_alive; > On 2015/10/20 00:31:39, hubbe wrote: > > On 2015/10/19 21:45:25, DaleCurtis wrote: > > > This is really messy, can you remove this in favor weakptrs? > > > > I could have a weak pointer factory, get a pointer to myself and then check if > > it's still valid when I return, but it doesn't really change anything except > add > > more overhead I think. > > > > I tried really hard to think up ways to not have to do something like this, > but > > failed I'm afraid. :( > > Hmm, that usage of WeakPtr wouldn't fly either. Lets discuss on Wednesday so I > can better understand what you're trying to do here. Last night, I thought of a good way to restructure the code which will make things better, simpler AND get rid of most or all operator overloading, stay tuned.
Refactored, cleaned up, sped up and added more tests. Still working on updating the design doc. Please take another look.
Haven't dug too deep, but here are a few things I noticed -- several of these: const& for scoped_refptr and const& vs auto occur in many places. I'm sure I missed some, so please hunt down the rest. Thanks! https://codereview.chromium.org/1165903002/diff/440001/media/blink/lru_unitte... File media/blink/lru_unittest.cc (right): https://codereview.chromium.org/1165903002/diff/440001/media/blink/lru_unitte... media/blink/lru_unittest.cc:77: LRUTest() : rnd_(42) {} Hmm, I don't understand the point of the random test if you are using a sequence which always remains the same. Instead shouldn't this be seeded based on base::RandInt() or something and if any failures occur the seed printed out? https://codereview.chromium.org/1165903002/diff/440001/media/blink/multibuffe... File media/blink/multibuffer.cc (right): https://codereview.chromium.org/1165903002/diff/440001/media/blink/multibuffe... media/blink/multibuffer.cc:96: freed--; How does this work? freed is zero and doesn't seem to be set then you subtract from it? Seems like this overflows "-1" unsigned. https://codereview.chromium.org/1165903002/diff/440001/media/blink/multibuffe... media/blink/multibuffer.cc:98: for (const auto &to_free_pair : to_free) { auto& https://codereview.chromium.org/1165903002/diff/440001/media/blink/multibuffe... media/blink/multibuffer.cc:107: scoped_refptr<GlobalLRU> global_lru) : const& https://codereview.chromium.org/1165903002/diff/440001/media/blink/multibuffe... media/blink/multibuffer.cc:218: for (MultiBufferBlockId to_free : blocks) { const auto&? https://codereview.chromium.org/1165903002/diff/440001/media/blink/multibuffe... media/blink/multibuffer.cc:330: int64_t blocks_added = -data_.size(); -size_t might be problematic? https://codereview.chromium.org/1165903002/diff/440001/media/blink/multibuffe... media/blink/multibuffer.cc:375: for (auto data : other->data_) { const auto& here and the three below. https://codereview.chromium.org/1165903002/diff/440001/media/blink/multibuffe... media/blink/multibuffer.cc:430: for (auto present_block_range = present_.find(transition_end - 1); ditto. https://codereview.chromium.org/1165903002/diff/440001/media/blink/multibuffer.h File media/blink/multibuffer.h (right): https://codereview.chromium.org/1165903002/diff/440001/media/blink/multibuffe... media/blink/multibuffer.h:123: class GlobalLRU: public base::RefCounted<GlobalLRU> { space before : https://codereview.chromium.org/1165903002/diff/440001/media/blink/multibuffe... media/blink/multibuffer.h:306: Remove extra line. https://codereview.chromium.org/1165903002/diff/440001/media/blink/multibuffe... File media/blink/multibuffer_reader.cc (right): https://codereview.chromium.org/1165903002/diff/440001/media/blink/multibuffe... media/blink/multibuffer_reader.cc:137: cb_= base::Closure(); Why? cb_.Reset() ? https://codereview.chromium.org/1165903002/diff/440001/media/blink/multibuffe... media/blink/multibuffer_reader.cc:170: void MultiBufferReader::Call(const base::Closure& cb) const { Should be a way to avoid this by making cb_ a CancelableClosure. Lets you remove the weak factory too I think. https://codereview.chromium.org/1165903002/diff/440001/media/blink/multibuffe... File media/blink/multibuffer_unittest.cc (right): https://codereview.chromium.org/1165903002/diff/440001/media/blink/multibuffe... media/blink/multibuffer_unittest.cc:124: scoped_refptr<media::MultiBuffer::GlobalLRU> lru) : const& for all scoped_refptr passes to avoid accidental atomic ref/unref https://codereview.chromium.org/1165903002/diff/440001/media/blink/multibuffe... media/blink/multibuffer_unittest.cc:492: } Needs non-random tests?
Description was changed from ========== Multi reader/writer cache/buffer MultiBuffer is meant to be integrated into buffered_resource_loader to provide more efficient cacheing. ========== to ========== Multi reader/writer cache/buffer MultiBuffer is meant to be integrated into buffered_resource_loader to provide more efficient cacheing. Design doc: https://docs.google.com/a/google.com/document/d/1hDkMV-bCTZm-Sf6pQj-v5PRqdSSl... ==========
Updated design doc and added it to the CL description.
PTAL https://codereview.chromium.org/1165903002/diff/440001/media/blink/lru_unitte... File media/blink/lru_unittest.cc (right): https://codereview.chromium.org/1165903002/diff/440001/media/blink/lru_unitte... media/blink/lru_unittest.cc:77: LRUTest() : rnd_(42) {} On 2015/10/28 23:06:13, DaleCurtis wrote: > Hmm, I don't understand the point of the random test if you are using a sequence > which always remains the same. Instead shouldn't this be seeded based on > base::RandInt() or something and if any failures occur the seed printed out? I don't think so. The point is not make each test different, the point is to stimulate internal state in ways that I might have missed in my other tests. https://codereview.chromium.org/1165903002/diff/440001/media/blink/multibuffe... File media/blink/multibuffer.cc (right): https://codereview.chromium.org/1165903002/diff/440001/media/blink/multibuffe... media/blink/multibuffer.cc:96: freed--; On 2015/10/28 23:06:13, DaleCurtis wrote: > How does this work? freed is zero and doesn't seem to be set then you subtract > from it? Seems like this overflows "-1" unsigned. OOps, should be ++. Added test as well. https://codereview.chromium.org/1165903002/diff/440001/media/blink/multibuffe... media/blink/multibuffer.cc:98: for (const auto &to_free_pair : to_free) { On 2015/10/28 23:06:13, DaleCurtis wrote: > auto& Done. https://codereview.chromium.org/1165903002/diff/440001/media/blink/multibuffe... media/blink/multibuffer.cc:107: scoped_refptr<GlobalLRU> global_lru) : On 2015/10/28 23:06:13, DaleCurtis wrote: > const& Done. https://codereview.chromium.org/1165903002/diff/440001/media/blink/multibuffe... media/blink/multibuffer.cc:218: for (MultiBufferBlockId to_free : blocks) { On 2015/10/28 23:06:13, DaleCurtis wrote: > const auto&? It's an integer type though, using "const auto&" seems to imply that it's something big and complicated that I don't want to copy. https://codereview.chromium.org/1165903002/diff/440001/media/blink/multibuffe... media/blink/multibuffer.cc:330: int64_t blocks_added = -data_.size(); On 2015/10/28 23:06:13, DaleCurtis wrote: > -size_t might be problematic? Assuming unary minus behaves anything like binary minus, it should always return a signed type. (int or bigger) However, making it more explicit will probably help the reader. https://codereview.chromium.org/1165903002/diff/440001/media/blink/multibuffe... media/blink/multibuffer.cc:375: for (auto data : other->data_) { On 2015/10/28 23:06:13, DaleCurtis wrote: > const auto& here and the three below. Done. The range iterator used in the two cases below is slightly non-standard, and the range that we get a const reference to would only exist in some temporary memory somewhere, but it works... https://codereview.chromium.org/1165903002/diff/440001/media/blink/multibuffe... media/blink/multibuffer.cc:430: for (auto present_block_range = present_.find(transition_end - 1); On 2015/10/28 23:06:13, DaleCurtis wrote: > ditto. Ditto what? This is an iterator... https://codereview.chromium.org/1165903002/diff/440001/media/blink/multibuffer.h File media/blink/multibuffer.h (right): https://codereview.chromium.org/1165903002/diff/440001/media/blink/multibuffe... media/blink/multibuffer.h:123: class GlobalLRU: public base::RefCounted<GlobalLRU> { On 2015/10/28 23:06:13, DaleCurtis wrote: > space before : Done. https://codereview.chromium.org/1165903002/diff/440001/media/blink/multibuffe... media/blink/multibuffer.h:306: On 2015/10/28 23:06:13, DaleCurtis wrote: > Remove extra line. Done. https://codereview.chromium.org/1165903002/diff/440001/media/blink/multibuffe... File media/blink/multibuffer_reader.cc (right): https://codereview.chromium.org/1165903002/diff/440001/media/blink/multibuffe... media/blink/multibuffer_reader.cc:137: cb_= base::Closure(); On 2015/10/28 23:06:13, DaleCurtis wrote: > Why? cb_.Reset() ? Done. (Also fixed in TryRead()) https://codereview.chromium.org/1165903002/diff/440001/media/blink/multibuffe... media/blink/multibuffer_reader.cc:170: void MultiBufferReader::Call(const base::Closure& cb) const { On 2015/10/28 23:06:13, DaleCurtis wrote: > Should be a way to avoid this by making cb_ a CancelableClosure. Lets you remove > the weak factory too I think. That doesn't seem like it would be better. Unless I'm missing something, I would have to remember the callback when I posttask it and then manually cancel it in the destructor, right? https://codereview.chromium.org/1165903002/diff/440001/media/blink/multibuffe... File media/blink/multibuffer_unittest.cc (right): https://codereview.chromium.org/1165903002/diff/440001/media/blink/multibuffe... media/blink/multibuffer_unittest.cc:124: scoped_refptr<media::MultiBuffer::GlobalLRU> lru) : On 2015/10/28 23:06:13, DaleCurtis wrote: > const& for all scoped_refptr passes to avoid accidental atomic ref/unref Done. https://codereview.chromium.org/1165903002/diff/440001/media/blink/multibuffe... media/blink/multibuffer_unittest.cc:492: } On 2015/10/28 23:06:13, DaleCurtis wrote: > Needs non-random tests? Only the last two tests are random.
Just some quick replies, haven't looked again yet. https://codereview.chromium.org/1165903002/diff/440001/media/blink/lru_unitte... File media/blink/lru_unittest.cc (right): https://codereview.chromium.org/1165903002/diff/440001/media/blink/lru_unitte... media/blink/lru_unittest.cc:77: LRUTest() : rnd_(42) {} On 2015/10/29 17:52:44, hubbe wrote: > On 2015/10/28 23:06:13, DaleCurtis wrote: > > Hmm, I don't understand the point of the random test if you are using a > sequence > > which always remains the same. Instead shouldn't this be seeded based on > > base::RandInt() or something and if any failures occur the seed printed out? > > I don't think so. The point is not make each test different, the point is to > stimulate internal state in ways that I might have missed in my other tests. Seems like varying the test would do that better though? So long as runs can be reconstructed it seems useful. https://codereview.chromium.org/1165903002/diff/440001/media/blink/multibuffe... File media/blink/multibuffer.cc (right): https://codereview.chromium.org/1165903002/diff/440001/media/blink/multibuffe... media/blink/multibuffer.cc:430: for (auto present_block_range = present_.find(transition_end - 1); On 2015/10/29 17:52:44, hubbe wrote: > On 2015/10/28 23:06:13, DaleCurtis wrote: > > ditto. > > Ditto what? This is an iterator... Ditto for const auto& unless you need the non-const iterator, it doesn't look like it. https://codereview.chromium.org/1165903002/diff/440001/media/blink/multibuffe... File media/blink/multibuffer_reader.cc (right): https://codereview.chromium.org/1165903002/diff/440001/media/blink/multibuffe... media/blink/multibuffer_reader.cc:170: void MultiBufferReader::Call(const base::Closure& cb) const { On 2015/10/29 17:52:44, hubbe wrote: > On 2015/10/28 23:06:13, DaleCurtis wrote: > > Should be a way to avoid this by making cb_ a CancelableClosure. Lets you > remove > > the weak factory too I think. > > That doesn't seem like it would be better. > Unless I'm missing something, I would have to remember the callback when I > posttask it and then manually cancel it in the destructor, right? It'll auto cancel when the member variable goes out of scope.
https://codereview.chromium.org/1165903002/diff/460001/media/blink/multibuffe... File media/blink/multibuffer.cc (right): https://codereview.chromium.org/1165903002/diff/460001/media/blink/multibuffe... media/blink/multibuffer.cc:336: eof = data_[pos]->end_of_stream(); eof = (data_[pos] = provider->Read()); to save a lookup. https://codereview.chromium.org/1165903002/diff/460001/media/blink/multibuffe... media/blink/multibuffer.cc:354: auto i = writer_index_.find(pos); how is it removed if we AddProvider() it above? if so, can't one just check for |!eof| ? https://codereview.chromium.org/1165903002/diff/460001/media/blink/multibuffer.h File media/blink/multibuffer.h (right): https://codereview.chromium.org/1165903002/diff/460001/media/blink/multibuffe... media/blink/multibuffer.h:178: // from A to B where: A <= |pos|, B >= |pos| and all blocks in [A..B) good explanation. https://codereview.chromium.org/1165903002/diff/460001/media/blink/multibuffe... File media/blink/multibuffer_reader.cc (right): https://codereview.chromium.org/1165903002/diff/460001/media/blink/multibuffe... media/blink/multibuffer_reader.cc:207: preload_pos_ = multibuffer_->FindNextUnavailable(preload_pos_); isn't it possible that this retruns preload_pos_+1, at which point we might just re-add at preload_pos_-1 @221? i think this happens if we're already at the max preload and on the frontier of the cached data. in which case, that seems like a common case that can be avoided just by reshuffling a bit to check first.
PTAL Addressed (most) comments, fixed some compile issues and cleaned up a few things. https://codereview.chromium.org/1165903002/diff/440001/media/blink/lru_unitte... File media/blink/lru_unittest.cc (right): https://codereview.chromium.org/1165903002/diff/440001/media/blink/lru_unitte... media/blink/lru_unittest.cc:77: LRUTest() : rnd_(42) {} On 2015/10/29 17:59:05, DaleCurtis wrote: > On 2015/10/29 17:52:44, hubbe wrote: > > On 2015/10/28 23:06:13, DaleCurtis wrote: > > > Hmm, I don't understand the point of the random test if you are using a > > sequence > > > which always remains the same. Instead shouldn't this be seeded based on > > > base::RandInt() or something and if any failures occur the seed printed out? > > > > I don't think so. The point is not make each test different, the point is to > > stimulate internal state in ways that I might have missed in my other tests. > > Seems like varying the test would do that better though? So long as runs can be > reconstructed it seems useful. I think the usefulness would be cancelled out by creating a potentially flaky test. https://codereview.chromium.org/1165903002/diff/440001/media/blink/multibuffe... File media/blink/multibuffer.cc (right): https://codereview.chromium.org/1165903002/diff/440001/media/blink/multibuffe... media/blink/multibuffer.cc:330: int64_t blocks_added = -data_.size(); On 2015/10/29 17:52:44, hubbe wrote: > On 2015/10/28 23:06:13, DaleCurtis wrote: > > -size_t might be problematic? > > Assuming unary minus behaves anything like binary minus, it should always return > a signed type. (int or bigger) > However, making it more explicit will probably help the reader. Turns out you're right, - on size_t is problematic. I should learn never to underestimate how quirky C++ is some day.... https://codereview.chromium.org/1165903002/diff/440001/media/blink/multibuffe... media/blink/multibuffer.cc:430: for (auto present_block_range = present_.find(transition_end - 1); On 2015/10/29 17:59:05, DaleCurtis wrote: > On 2015/10/29 17:52:44, hubbe wrote: > > On 2015/10/28 23:06:13, DaleCurtis wrote: > > > ditto. > > > > Ditto what? This is an iterator... > > Ditto for const auto& unless you need the non-const iterator, it doesn't look > like it. A const iterator is an iterator to a const container, the iterator itself is not const. (If it was, ++ wouldn't work.) If I make this const, then I get an error on line 432 when it tries to decrement the iterator. Perhaps I should just avoid using auto here, that might make it more clear? https://codereview.chromium.org/1165903002/diff/440001/media/blink/multibuffe... File media/blink/multibuffer_reader.cc (right): https://codereview.chromium.org/1165903002/diff/440001/media/blink/multibuffe... media/blink/multibuffer_reader.cc:170: void MultiBufferReader::Call(const base::Closure& cb) const { On 2015/10/29 17:59:05, DaleCurtis wrote: > On 2015/10/29 17:52:44, hubbe wrote: > > On 2015/10/28 23:06:13, DaleCurtis wrote: > > > Should be a way to avoid this by making cb_ a CancelableClosure. Lets you > > remove > > > the weak factory too I think. > > > > That doesn't seem like it would be better. > > Unless I'm missing something, I would have to remember the callback when I > > posttask it and then manually cancel it in the destructor, right? > > It'll auto cancel when the member variable goes out of scope. Interesting, but I'm still not convinced it's better. I'd still need to keep cb_ so that I can test if's been set or not, and then put it into cb_cancellable_ when I do the PostTask call. Also, I've decided to PostTask the progress callback as well (simplifies things in the next CL) and I can do that through the same Call() method. If I use a CancellableClosure I'd need one for each callback. (Or am I still missing something?) https://codereview.chromium.org/1165903002/diff/460001/media/blink/multibuffe... File media/blink/multibuffer.cc (right): https://codereview.chromium.org/1165903002/diff/460001/media/blink/multibuffe... media/blink/multibuffer.cc:336: eof = data_[pos]->end_of_stream(); On 2015/10/29 18:44:00, liberato wrote: > eof = (data_[pos] = provider->Read()); to save a lookup. Saved the lookup slightly differently, hope that's ok. https://codereview.chromium.org/1165903002/diff/460001/media/blink/multibuffe... media/blink/multibuffer.cc:354: auto i = writer_index_.find(pos); On 2015/10/29 18:44:00, liberato wrote: > how is it removed if we AddProvider() it above? if so, can't one just check for > |!eof| ? Comment added. https://codereview.chromium.org/1165903002/diff/460001/media/blink/multibuffer.h File media/blink/multibuffer.h (right): https://codereview.chromium.org/1165903002/diff/460001/media/blink/multibuffe... media/blink/multibuffer.h:178: // from A to B where: A <= |pos|, B >= |pos| and all blocks in [A..B) On 2015/10/29 18:44:00, liberato wrote: > good explanation. Acknowledged. https://codereview.chromium.org/1165903002/diff/460001/media/blink/multibuffe... File media/blink/multibuffer_reader.cc (right): https://codereview.chromium.org/1165903002/diff/460001/media/blink/multibuffe... media/blink/multibuffer_reader.cc:207: preload_pos_ = multibuffer_->FindNextUnavailable(preload_pos_); On 2015/10/29 18:44:00, liberato wrote: > isn't it possible that this retruns preload_pos_+1, at which point we might just > re-add at preload_pos_-1 @221? i think this happens if we're already at the max > preload and on the frontier of the cached data. > > in which case, that seems like a common case that can be avoided just by > reshuffling a bit to check first. Renamed function added comments as per offline discussion.
Still processing the rangemap magic, partial comment dump. https://codereview.chromium.org/1165903002/diff/440001/media/blink/lru_unitte... File media/blink/lru_unittest.cc (right): https://codereview.chromium.org/1165903002/diff/440001/media/blink/lru_unitte... media/blink/lru_unittest.cc:77: LRUTest() : rnd_(42) {} On 2015/10/29 21:45:05, hubbe wrote: > On 2015/10/29 17:59:05, DaleCurtis wrote: > > On 2015/10/29 17:52:44, hubbe wrote: > > > On 2015/10/28 23:06:13, DaleCurtis wrote: > > > > Hmm, I don't understand the point of the random test if you are using a > > > sequence > > > > which always remains the same. Instead shouldn't this be seeded based on > > > > base::RandInt() or something and if any failures occur the seed printed > out? > > > > > > I don't think so. The point is not make each test different, the point is to > > > stimulate internal state in ways that I might have missed in my other tests. > > > > Seems like varying the test would do that better though? So long as runs can > be > > reconstructed it seems useful. > > I think the usefulness would be cancelled out by creating a potentially flaky > test. > That's why I was expecting failures to print the seed used so that any time it does fail you'll have the values necessary to reconstruct the failure. The caveat would be we need to ensure all test failures are logged (even those that automatic retries correct). I think our flakiness dashboard tracks this, but I'm not positive. A test which always runs the same path based on a value which is always the same seems no more random than the arbitrarily set of test cases one might normally choose to write by hand :) In which case the addition of a "random" number generator feels like cognitive overhead. https://codereview.chromium.org/1165903002/diff/500001/media/blink/lru_unitte... File media/blink/lru_unittest.cc (right): https://codereview.chromium.org/1165903002/diff/500001/media/blink/lru_unitte... media/blink/lru_unittest.cc:20: class SimpleLRU { Typically reimplementation type tests like this are considered a bit overboard. Since both typically have the same author they can suffer from repeated mistakes as well. I've generally found it to be just as effective to compare against known good results. Especially in cases like this where you're evaluating the correctness of an algorithm. A few hand picked test cases are generally sufficient. So long as this doesn't add seconds of test runtime it's fine though, so it's up to you if you prefer to keep this. https://codereview.chromium.org/1165903002/diff/500001/media/blink/multibuffer.h File media/blink/multibuffer.h (right): https://codereview.chromium.org/1165903002/diff/500001/media/blink/multibuffe... media/blink/multibuffer.h:36: return base::HashPair(reinterpret_cast<uint64>(key.first), key.second); This doesn't seem right, forcing it to a 64-bit always will leave the upper 32 bits zero on 32-bit platforms. It seems more likely that you want to use the correct size here. I also don't think you need this, since you've got a GlobalLRU which will manage multibuffer instances, can't each MultiBuffer should just get a unique ID from the GLRU and use that instead of a raw ptr hash?
PTAL. Would you prefer it if I split up this CL into smaller pieces? LRU, Rangemap, multibuffer and multibuffer_reader can all potentially be individual code reviews. https://codereview.chromium.org/1165903002/diff/440001/media/blink/lru_unitte... File media/blink/lru_unittest.cc (right): https://codereview.chromium.org/1165903002/diff/440001/media/blink/lru_unitte... media/blink/lru_unittest.cc:77: LRUTest() : rnd_(42) {} On 2015/10/30 00:24:24, DaleCurtis wrote: > On 2015/10/29 21:45:05, hubbe wrote: > > On 2015/10/29 17:59:05, DaleCurtis wrote: > > > On 2015/10/29 17:52:44, hubbe wrote: > > > > On 2015/10/28 23:06:13, DaleCurtis wrote: > > > > > Hmm, I don't understand the point of the random test if you are using a > > > > sequence > > > > > which always remains the same. Instead shouldn't this be seeded based on > > > > > base::RandInt() or something and if any failures occur the seed printed > > out? > > > > > > > > I don't think so. The point is not make each test different, the point is > to > > > > stimulate internal state in ways that I might have missed in my other > tests. > > > > > > Seems like varying the test would do that better though? So long as runs can > > be > > > reconstructed it seems useful. > > > > I think the usefulness would be cancelled out by creating a potentially flaky > > test. > > > > That's why I was expecting failures to print the seed used so that any time it > does fail you'll have the values necessary to reconstruct the failure. The > caveat would be we need to ensure all test failures are logged (even those that > automatic retries correct). I think our flakiness dashboard tracks this, but I'm > not positive. > > A test which always runs the same path based on a value which is always the same > seems no more random than the arbitrarily set of test cases one might normally > choose to write by hand :) In which case the addition of a "random" number > generator feels like cognitive overhead. The difference is that a hand written tests might try 10 different internal state combinations, while one with a loop and a random might test 1000 or 10000. If using a real random seed would actually find additional problems, chances are that the automatic re-trying of tests would hide those problems anyways. https://codereview.chromium.org/1165903002/diff/500001/media/blink/lru_unitte... File media/blink/lru_unittest.cc (right): https://codereview.chromium.org/1165903002/diff/500001/media/blink/lru_unitte... media/blink/lru_unittest.cc:20: class SimpleLRU { On 2015/10/30 00:24:24, DaleCurtis wrote: > Typically reimplementation type tests like this are considered a bit overboard. > Since both typically have the same author they can suffer from repeated mistakes > as well. > > I've generally found it to be just as effective to compare against known good > results. Especially in cases like this where you're evaluating the correctness > of an algorithm. A few hand picked test cases are generally sufficient. > > So long as this doesn't add seconds of test runtime it's fine though, so it's up > to you if you prefer to keep this. I do tend to go a bit overboard sometimes. :) This class wouldn't really be needed if it wasn't for the random test, and if you take a look below, I think you'll see that I use some simple hand-coded examples as well. Since I do like my random tests, I prefer to keep this class. I understand the concern about repeated mistakes though. I'm hoping that this class is simple enough to make such mistakes obvious to anybody reading the code... https://codereview.chromium.org/1165903002/diff/500001/media/blink/multibuffer.h File media/blink/multibuffer.h (right): https://codereview.chromium.org/1165903002/diff/500001/media/blink/multibuffe... media/blink/multibuffer.h:36: return base::HashPair(reinterpret_cast<uint64>(key.first), key.second); Guess that's what I get from copying code from somewhere else in chrome. :) I replaced uint64 with inptr_t, I don't think it makes any difference in terms of function, but it makes the code cleaner and it might save an instruction or two. As for replacing the pointer with an integer: GlobalLRU uses the pointer to call functions in the multibuffer. I could replace the pointer with an ID, but I'd also need a map from id to multibuffer. Unless I'm missing something about the current implementation, I think it's simpler than using integer IDs.
Splitting this would be helpful since I'll be traveling the next few days and may not have a chance to review this before Friday (sorry). If you split I defer to xhwang@, liberato@ on the various pieces so you can keep the conversations local to KIR.
https://codereview.chromium.org/1165903002/diff/540001/media/blink/multibuffe... File media/blink/multibuffer.cc (right): https://codereview.chromium.org/1165903002/diff/540001/media/blink/multibuffe... media/blink/multibuffer.cc:88: // Group the blocks by multibuffer, then free them. why group them? https://codereview.chromium.org/1165903002/diff/540001/media/blink/multibuffe... media/blink/multibuffer.cc:144: closest_block = pos; not sure we should ever get here. won't the || Contains(pos) @131 return true if |pos| is present? https://codereview.chromium.org/1165903002/diff/540001/media/blink/multibuffe... media/blink/multibuffer.cc:151: if (closest_writer > closest_block) { is this to make sure that any writer we choose won't hit a present block before filling the request? if so, then that's probably worth a comment. https://codereview.chromium.org/1165903002/diff/540001/media/blink/multibuffe... media/blink/multibuffer.cc:356: // or self-destruct and clean up any associated writers.) comment would be helpful: "// or we didn't re-add due to colliding providers or eof." https://codereview.chromium.org/1165903002/diff/540001/media/blink/multibuffe... media/blink/multibuffer.cc:379: lru_->Insert(this, data.first); what happens with IncrementDataSize? presumably, the other cache already added it to the LRU, but we both might subtract it later. since the data is really shared, it's kinda hard to get right. is there going to be a case where this couldn't be used as "MoveFrom()"? that would avoid both issues. https://codereview.chromium.org/1165903002/diff/540001/media/blink/multibuffe... media/blink/multibuffer.cc:399: } should this cleanup writers? we might have just merged blocks in that make them unneeded.
Commentes addressed. Will split out multibuffer_reader into a separate CL next. https://codereview.chromium.org/1165903002/diff/540001/media/blink/multibuffe... File media/blink/multibuffer.cc (right): https://codereview.chromium.org/1165903002/diff/540001/media/blink/multibuffe... media/blink/multibuffer.cc:88: // Group the blocks by multibuffer, then free them. On 2015/11/06 17:29:38, liberato wrote: > why group them? Comment updated. https://codereview.chromium.org/1165903002/diff/540001/media/blink/multibuffe... media/blink/multibuffer.cc:144: closest_block = pos; On 2015/11/06 17:29:38, liberato wrote: > not sure we should ever get here. won't the || Contains(pos) @131 return true > if |pos| is present? Good point. Added NOTREACHED() and a comment. https://codereview.chromium.org/1165903002/diff/540001/media/blink/multibuffe... media/blink/multibuffer.cc:151: if (closest_writer > closest_block) { On 2015/11/06 17:29:38, liberato wrote: > is this to make sure that any writer we choose won't hit a present block before > filling the request? if so, then that's probably worth a comment. Done. https://codereview.chromium.org/1165903002/diff/540001/media/blink/multibuffe... media/blink/multibuffer.cc:356: // or self-destruct and clean up any associated writers.) On 2015/11/06 17:29:38, liberato wrote: > comment would be helpful: "// or we didn't re-add due to colliding providers or > eof." Comment updated. https://codereview.chromium.org/1165903002/diff/540001/media/blink/multibuffe... media/blink/multibuffer.cc:379: lru_->Insert(this, data.first); On 2015/11/06 17:29:38, liberato wrote: > what happens with IncrementDataSize? presumably, the other cache already added > it to the LRU, but we both might subtract it later. > > since the data is really shared, it's kinda hard to get right. > > is there going to be a case where this couldn't be used as "MoveFrom()"? that > would avoid both issues. The LRU will just assume that the data is not shared, which I think is fine. In most cases, "other" will end up getting cleaned up soon-ish and the problem goes away. https://codereview.chromium.org/1165903002/diff/540001/media/blink/multibuffe... media/blink/multibuffer.cc:399: } On 2015/11/06 17:29:38, liberato wrote: > should this cleanup writers? we might have just merged blocks in that make them > unneeded. They will figure it out next time they try to add data.
Description was changed from ========== Multi reader/writer cache/buffer MultiBuffer is meant to be integrated into buffered_resource_loader to provide more efficient cacheing. Design doc: https://docs.google.com/a/google.com/document/d/1hDkMV-bCTZm-Sf6pQj-v5PRqdSSl... ========== to ========== Multi reader/writer cache/buffer MultiBuffer is meant to be integrated into buffered_resource_loader to provide more efficient cacheing. Unit tests are in a separate CL: https://codereview.chromium.org/1420883004 Depends on: https://codereview.chromium.org/1427433012/ https://codereview.chromium.org/1422523007/ Media cache design doc: https://docs.google.com/document/d/15q6LTG0iDUe30QcoMtj4XNmKCa_7W_Q2uUIPFsJhS... BUG=514719 ==========
Dale, I've split out as much as I can from this CL. (Perhaps multibuffer_reader shouldn't have been split up, but I'll let you decide, there is a link in the CL description to the multibuffer reader, which also has the unit tests for this CL.) You're still the reviewer on this CL, and feel free to check in on the split out pieces as you see fit.
lgtm % some minor changes. Nice work! I gave the doc another once over too and this all roughly makes sense with what's in there. As discussed offline, I'm not 100% on what interactions may fall out w/ your other CL, so please put this through the layout and telemetry tests. https://codereview.chromium.org/1165903002/diff/640001/media/blink/BUILD.gn File media/blink/BUILD.gn (right): https://codereview.chromium.org/1165903002/diff/640001/media/blink/BUILD.gn#n... media/blink/BUILD.gn:46: "lru.h", Presumably lru.h, rangemap.h will be added in the respective CLs? https://codereview.chromium.org/1165903002/diff/640001/media/blink/media_blin... File media/blink/media_blink.gyp (right): https://codereview.chromium.org/1165903002/diff/640001/media/blink/media_blin... media/blink/media_blink.gyp:51: 'lru.h', Ditto? https://codereview.chromium.org/1165903002/diff/640001/media/blink/multibuffe... File media/blink/multibuffer.cc (right): https://codereview.chromium.org/1165903002/diff/640001/media/blink/multibuffe... media/blink/multibuffer.cc:129: std::set<Reader*>& set_of_readers = readers_[pos]; Is & what you wanted here? https://codereview.chromium.org/1165903002/diff/640001/media/blink/multibuffe... media/blink/multibuffer.cc:139: DataProvider* provider = NULL; nullptr https://codereview.chromium.org/1165903002/diff/640001/media/blink/multibuffe... media/blink/multibuffer.cc:167: base::Unretained(provider))); Second unretained necessary? https://codereview.chromium.org/1165903002/diff/640001/media/blink/multibuffe... media/blink/multibuffer.cc:219: void MultiBuffer::ReleaseBlocks(const std::vector<MultiBufferBlockId> blocks) { Missing & https://codereview.chromium.org/1165903002/diff/640001/media/blink/multibuffe... media/blink/multibuffer.cc:248: auto j = i; auto j = i - 1? https://codereview.chromium.org/1165903002/diff/640001/media/blink/multibuffe... media/blink/multibuffer.cc:255: auto j = i; auto j = i + 1 https://codereview.chromium.org/1165903002/diff/640001/media/blink/multibuffer.h File media/blink/multibuffer.h (right): https://codereview.chromium.org/1165903002/diff/640001/media/blink/multibuffe... media/blink/multibuffer.h:41: return base::HashPair(reinterpret_cast<int64_t>(key.first), key.second); intptr_t ? https://codereview.chromium.org/1165903002/diff/640001/media/blink/multibuffe... media/blink/multibuffer.h:294: std::map<BlockId, DataProvider*> writer_index_; ScopedPtrMap?
https://codereview.chromium.org/1165903002/diff/640001/media/blink/BUILD.gn File media/blink/BUILD.gn (right): https://codereview.chromium.org/1165903002/diff/640001/media/blink/BUILD.gn#n... media/blink/BUILD.gn:46: "lru.h", On 2015/11/11 23:48:17, DaleCurtis wrote: > Presumably lru.h, rangemap.h will be added in the respective CLs? I'm adding them here since since they are now used. (Before they were not.) https://codereview.chromium.org/1165903002/diff/640001/media/blink/media_blin... File media/blink/media_blink.gyp (right): https://codereview.chromium.org/1165903002/diff/640001/media/blink/media_blin... media/blink/media_blink.gyp:51: 'lru.h', On 2015/11/11 23:48:17, DaleCurtis wrote: > Ditto? Acknowledged. https://codereview.chromium.org/1165903002/diff/640001/media/blink/multibuffe... File media/blink/multibuffer.cc (right): https://codereview.chromium.org/1165903002/diff/640001/media/blink/multibuffe... media/blink/multibuffer.cc:129: std::set<Reader*>& set_of_readers = readers_[pos]; On 2015/11/11 23:48:17, DaleCurtis wrote: > Is & what you wanted here? Converted to pointer. https://codereview.chromium.org/1165903002/diff/640001/media/blink/multibuffe... media/blink/multibuffer.cc:139: DataProvider* provider = NULL; On 2015/11/11 23:48:17, DaleCurtis wrote: > nullptr Done. https://codereview.chromium.org/1165903002/diff/640001/media/blink/multibuffe... media/blink/multibuffer.cc:167: base::Unretained(provider))); On 2015/11/11 23:48:17, DaleCurtis wrote: > Second unretained necessary? Apparently so, removed. https://codereview.chromium.org/1165903002/diff/640001/media/blink/multibuffe... media/blink/multibuffer.cc:219: void MultiBuffer::ReleaseBlocks(const std::vector<MultiBufferBlockId> blocks) { On 2015/11/11 23:48:17, DaleCurtis wrote: > Missing & Done. https://codereview.chromium.org/1165903002/diff/640001/media/blink/multibuffe... media/blink/multibuffer.cc:248: auto j = i; On 2015/11/11 23:48:17, DaleCurtis wrote: > auto j = i - 1? i/j are not RandomAccessIterators (They are BidirectionalIterators) https://codereview.chromium.org/1165903002/diff/640001/media/blink/multibuffe... media/blink/multibuffer.cc:255: auto j = i; On 2015/11/11 23:48:17, DaleCurtis wrote: > auto j = i + 1 See above. https://codereview.chromium.org/1165903002/diff/640001/media/blink/multibuffer.h File media/blink/multibuffer.h (right): https://codereview.chromium.org/1165903002/diff/640001/media/blink/multibuffe... media/blink/multibuffer.h:41: return base::HashPair(reinterpret_cast<int64_t>(key.first), key.second); On 2015/11/11 23:48:17, DaleCurtis wrote: > intptr_t ? Did you read the comment? https://codereview.chromium.org/1165903002/diff/640001/media/blink/multibuffe... media/blink/multibuffer.h:294: std::map<BlockId, DataProvider*> writer_index_; On 2015/11/11 23:48:17, DaleCurtis wrote: > ScopedPtrMap? I'd need to fix ScopedPtrMap to expose upper_bound/lower_bound (and a "get" function would be really helpful...) I've added a TODO for it.
https://codereview.chromium.org/1165903002/diff/640001/media/blink/multibuffer.h File media/blink/multibuffer.h (right): https://codereview.chromium.org/1165903002/diff/640001/media/blink/multibuffe... media/blink/multibuffer.h:41: return base::HashPair(reinterpret_cast<int64_t>(key.first), key.second); On 2015/11/12 22:18:37, hubbe wrote: > On 2015/11/11 23:48:17, DaleCurtis wrote: > > intptr_t ? > > Did you read the comment? No, sorry! Does uintptr_t work?
https://codereview.chromium.org/1165903002/diff/640001/media/blink/multibuffer.h File media/blink/multibuffer.h (right): https://codereview.chromium.org/1165903002/diff/640001/media/blink/multibuffe... media/blink/multibuffer.h:294: std::map<BlockId, DataProvider*> writer_index_; On 2015/11/12 22:18:37, hubbe wrote: > On 2015/11/11 23:48:17, DaleCurtis wrote: > > ScopedPtrMap? > > I'd need to fix ScopedPtrMap to expose upper_bound/lower_bound (and a "get" > function would be really helpful...) > > I've added a TODO for it. Hmm, does it not work with std::lower_bound, std::upper_bound?
https://codereview.chromium.org/1165903002/diff/640001/media/blink/multibuffer.h File media/blink/multibuffer.h (right): https://codereview.chromium.org/1165903002/diff/640001/media/blink/multibuffe... media/blink/multibuffer.h:41: return base::HashPair(reinterpret_cast<int64_t>(key.first), key.second); On 2015/11/12 23:22:03, DaleCurtis wrote: > On 2015/11/12 22:18:37, hubbe wrote: > > On 2015/11/11 23:48:17, DaleCurtis wrote: > > > intptr_t ? > > > > Did you read the comment? > > No, sorry! Does uintptr_t work? No, same problem. https://codereview.chromium.org/1165903002/diff/640001/media/blink/multibuffe... media/blink/multibuffer.h:294: std::map<BlockId, DataProvider*> writer_index_; On 2015/11/12 23:22:54, DaleCurtis wrote: > On 2015/11/12 22:18:37, hubbe wrote: > > On 2015/11/11 23:48:17, DaleCurtis wrote: > > > ScopedPtrMap? > > > > I'd need to fix ScopedPtrMap to expose upper_bound/lower_bound (and a "get" > > function would be really helpful...) > > > > I've added a TODO for it. > > Hmm, does it not work with std::lower_bound, std::upper_bound? It might, but the performance would suffer since std::map::iterator is not a RandomAccessIterator. (It would have to walk the tree to find stuff.) Adding lower_bound/upper_bound to ScopedPtrMap is a better solution imho, but outside the scope of this CL.
https://codereview.chromium.org/1165903002/diff/640001/media/blink/multibuffer.h File media/blink/multibuffer.h (right): https://codereview.chromium.org/1165903002/diff/640001/media/blink/multibuffe... media/blink/multibuffer.h:41: return base::HashPair(reinterpret_cast<int64_t>(key.first), key.second); On 2015/11/12 23:34:40, hubbe wrote: > On 2015/11/12 23:22:03, DaleCurtis wrote: > > On 2015/11/12 22:18:37, hubbe wrote: > > > On 2015/11/11 23:48:17, DaleCurtis wrote: > > > > intptr_t ? > > > > > > Did you read the comment? > > > > No, sorry! Does uintptr_t work? > > No, same problem. #ifdef ARCH_CPU_64_BITS ?
https://codereview.chromium.org/1165903002/diff/640001/media/blink/multibuffer.h File media/blink/multibuffer.h (right): https://codereview.chromium.org/1165903002/diff/640001/media/blink/multibuffe... media/blink/multibuffer.h:41: return base::HashPair(reinterpret_cast<int64_t>(key.first), key.second); On 2015/11/12 23:43:14, DaleCurtis wrote: > On 2015/11/12 23:34:40, hubbe wrote: > > On 2015/11/12 23:22:03, DaleCurtis wrote: > > > On 2015/11/12 22:18:37, hubbe wrote: > > > > On 2015/11/11 23:48:17, DaleCurtis wrote: > > > > > intptr_t ? > > > > > > > > Did you read the comment? > > > > > > No, sorry! Does uintptr_t work? > > > > No, same problem. > > #ifdef ARCH_CPU_64_BITS ? Should work, done.
still lgtm,
The CQ bit was checked by hubbe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from liberato@chromium.org Link to the patchset: https://codereview.chromium.org/1165903002/#ps680001 (title: "save a few cycles on 32 bit platforms")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1165903002/680001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1165903002/680001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by hubbe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org, liberato@chromium.org Link to the patchset: https://codereview.chromium.org/1165903002/#ps700001 (title: "formatted")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1165903002/700001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1165903002/700001
Message was sent while issue was closed.
Committed patchset #36 (id:700001)
Message was sent while issue was closed.
Patchset 36 (id:??) landed as https://crrev.com/89d5cdfa35dad6711cc0177e3eaae75f5e08d83a Cr-Commit-Position: refs/heads/master@{#359627} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
