Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2519473002:
Fixes the cache lock issue. (Closed)
|
Created:
4 years, 1 month ago by shivanisha Modified:
3 years, 8 months ago CC:
cbentzel+watch_chromium.org, chromium-reviews, devtools-reviews_chromium.org, gavinp+disk_chromium.org, kinuko+cache_chromium.org, pfeldman Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFixes the cache lock issue such that multiple transactions can start
consuming the response data via a single network transaction.
Design document:
https://docs.google.com/document/d/1KHT2sQ2ASTs-_t1xvptn2anUDx7FOo8Dz8NKpkuWtsE/edit?usp=sharing
BUG=472740
Patch Set 1 : Initial patch #
Total comments: 45
Patch Set 2 : Josh's initial feedback incorporated. #Patch Set 3 : Making CacheWriteFailedSharedWriters return a std::unique_ptr instead of raw pointer so that it is … #Patch Set 4 : Added traces and net log in HttpCache::Transaction for shared events. #Patch Set 5 : Redesigned the fix using DataAccess class for eliminating Orphan API.(Rebased till refs/heads/master@{#442607}) #
Total comments: 47
Patch Set 6 : Removing the need for SharedWriters to destroy itself. #Patch Set 7 : Feedback incorporated (Rebased till refs/heads/master@{#446065}) #
Total comments: 36
Patch Set 8 : Test only change to fix a memory leak. #
Total comments: 25
Patch Set 9 : Josh's early feedback addressed. #
Total comments: 4
Patch Set 10 : Feedback addressed #
Messages
Total messages: 106 (63 generated)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
Patchset #1 (id:40001) has been deleted
The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run
Patchset #1 (id:60001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
Description was changed from ========== Fixes the cache lock issue such that multiple transactions can start consuming the response data via a single network transaction. Design document: https://docs.google.com/document/d/1KHT2sQ2ASTs-_t1xvptn2anUDx7FOo8Dz8NKpkuWt... BUG=472740 ========== to ========== Please take a look. Thanks! Its a large CL so please feel free to let me know if you want to discuss parts of it. Also, the design doc has an added section in the end which describes some of the scenarios that lead to either a transaction destruction from SharedWriters or SharedWriters destruction. I will be working on some more UMA/logging and upload it in a follow up CL/patch. --------- Fixes the cache lock issue such that multiple transactions can start consuming the response data via a single network transaction. Design document: https://docs.google.com/document/d/1KHT2sQ2ASTs-_t1xvptn2anUDx7FOo8Dz8NKpkuWt... BUG=472740 ==========
shivanisha@chromium.org changed reviewers: + jkarlin@chromium.org, rdsmith@chromium.org
Description was changed from ========== Please take a look. Thanks! Its a large CL so please feel free to let me know if you want to discuss parts of it. Also, the design doc has an added section in the end which describes some of the scenarios that lead to either a transaction destruction from SharedWriters or SharedWriters destruction. I will be working on some more UMA/logging and upload it in a follow up CL/patch. --------- Fixes the cache lock issue such that multiple transactions can start consuming the response data via a single network transaction. Design document: https://docs.google.com/document/d/1KHT2sQ2ASTs-_t1xvptn2anUDx7FOo8Dz8NKpkuWt... BUG=472740 ========== to ========== Fixes the cache lock issue such that multiple transactions can start consuming the response data via a single network transaction. Design document: https://docs.google.com/document/d/1KHT2sQ2ASTs-_t1xvptn2anUDx7FOo8Dz8NKpkuWt... BUG=472740 ==========
Please take a look. Thanks! Its a large CL so please feel free to let me know if you want to discuss parts of it. Also, the design doc has an added section in the end which describes some of the scenarios that lead to either a transaction destruction from SharedWriters or SharedWriters destruction. I will be working on some more UMA/logging and upload it in a follow up CL/patch.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Just some super early feedback. PTAL at the request to move the SharedWriters class out into its own file with its own _unittests.cpp. That will also make reviewing easier. https://codereview.chromium.org/2519473002/diff/120001/chrome/browser/devtool... File chrome/browser/devtools/devtools_network_transaction.h (right): https://codereview.chromium.org/2519473002/diff/120001/chrome/browser/devtool... chrome/browser/devtools/devtools_network_transaction.h:60: void Orphan(std::unique_ptr<HttpTransaction> trans) override; Avoid abbreviating variable names, prefer http_transaction. http_cache_transaction uses 'trans' in places, but that should get cleaned up one day. https://codereview.chromium.org/2519473002/diff/120001/net/base/load_states_l... File net/base/load_states_list.h (right): https://codereview.chromium.org/2519473002/diff/120001/net/base/load_states_l... net/base/load_states_list.h:44: // requests will be deferred until the writing completes or the writer Can you omit 'or the write transactions compelte'? It seems like "the writing completes" covers that. https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache.h File net/http/http_cache.h (right): https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache.h#... net/http/http_cache.h:254: class SharedWriters { Let's put this class in a new file and give it its own unittest file. https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache.h#... net/http/http_cache.h:280: int Read(IOBuffer* buf, It's against style to have an output argument (the IOBuffer) before input arguments. But.. I guess it should stay since that's how the other Read() methods are. https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache.h#... net/http/http_cache.h:284: bool& read_in_progress); If it's an output variable, prefer to pass as bool* https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache.h#... net/http/http_cache.h:393: // Notifies the waiting_writers_ of the result, by posting a task for each There should be a blank line above comments to aid readability. https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache.h#... net/http/http_cache.h:418: ActiveEntry* entry_; Document that the ActiveEntry* owns this. https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache.h#... net/http/http_cache.h:470: // Used to keep information that ehtry should be destroyed later in the s/ehtry/entry/ https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache.h#... net/http/http_cache.h:471: // flow. iIt cannot be destroyed at this moment because doomed_writer_ needs s/iIt/It/ https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache.h#... net/http/http_cache.h:491: TransactionSet readers; Is it okay to lose the ordering by changing from list to set? https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache.h#... net/http/http_cache.h:549: // null, like writer, readers, shared_writers, pending_queue. s/null/nullptr/ https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache.h#... net/http/http_cache.h:638: void DestroyEntryAndRestartPendingQueueTxns(ActiveEntry* entry); s/Txns/Transactions/ https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache_tr... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:644: // A few common patterns: (Foo* means Foo -> FooComplete) Need to update this comment with a typical flow that uses the new shared states https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache_tr... File net/http/http_cache_transaction.h (right): https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache_tr... net/http/http_cache_transaction.h:171: // transaction should wait till the whole response is in the cache. s/till/until/ https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache_tr... net/http/http_cache_transaction.h:174: // - todo: Is this required: It is not a no-store data or data that must not s/todo:/TODO(shivanisha):/ https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache_tr... net/http/http_cache_transaction.h:450: void SaveNetworkTransInfo(); s/Trans/Transaction/ https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache_tr... net/http/http_cache_transaction.h:451: void SaveSharedNetworkTransInfo(); ditto https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache_tr... net/http/http_cache_transaction.h:507: // Reads from the network using a SharedWriters object. empty line above comment missing. https://codereview.chromium.org/2519473002/diff/120001/net/http/http_transact... File net/http/http_transaction.h (right): https://codereview.chromium.org/2519473002/diff/120001/net/http/http_transact... net/http/http_transaction.h:75: // Ownership of the transaction is transferred from the consumer to the s/from the consumer to the transaction/to the transaction itself/. https://codereview.chromium.org/2519473002/diff/120001/net/http/http_transact... net/http/http_transaction.h:78: virtual void Orphan(std::unique_ptr<HttpTransaction> trans) = 0; Let's move this method to the bottom of the public API as this is about cleanup and not the primary interface readers are looking for.
One thought that I keep coming back to: Could the SharedWriters be renamed to SharedNetworkTransaction and have it inherit from NetworkTransaction? It seems to mostly be a transaction already. Then, replace HttpCache::Transaction::network_trans_ with the shared writer version when it's time to share. That should remove a bunch of code duplication (e.g., copy from entry->shared if sharing else from network_trans_). I believe you could then delete the new states from the HttpCache::Transaction by just having it call network_trans_->Read() per usual and let the SharedNetworkTransaction update the other shared transactions with the new data. WDYT? I'm probably missing something important. https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache_tr... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:3387: if (orphaned && finalize_doomed_) { finalized_doomed_ should cause a crash if this object is deleted during DoLoop correct? https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:3388: cache_->FinalizeDoomedSharedWriter(entry_); Also entry_
The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2519473002/diff/120001/chrome/browser/devtool... File chrome/browser/devtools/devtools_network_transaction.h (right): https://codereview.chromium.org/2519473002/diff/120001/chrome/browser/devtool... chrome/browser/devtools/devtools_network_transaction.h:60: void Orphan(std::unique_ptr<HttpTransaction> trans) override; On 2016/12/06 at 18:08:17, jkarlin wrote: > Avoid abbreviating variable names, prefer http_transaction. http_cache_transaction uses 'trans' in places, but that should get cleaned up one day. Done in all the new code. https://codereview.chromium.org/2519473002/diff/120001/net/base/load_states_l... File net/base/load_states_list.h (right): https://codereview.chromium.org/2519473002/diff/120001/net/base/load_states_l... net/base/load_states_list.h:44: // requests will be deferred until the writing completes or the writer On 2016/12/06 at 18:08:17, jkarlin wrote: > Can you omit 'or the write transactions compelte'? It seems like "the writing completes" covers that. Done. Also rephrased the last sentence to be "This may be done to optimize for cache reuse in some cases or because the waiting requests are read-only." https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache.h File net/http/http_cache.h (right): https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache.h#... net/http/http_cache.h:254: class SharedWriters { On 2016/12/06 at 18:08:17, jkarlin wrote: > Let's put this class in a new file and give it its own unittest file. Good idea, done. https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache.h#... net/http/http_cache.h:280: int Read(IOBuffer* buf, On 2016/12/06 at 18:08:18, jkarlin wrote: > It's against style to have an output argument (the IOBuffer) before input arguments. But.. I guess it should stay since that's how the other Read() methods are. Good to know. Will leave intact in this case, though, for consistency. https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache.h#... net/http/http_cache.h:284: bool& read_in_progress); On 2016/12/06 at 18:08:17, jkarlin wrote: > If it's an output variable, prefer to pass as bool* done. https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache.h#... net/http/http_cache.h:393: // Notifies the waiting_writers_ of the result, by posting a task for each On 2016/12/06 at 18:08:17, jkarlin wrote: > There should be a blank line above comments to aid readability. Done. https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache.h#... net/http/http_cache.h:418: ActiveEntry* entry_; On 2016/12/06 at 18:08:17, jkarlin wrote: > Document that the ActiveEntry* owns this. Done https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache.h#... net/http/http_cache.h:470: // Used to keep information that ehtry should be destroyed later in the On 2016/12/06 at 18:08:18, jkarlin wrote: > s/ehtry/entry/ done https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache.h#... net/http/http_cache.h:471: // flow. iIt cannot be destroyed at this moment because doomed_writer_ needs On 2016/12/06 at 18:08:18, jkarlin wrote: > s/iIt/It/ done https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache.h#... net/http/http_cache.h:491: TransactionSet readers; On 2016/12/06 at 18:08:18, jkarlin wrote: > Is it okay to lose the ordering by changing from list to set? Yes, since readers are only waiting for read to be invoked on them. There is no role they play that requires ordering. https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache.h#... net/http/http_cache.h:549: // null, like writer, readers, shared_writers, pending_queue. On 2016/12/06 at 18:08:17, jkarlin wrote: > s/null/nullptr/ done https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache.h#... net/http/http_cache.h:638: void DestroyEntryAndRestartPendingQueueTxns(ActiveEntry* entry); On 2016/12/06 at 18:08:17, jkarlin wrote: > s/Txns/Transactions/ done https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache_tr... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:3387: if (orphaned && finalize_doomed_) { On 2016/12/06 at 21:06:28, jkarlin wrote: > finalized_doomed_ should cause a crash if this object is deleted during DoLoop correct? The underlying assumption here is that since orphaned being true implies there is no consumer , thus this object cannot be destroyed. If it is false, the next condition will not be checked anyways. https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:3388: cache_->FinalizeDoomedSharedWriter(entry_); On 2016/12/06 at 21:06:28, jkarlin wrote: > Also entry_ Same as above. https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache_tr... File net/http/http_cache_transaction.h (right): https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache_tr... net/http/http_cache_transaction.h:171: // transaction should wait till the whole response is in the cache. On 2016/12/06 at 18:08:18, jkarlin wrote: > s/till/until/ done. https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache_tr... net/http/http_cache_transaction.h:174: // - todo: Is this required: It is not a no-store data or data that must not On 2016/12/06 at 18:08:18, jkarlin wrote: > s/todo:/TODO(shivanisha):/ todo comment not needed, removed. https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache_tr... net/http/http_cache_transaction.h:450: void SaveNetworkTransInfo(); On 2016/12/06 at 18:08:18, jkarlin wrote: > s/Trans/Transaction/ done https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache_tr... net/http/http_cache_transaction.h:451: void SaveSharedNetworkTransInfo(); On 2016/12/06 at 18:08:18, jkarlin wrote: > ditto done https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache_tr... net/http/http_cache_transaction.h:507: // Reads from the network using a SharedWriters object. On 2016/12/06 at 18:08:18, jkarlin wrote: > empty line above comment missing. Added. Also added comments for finalize_doomed_ and orphaned_. https://codereview.chromium.org/2519473002/diff/120001/net/http/http_transact... File net/http/http_transaction.h (right): https://codereview.chromium.org/2519473002/diff/120001/net/http/http_transact... net/http/http_transaction.h:75: // Ownership of the transaction is transferred from the consumer to the On 2016/12/06 at 18:08:18, jkarlin wrote: > s/from the consumer to the transaction/to the transaction itself/. done. https://codereview.chromium.org/2519473002/diff/120001/net/http/http_transact... net/http/http_transaction.h:78: virtual void Orphan(std::unique_ptr<HttpTransaction> trans) = 0; On 2016/12/06 at 18:08:18, jkarlin wrote: > Let's move this method to the bottom of the public API as this is about cleanup and not the primary interface readers are looking for. done.
On 2016/12/06 at 21:53:35, shivanisha wrote: > https://codereview.chromium.org/2519473002/diff/120001/chrome/browser/devtool... > File chrome/browser/devtools/devtools_network_transaction.h (right): > > https://codereview.chromium.org/2519473002/diff/120001/chrome/browser/devtool... > chrome/browser/devtools/devtools_network_transaction.h:60: void Orphan(std::unique_ptr<HttpTransaction> trans) override; > On 2016/12/06 at 18:08:17, jkarlin wrote: > > Avoid abbreviating variable names, prefer http_transaction. http_cache_transaction uses 'trans' in places, but that should get cleaned up one day. > > Done in all the new code. > > https://codereview.chromium.org/2519473002/diff/120001/net/base/load_states_l... > File net/base/load_states_list.h (right): > > https://codereview.chromium.org/2519473002/diff/120001/net/base/load_states_l... > net/base/load_states_list.h:44: // requests will be deferred until the writing completes or the writer > On 2016/12/06 at 18:08:17, jkarlin wrote: > > Can you omit 'or the write transactions compelte'? It seems like "the writing completes" covers that. > > Done. Also rephrased the last sentence to be "This may be done to optimize for cache reuse in some cases or because the waiting requests are read-only." > > https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache.h > File net/http/http_cache.h (right): > > https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache.h#... > net/http/http_cache.h:254: class SharedWriters { > On 2016/12/06 at 18:08:17, jkarlin wrote: > > Let's put this class in a new file and give it its own unittest file. > > Good idea, done. > > https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache.h#... > net/http/http_cache.h:280: int Read(IOBuffer* buf, > On 2016/12/06 at 18:08:18, jkarlin wrote: > > It's against style to have an output argument (the IOBuffer) before input arguments. But.. I guess it should stay since that's how the other Read() methods are. > > Good to know. Will leave intact in this case, though, for consistency. > > https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache.h#... > net/http/http_cache.h:284: bool& read_in_progress); > On 2016/12/06 at 18:08:17, jkarlin wrote: > > If it's an output variable, prefer to pass as bool* > > done. > > https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache.h#... > net/http/http_cache.h:393: // Notifies the waiting_writers_ of the result, by posting a task for each > On 2016/12/06 at 18:08:17, jkarlin wrote: > > There should be a blank line above comments to aid readability. > > Done. > > https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache.h#... > net/http/http_cache.h:418: ActiveEntry* entry_; > On 2016/12/06 at 18:08:17, jkarlin wrote: > > Document that the ActiveEntry* owns this. > > Done > > https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache.h#... > net/http/http_cache.h:470: // Used to keep information that ehtry should be destroyed later in the > On 2016/12/06 at 18:08:18, jkarlin wrote: > > s/ehtry/entry/ > > done > > https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache.h#... > net/http/http_cache.h:471: // flow. iIt cannot be destroyed at this moment because doomed_writer_ needs > On 2016/12/06 at 18:08:18, jkarlin wrote: > > s/iIt/It/ > > done > > https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache.h#... > net/http/http_cache.h:491: TransactionSet readers; > On 2016/12/06 at 18:08:18, jkarlin wrote: > > Is it okay to lose the ordering by changing from list to set? > > Yes, since readers are only waiting for read to be invoked on them. There is no role they play that requires ordering. > > https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache.h#... > net/http/http_cache.h:549: // null, like writer, readers, shared_writers, pending_queue. > On 2016/12/06 at 18:08:17, jkarlin wrote: > > s/null/nullptr/ > > done > > https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache.h#... > net/http/http_cache.h:638: void DestroyEntryAndRestartPendingQueueTxns(ActiveEntry* entry); > On 2016/12/06 at 18:08:17, jkarlin wrote: > > s/Txns/Transactions/ > > done > > https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache_tr... > File net/http/http_cache_transaction.cc (right): > > https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache_tr... > net/http/http_cache_transaction.cc:3387: if (orphaned && finalize_doomed_) { > On 2016/12/06 at 21:06:28, jkarlin wrote: > > finalized_doomed_ should cause a crash if this object is deleted during DoLoop correct? > > The underlying assumption here is that since orphaned being true implies there is no consumer , thus this object cannot be destroyed. If it is false, the next condition will not be checked anyways. > > https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache_tr... > net/http/http_cache_transaction.cc:3388: cache_->FinalizeDoomedSharedWriter(entry_); > On 2016/12/06 at 21:06:28, jkarlin wrote: > > Also entry_ > > Same as above. > > https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache_tr... > File net/http/http_cache_transaction.h (right): > > https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache_tr... > net/http/http_cache_transaction.h:171: // transaction should wait till the whole response is in the cache. > On 2016/12/06 at 18:08:18, jkarlin wrote: > > s/till/until/ > > done. > > https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache_tr... > net/http/http_cache_transaction.h:174: // - todo: Is this required: It is not a no-store data or data that must not > On 2016/12/06 at 18:08:18, jkarlin wrote: > > s/todo:/TODO(shivanisha):/ > > todo comment not needed, removed. > > https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache_tr... > net/http/http_cache_transaction.h:450: void SaveNetworkTransInfo(); > On 2016/12/06 at 18:08:18, jkarlin wrote: > > s/Trans/Transaction/ > > done > > https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache_tr... > net/http/http_cache_transaction.h:451: void SaveSharedNetworkTransInfo(); > On 2016/12/06 at 18:08:18, jkarlin wrote: > > ditto > > done > > https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache_tr... > net/http/http_cache_transaction.h:507: // Reads from the network using a SharedWriters object. > On 2016/12/06 at 18:08:18, jkarlin wrote: > > empty line above comment missing. > > Added. Also added comments for finalize_doomed_ and orphaned_. > > https://codereview.chromium.org/2519473002/diff/120001/net/http/http_transact... > File net/http/http_transaction.h (right): > > https://codereview.chromium.org/2519473002/diff/120001/net/http/http_transact... > net/http/http_transaction.h:75: // Ownership of the transaction is transferred from the consumer to the > On 2016/12/06 at 18:08:18, jkarlin wrote: > > s/from the consumer to the transaction/to the transaction itself/. > > done. > > https://codereview.chromium.org/2519473002/diff/120001/net/http/http_transact... > net/http/http_transaction.h:78: virtual void Orphan(std::unique_ptr<HttpTransaction> trans) = 0; > On 2016/12/06 at 18:08:18, jkarlin wrote: > > Let's move this method to the bottom of the public API as this is about cleanup and not the primary interface readers are looking for. > > done. I have checked and fixed for usage of acronyms like 'trans' for 'transaction' only in the new code in http_cache.h, http_cache.cc, devtools_network_transaction.h and failing_http_transaction_factory.cc. Will do so for other files as well.
https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache_tr... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:3388: cache_->FinalizeDoomedSharedWriter(entry_); On 2016/12/06 at 21:53:35, shivanisha wrote: > On 2016/12/06 at 21:06:28, jkarlin wrote: > > Also entry_ > > Same as above. For clarity, I will add a comment for this assumption here and also make 'if (orphaned)' a separate condition.
https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache_tr... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:3387: if (orphaned && finalize_doomed_) { On 2016/12/06 21:53:35, shivanisha wrote: > On 2016/12/06 at 21:06:28, jkarlin wrote: > > finalized_doomed_ should cause a crash if this object is deleted during DoLoop > correct? > > The underlying assumption here is that since orphaned being true implies there > is no consumer , thus this object cannot be destroyed. If it is false, the next > condition will not be checked anyways. Makes sense, thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2016/12/06 at 21:06:28, jkarlin wrote: > One thought that I keep coming back to: > > Could the SharedWriters be renamed to SharedNetworkTransaction and have it inherit from NetworkTransaction? It seems to mostly be a transaction already. Then, replace HttpCache::Transaction::network_trans_ with the shared writer version when it's time to share. That should remove a bunch of code duplication (e.g., copy from entry->shared if sharing else from network_trans_). > > I believe you could then delete the new states from the HttpCache::Transaction by just having it call network_trans_->Read() per usual and let the SharedNetworkTransaction update the other shared transactions with the new data. > > WDYT? I'm probably missing something important. I do see advantage of having it as a derived class of HttpNetworkTransaction in simplifying the code around invoking Read and the various getter functions. But a lot of callbacks in SharedWriters e.g. OnCacheWriteSuccess are actually invoked through HttpCache e.g. CacheWriteSuccessSharedWriters because that is an owner of both entry and in turn of entry->shared_writers, thus making destruction of these objects cleaner in these callbacks. So in these callbacks, we would not benefit from a generic HttpTransaction object. Also these callbacks are public API in SharedWriters currently so either we will have to make them APIs in HttpTransaction(which is not desirable since they are specific to SharedWriters) or make HttpCache and SharedWriters friend classes. We will also require a different reference model, something like a raw pointer of HttpTransaction in HttpCache::Transaction which is in turn owned as a unique_ptr by ActiveEntry in case of shared but by the transaction itself in case of non-shared. Since SharedWriters is a collection of HttpCache::Transactions and owns a network transaction, it feels less like "being a" HttpNetworkTransaction and more like a manager/coordinating class at the same level as ActiveEntry and thus deriving it from HttpNetworkTransaction, seems a little odd. Given these design concerns, I feel more inclined to not make it a derived class of HttpNetworkTransaction. WDYT? > > https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache_tr... > File net/http/http_cache_transaction.cc (right): > > https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache_tr... > net/http/http_cache_transaction.cc:3387: if (orphaned && finalize_doomed_) { > finalized_doomed_ should cause a crash if this object is deleted during DoLoop correct? > > https://codereview.chromium.org/2519473002/diff/120001/net/http/http_cache_tr... > net/http/http_cache_transaction.cc:3388: cache_->FinalizeDoomedSharedWriter(entry_); > Also entry_
Created patch 3 which changes CacheWriteFailedSharedWriters to return a std::unique_ptr instead of a raw pointer so that it is clear that it is tranfering ownership.
The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run
Patchset #5 (id:200001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
As per in-person and mail discussions on the earlier CL, this new patch eliminates the need of transferring the ownership of HttpCache::Transaction and allows it to be destroyed whenever its consumer destroys it. This is achieved using a new class DataAccess. The design document has a section on this: https://docs.google.com/document/d/1KHT2sQ2ASTs-_t1xvptn2anUDx7FOo8Dz8NKpkuWt... PTAL, Thanks!
On 2017/01/12 at 20:10:19, shivanisha wrote: > As per in-person and mail discussions on the earlier CL, this new patch eliminates the need of transferring the ownership of HttpCache::Transaction and allows it to be destroyed whenever its consumer destroys it. This is achieved using a new class DataAccess. The design document has a section on this: > https://docs.google.com/document/d/1KHT2sQ2ASTs-_t1xvptn2anUDx7FOo8Dz8NKpkuWt... > > PTAL, Thanks! I spent a fair bit of time looking into why some of the content_browsertests are timing out but its still unclear. The symptom is that content::TestNavigationObserver::OnDidStopLoading is not getting called for the navigations in those tests. Worth noting though, that locally when I run one of those tests on master, it sometimes hangs with the same reason but the hang is inconsistent in master. I will continue to look into this and update what I find.
shivanisha@chromium.org changed reviewers: + davidben@chromium.org
Adding David. davidben@, PTAL, Thanks!
Shivani: I think the biggest bang for the buck for me on this review is to try to orient towards high level comments this week. There are a lot of nits below, but you should feel free to ignore them, as I suspect the overall shape of the code will change enough that a lot of them won't be relevant anymore once we get down to the home stretch. (Having said that, it's probably worth *reading* them, as I do make some general suggestions in them that you might want to consider.) The problem with high level comments, of course, is that there may well be some problems in the details that I'm not seeing that will render the high level ideas broken. So please take all of the following as being proposals for which I'm interested in your evaluation. * As discussed by VC, I'm concerned by SharedWriters having control of its own destruction timing--I think it makes reasoning about shutdown and when we can assume all resources associated with a set of requests have been torn down much harder (not impossible, but harder). To summarize the VC conversation for other folks: The reason why shared writers needs to handle its own destruction timing is that it's making fixed callbacks into the HttpCache (which may result in triggering its own destruction) but needs to call the callback stored by a HttpCache::Transaction consumer after it comes out of the DoLoop(). So basically, it's got two consumers, one of which it handles abstractly by that consumer passing in a callback, and the other (which controls its lifetime, conceptually at least) it calls concrete methods on. The way to handle this we came up with was to wrap the HC::T provided callback with a static function that would call into the HC if calls into the HC that might result in deletion occurred. That would allow the SharedWriters object to simply have to protect itself against deletion after the callback upcall, which is an unfortunately common idiom of the network stack. * Would it be possible to encapsulate the validation processing on the SharedWriters object? It feels like a strange split of responsibility to have each HC::T responsible for doing its own validation, but SharedWriters responsible for ordering those validations. * DataAccess seems like it does very few things: It owns the HttpNetworkTransaction, it provides a simple async callback wrapper around reading from the network transaction, and it provides a simple async callback interface around writing to the cache, and it has DCHECKs in place to make sure that only one of those two things is ever happening at any time. Especially given that as best I can tell all the states for each of those things are present in SharedWriters, it seems like the code would be noticeably shorter and somewhat simpler if SharedWriters just included the functionality of DataAccess. * As I mention below, I'd like to explore whether it makes sense to encapsulate writing to the cache inside of SharedWriters rather than having the current_writer_ (if there is one) call back in to write to the cache. Why isn't the interface between HC::T and SharedWriters purely a "Read()" (sync/async as usual) interface which either reads from the network, writes to the cache, and returns/calls back, or waits for the currently running network read to complete, copies the data across, and calls back? * Currently HC::T sometimes calls into HttpCache and sometimes calls into SharedWriters (indirecting through the active entry). That makes sense to me, and I don't think we have a good way around it. But I think it would be a useful encapsulation invariant if, if the HC::T was in the shared state, it always called methods on the SharedWriters object, and that object managed the interactions with the cache. Would that work? * Related to the above, I find myself wondering if there's a better way to express the functionality on HttpCache embodied by the *SharedWriters() methods that have been added. Some of those methods are called from HC::T when it's in the Shared state (so I find myself wondering if they couldn't be on the SharedWriters object), and the ones that aren't appropriate for the SharedWriters object presumably are manipulating the state of the ActiveEntry or some other aspect of the cache, and so could be described without reference to SharedWriters? * I'm confused by the friend class decls in DataAccess. Those classes are the only things that have access to a DataAccess object (?), so I would assume that the interface on the DataAccess object was designed for the needs of those classes. Given that, why declare those classes as friends? To put it somewhat differently, I'm not sure what the point of having a private: decl is if that friend declaration is necessary (but I suspect it's not, with the appropriate interface tweaks). I hope the above is useful. I should be able to do at least one more round trip on this review this week at this level, so please feel free to respond to the above comments (or grab me for a VC) and we can figure out which (if any :-}) of them make sense. Thanks! https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache.h File net/http/http_cache.h (right): https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache.h#... net/http/http_cache.h:23: #include <utility> What is this include for? I think of <utility> as being for algorithms, which I wouldn't think would be needed in the header. https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache.h#... net/http/http_cache.h:438: // To me, it would make it more clear that these comments apply to all the following routines if there was a blank line between the and the first one; my gut instinct about this formatting is that the comment applies just to the next routine. https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_da... File net/http/http_cache_data_access.cc (right): https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_da... net/http/http_cache_data_access.cc:69: DCHECK(callback_.is_null()); Suggestion: Group these by state invariants (i.e. DCHECKs that involve the member variables) and argument invariants (DCHECKs that involve function arguments). https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_da... net/http/http_cache_data_access.cc:69: DCHECK(callback_.is_null()); Both the state invariants and argument invariants are part of the interface contract for this function, and arguably should be documented in the header file. I say "arguably" because some of them strike me as both obvious and according to network stack idiom, but some generalizations ("Must not be called while a previous Read() is outstanding", maybe also the other requirements) should be gestured at there. https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_da... net/http/http_cache_data_access.cc:71: read_buf_ = std::move(buf); Should some assertion about read_buf_ being null be part of the DCHECKs above? https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_da... net/http/http_cache_data_access.cc:90: return result; Worthwhile nulling out read_buf_ and maybe io_buf_len_ here? https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_da... File net/http/http_cache_data_access.h (right): https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_da... net/http/http_cache_data_access.h:31: ActiveEntry* entry); I think it's important to document the lifetime requirements on the consumer of objects passed by raw pointers and stored in a persistent object. "Consumer must ensure that |*entry| outlives this." is plenty. https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_da... net/http/http_cache_data_access.h:64: CompletionCallback io_callback_; Suggestion: This is currently set in the constructor and not changed for the lifetime of the class, so a) it can be const (though that might require setting it in the constructor through construction rather than assignment), and b) it could just be eliminated and written out at each location. I've vote in favor of the latter; it's more text, but it's less lookup for future readers of the code. https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_sh... File net/http/http_cache_shared_writers.cc (right): https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_sh... net/http/http_cache_shared_writers.cc:51: validating_transaction_ = transaction; How hard would it be to move the validation work into SharedWriters? It feels weird to have SharedWriters tracking the validation queue, but not actually manage the validation. https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_sh... net/http/http_cache_shared_writers.cc:123: *read_in_progress = true; So this strikes me as problematic API design--along one asynchronous path out of this routine (no current_writer_), the callback passed in is called, and along the other (this one) we instead delegate to the cache to call a callback associated with the transaction. Both pathways should use the same callback mechanism; they're both telling the consumer "Your data is ready". https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_sh... net/http/http_cache_shared_writers.cc:171: next_state_ = STATE_CACHE_WRITE_DATA; Thought: It occurs to me that it might be useful from a performance POV to race writing the data back to the cache with returning it to the consumer, but it complicates the state machine. OTOH, it might make the responsibilities between the consumer and the SharedWriters object simpler; the consumer(s) just call Read() as appropriate and get responses back, and doesn't need to worry about writing data to the Cache; SharedWriters takes care of all that and hides the details from the consumer. I'm inclined to think this is worth exploring if it makes the SharedWriters API simpler, and not if it doesn't, but I haven't gotten far enough with this CL to evaluate that. https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_sh... net/http/http_cache_shared_writers.cc:205: destroy_ = true; Hmmm. Setting destroy_ to true means that on the next OnIOComplete() after its set, *after* the DoLoop call, the object will be destroyed. To me this means that destroy_ should only be set at the same time as an async method is dispatched from this object, so that you can be sure that OnIOComplete() will be called. But if this routine is being executed inside of a DoLoop invoked by an async response from, e.g., writing the cache response info, it seems like the object will leak. What am I missing? https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_sh... net/http/http_cache_shared_writers.cc:605: SelfDestroy(); Worth a DCHECK that DoLoop() didn't return ERR_IO_PENDING? Or is that ok? https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_sh... net/http/http_cache_shared_writers.cc:608: void HttpCache::SharedWriters::SelfDestroy() { I don't think there's a need for a separate function. https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_sh... File net/http/http_cache_shared_writers.h (right): https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_sh... net/http/http_cache_shared_writers.h:36: // - StopCaching API is invoked and caching was successfully stopped. Suggestion: My model for class documentation is that you document the requirements the class has on the consumers, and the behavior the class provides to the consumers. This documentation is describing how the consumers use the class, which I think of as better being with the consumers than here. Having said that, these classes are all intertwined, and I could imagine that this is the best place for the documentation on how this class is used--I haven't gotten far enough into this review to say definitively that it isn't. But in general (and possibly in specific) I'd recommend sticking to the "requirements on consumers + behavior provided to consumers" in class documentation, and figuring out a different place to put documentation on how a particular class is used. The idea is that how classes are used should be able to be changed without changing the class itself, and structuring the documentation that way helps that goal (by making the interface definition more precise). https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_sh... net/http/http_cache_shared_writers.h:46: std::unique_ptr<HttpTransaction> network_transaction); Recommend documenting lifetimes for raw pointers (I.e. requirements the class has on its consumer). https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_sh... net/http/http_cache_shared_writers.h:228: State next_state_ = STATE_NONE; I think it's good to use either all in-declaration initializers, or all at-constructor initializers; mixing them means that readers have to look in two different places (and remember to look in two different places) for where members are initialized. https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_sh... net/http/http_cache_shared_writers.h:275: WaitingWritersList waiting_writers_; Suggestion: I'm finding the reader/writer nomenclature a bit confusing in context. "SharedWriters" is about writing to the cache, but really, the SharedWriters object (and the DataAccess object it owns) is doing the writing. Everything other than the current_writer_ will never write to the cache (and the current_writer_ won't either, conceptually; the SharedWriters object will). Maybe switch over to active_transaction_ and waiting_for_read_transactions_ (naming by what the consumer is trying to do), https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_tr... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:496: cache_->StopCachingSharedWriters(this, entry_); Why not delegate to entry_->shared_writers? That seems like it's better encapsulation than going through the cache.
https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache.h File net/http/http_cache.h (right): https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache.h#... net/http/http_cache.h:23: #include <utility> On 2017/01/19 at 00:53:42, Randy Smith - Not in Mondays wrote: > What is this include for? I think of <utility> as being for algorithms, which I wouldn't think would be needed in the header. Included this as per git cl lint. I agree it's not needed, though. Do you suggest to ignore those lint messages? https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_sh... File net/http/http_cache_shared_writers.cc (right): https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_sh... net/http/http_cache_shared_writers.cc:51: validating_transaction_ = transaction; On 2017/01/19 at 00:53:43, Randy Smith - Not in Mondays wrote: > How hard would it be to move the validation work into SharedWriters? It feels weird to have SharedWriters tracking the validation queue, but not actually manage the validation. Moving the validation work would mean moving the logic of states after STATE_ADD_TO_ENTRY_COMPLETE and before STATE_NETWORK_READ. We probably do not want all that to be part of SW anyways. Conceptually I look at SW as mainly pertaining to response body read/write but it also needs to keep track of any simultaneous transaction in the headers phase to enable that transaction to ultimately join the shared writing. https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_sh... net/http/http_cache_shared_writers.cc:123: *read_in_progress = true; On 2017/01/19 at 00:53:43, Randy Smith - Not in Mondays wrote: > So this strikes me as problematic API design--along one asynchronous path out of this routine (no current_writer_), the callback passed in is called, and along the other (this one) we instead delegate to the cache to call a callback associated with the transaction. Both pathways should use the same callback mechanism; they're both telling the consumer "Your data is ready". The difference in the design between the 2 cases is because in this case, its possible that SW is destroyed after posting the tasks. Since we can be sure of HttpCache being alive the responsibility is delegated to it. I wanted to avoid invoking callbacks as a function call instead of posting individual tasks for all transactions to avoid the complexities of going through the state flows of various transactions in a single control flow. https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_tr... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:496: cache_->StopCachingSharedWriters(this, entry_); On 2017/01/19 at 00:53:43, Randy Smith - Not in Mondays wrote: > Why not delegate to entry_->shared_writers? That seems like it's better encapsulation than going through the cache. Going through HttpCache in this case basically ensures simplicity of destruction of SW (reset invoked by the owner HttpCache::ActiveEntry). For consistency, though, I can see it going through SW and before it returns, it will be reset by its owner.
On 2017/01/19 21:13:06, shivanisha wrote: > https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache.h > File net/http/http_cache.h (right): > > https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache.h#... > net/http/http_cache.h:23: #include <utility> > On 2017/01/19 at 00:53:42, Randy Smith - Not in Mondays wrote: > > What is this include for? I think of <utility> as being for algorithms, which > I wouldn't think would be needed in the header. > > Included this as per git cl lint. I agree it's not needed, though. Do you > suggest to ignore those lint messages? > > https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_sh... > File net/http/http_cache_shared_writers.cc (right): > > https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_sh... > net/http/http_cache_shared_writers.cc:51: validating_transaction_ = transaction; > On 2017/01/19 at 00:53:43, Randy Smith - Not in Mondays wrote: > > How hard would it be to move the validation work into SharedWriters? It feels > weird to have SharedWriters tracking the validation queue, but not actually > manage the validation. > > Moving the validation work would mean moving the logic of states after > STATE_ADD_TO_ENTRY_COMPLETE and before STATE_NETWORK_READ. We probably do not > want all that to be part of SW anyways. Conceptually I look at SW as mainly > pertaining to response body read/write but it also needs to keep track of any > simultaneous transaction in the headers phase to enable that transaction to > ultimately join the shared writing. > > https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_sh... > net/http/http_cache_shared_writers.cc:123: *read_in_progress = true; > On 2017/01/19 at 00:53:43, Randy Smith - Not in Mondays wrote: > > So this strikes me as problematic API design--along one asynchronous path out > of this routine (no current_writer_), the callback passed in is called, and > along the other (this one) we instead delegate to the cache to call a callback > associated with the transaction. Both pathways should use the same callback > mechanism; they're both telling the consumer "Your data is ready". > > The difference in the design between the 2 cases is because in this case, its > possible that SW is destroyed after posting the tasks. Since we can be sure of > HttpCache being alive the responsibility is delegated to it. I wanted to avoid > invoking callbacks as a function call instead of posting individual tasks for > all transactions to avoid the complexities of going through the state flows of > various transactions in a single control flow. > > https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_tr... > File net/http/http_cache_transaction.cc (right): > > https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_tr... > net/http/http_cache_transaction.cc:496: cache_->StopCachingSharedWriters(this, > entry_); > On 2017/01/19 at 00:53:43, Randy Smith - Not in Mondays wrote: > > Why not delegate to entry_->shared_writers? That seems like it's better > encapsulation than going through the cache. > > Going through HttpCache in this case basically ensures simplicity of destruction > of SW (reset invoked by the owner HttpCache::ActiveEntry). For consistency, > though, I can see it going through SW and before it returns, it will be reset by > its owner. Just wanting to confirm you'll be responding to my top level comments as well? Those are really the important ones.
On 2017/01/19 at 21:13:06, shivanisha wrote: > https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache.h > File net/http/http_cache.h (right): > > https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache.h#... > net/http/http_cache.h:23: #include <utility> > On 2017/01/19 at 00:53:42, Randy Smith - Not in Mondays wrote: > > What is this include for? I think of <utility> as being for algorithms, which I wouldn't think would be needed in the header. > > Included this as per git cl lint. I agree it's not needed, though. Do you suggest to ignore those lint messages? > > https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_sh... > File net/http/http_cache_shared_writers.cc (right): > > https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_sh... > net/http/http_cache_shared_writers.cc:51: validating_transaction_ = transaction; > On 2017/01/19 at 00:53:43, Randy Smith - Not in Mondays wrote: > > How hard would it be to move the validation work into SharedWriters? It feels weird to have SharedWriters tracking the validation queue, but not actually manage the validation. > > Moving the validation work would mean moving the logic of states after STATE_ADD_TO_ENTRY_COMPLETE and before STATE_NETWORK_READ. We probably do not want all that to be part of SW anyways. Conceptually I look at SW as mainly pertaining to response body read/write but it also needs to keep track of any simultaneous transaction in the headers phase to enable that transaction to ultimately join the shared writing. > > https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_sh... > net/http/http_cache_shared_writers.cc:123: *read_in_progress = true; > On 2017/01/19 at 00:53:43, Randy Smith - Not in Mondays wrote: > > So this strikes me as problematic API design--along one asynchronous path out of this routine (no current_writer_), the callback passed in is called, and along the other (this one) we instead delegate to the cache to call a callback associated with the transaction. Both pathways should use the same callback mechanism; they're both telling the consumer "Your data is ready". > > The difference in the design between the 2 cases is because in this case, its possible that SW is destroyed after posting the tasks. Since we can be sure of HttpCache being alive the responsibility is delegated to it. I wanted to avoid invoking callbacks as a function call instead of posting individual tasks for all transactions to avoid the complexities of going through the state flows of various transactions in a single control flow. > > https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_tr... > File net/http/http_cache_transaction.cc (right): > > https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_tr... > net/http/http_cache_transaction.cc:496: cache_->StopCachingSharedWriters(this, entry_); > On 2017/01/19 at 00:53:43, Randy Smith - Not in Mondays wrote: > > Why not delegate to entry_->shared_writers? That seems like it's better encapsulation than going through the cache. > > Going through HttpCache in this case basically ensures simplicity of destruction of SW (reset invoked by the owner HttpCache::ActiveEntry). For consistency, though, I can see it going through SW and before it returns, it will be reset by its owner. Its strange. I wrote up the response to the high level comments and press "send message" and the message is lost :( Well, I will write it again.
> Its strange. I wrote up the response to the high level comments and press
"send
> message" and the message is lost :(
> Well, I will write it again.
Oh, that sucks :-{.
Thanks Randy for the initial comments. Responses to the high level comments are inline and I have responded to some of the low level comments in an earlier message and will get to the rest. Shivani: I think the biggest bang for the buck for me on this review is to try to orient towards high level comments this week. There are a lot of nits below, but you should feel free to ignore them, as I suspect the overall shape of the code will change enough that a lot of them won't be relevant anymore once we get down to the home stretch. (Having said that, it's probably worth *reading* them, as I do make some general suggestions in them that you might want to consider.) The problem with high level comments, of course, is that there may well be some problems in the details that I'm not seeing that will render the high level ideas broken. So please take all of the following as being proposals for which I'm interested in your evaluation. * As discussed by VC, I'm concerned by SharedWriters having control of its own destruction timing--I think it makes reasoning about shutdown and when we can assume all resources associated with a set of requests have been torn down much harder (not impossible, but harder). To summarize the VC conversation for other folks: The reason why shared writers needs to handle its own destruction timing is that it's making fixed callbacks into the HttpCache (which may result in triggering its own destruction) but needs to call the callback stored by a HttpCache::Transaction consumer after it comes out of the DoLoop(). So basically, it's got two consumers, one of which it handles abstractly by that consumer passing in a callback, and the other (which controls its lifetime, conceptually at least) it calls concrete methods on. The way to handle this we came up with was to wrap the HC::T provided callback with a static function that would call into the HC if calls into the HC that might result in deletion occurred. That would allow the SharedWriters object to simply have to protect itself against deletion after the callback upcall, which is an unfortunately common idiom of the network stack. - Will work on this and update on how it goes. * Would it be possible to encapsulate the validation processing on the SharedWriters object? It feels like a strange split of responsibility to have each HC::T responsible for doing its own validation, but SharedWriters responsible for ordering those validations. - Moving the validation work would mean moving the logic of states between STATE_ADD_TO_ENTRY_COMPLETE and STATE_NETWORK_READ to SW. We do not want all that to be part of SW. Conceptually I look at SW as mainly pertaining to "response body" read/write but it also needs to keep track of any simultaneous transaction in the "headers" phase to enable that transaction to ultimately join the shared writing. * DataAccess seems like it does very few things: It owns the HttpNetworkTransaction, it provides a simple async callback wrapper around reading from the network transaction, and it provides a simple async callback interface around writing to the cache, and it has DCHECKs in place to make sure that only one of those two things is ever happening at any time. Especially given that as best I can tell all the states for each of those things are present in SharedWriters, it seems like the code would be noticeably shorter and somewhat simpler if SharedWriters just included the functionality of DataAccess. - DataAccess needs to be a separate class to enable either HC::T (for non shared transactions) or SW (for shared transactions) to be its owner and consumer. It's a thin layer meant to encapsulate the network transaction, entry_ and the functionalities on them for response body reading and cache writing so it basically just needs to keep the network transaction, entry and any other transient state like read buffer, buffer length etc. * As I mention below, I'd like to explore whether it makes sense to encapsulate writing to the cache inside of SharedWriters rather than having the current_writer_ (if there is one) call back in to write to the cache. Why isn't the interface between HC::T and SharedWriters purely a "Read()" (sync/async as usual) interface which either reads from the network, writes to the cache, and returns/calls back, or waits for the currently running network read to complete, copies the data across, and calls back? - It would work for success cases but for failure case, HC::T would not know if it was a network read failure or cache write failure. It needs to handle both failures differently. In case of network read failure , it should consider it as a failure and return to consumer and in case of cache write failure, it doesn't tell the consumer about it since it can continue reading the response from the network. * Currently HC::T sometimes calls into HttpCache and sometimes calls into SharedWriters (indirecting through the active entry). That makes sense to me, and I don't think we have a good way around it. But I think it would be a useful encapsulation invariant if, if the HC::T was in the shared state, it always called methods on the SharedWriters object, and that object managed the interactions with the cache. Would that work? - That would work, the only thing is that in some cases SW will be reset just before the SW function returned. * Related to the above, I find myself wondering if there's a better way to express the functionality on HttpCache embodied by the *SharedWriters() methods that have been added. Some of those methods are called from HC::T when it's in the Shared state (so I find myself wondering if they couldn't be on the SharedWriters object), and the ones that aren't appropriate for the SharedWriters object presumably are manipulating the state of the ActiveEntry or some other aspect of the cache, and so could be described without reference to SharedWriters? - As mentioned in the above response all HC::*SharedWriters functions can be invoked from SW and HC::T always invokes the SW function, given that we are fine with SW getting reset at the end of the function. The only function that would remain in HC will be CreateSharedWriters which should be fine. * I'm confused by the friend class decls in DataAccess. Those classes are the only things that have access to a DataAccess object (?), so I would assume that the interface on the DataAccess object was designed for the needs of those classes. Given that, why declare those classes as friends? To put it somewhat differently, I'm not sure what the point of having a private: decl is if that friend declaration is necessary (but I suspect it's not, with the appropriate interface tweaks). - The only reason I kept the friend class decls in DataAccess was to directly access data_access_->network_transaction_ from SW and entry_->shared_writers->data_access_->network_transaction_ from HC::T instead of calling a network_transaction() function or making the network_transaction_ public. Would you suggest the latter?
https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_sh... File net/http/http_cache_shared_writers.cc (right): https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_sh... net/http/http_cache_shared_writers.cc:171: next_state_ = STATE_CACHE_WRITE_DATA; I remember talking to others about it and that SimplaCache actually does not wait for data to be written to the cache before returning. I will confirm with Gavin, though. https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_sh... File net/http/http_cache_shared_writers.h (right): https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_sh... net/http/http_cache_shared_writers.h:275: WaitingWritersList waiting_writers_; active_transaction_ and waiting_for_read_ sound good to me. How about all_writers_? I might want to keep all_writers_ as is because it does not include the transactions in validation phase, so making it all_transactions_ will be confusing.
The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
* As discussed by VC, I'm concerned by SharedWriters having control of its own destruction timing--I think it makes reasoning about shutdown and when we can assume all resources associated with a set of requests have been torn down much harder (not impossible, but harder). To summarize the VC conversation for other folks: The reason why shared writers needs to handle its own destruction timing is that it's making fixed callbacks into the HttpCache (which may result in triggering its own destruction) but needs to call the callback stored by a HttpCache::Transaction consumer after it comes out of the DoLoop(). So basically, it's got two consumers, one of which it handles abstractly by that consumer passing in a callback, and the other (which controls its lifetime, conceptually at least) it calls concrete methods on. The way to handle this we came up with was to wrap the HC::T provided callback with a static function that would call into the HC if calls into the HC that might result in deletion occurred. That would allow the SharedWriters object to simply have to protect itself against deletion after the callback upcall, which is an unfortunately common idiom of the network stack. - The latest patch removes the need for Shared Writers to delete itself and cache_ can reset it when needed. The design includes allowing some SW::Do* functions to invoke cache_ functions which might destroy the object and they explicitly confirm it by setting the argument destroyed to true. This makes it clear upfront which Do* functions can lead to the object being destroyed and that after the state loop , one should always check the boolean destroyed if there is a need to access any member variable. I have also commented for both Do* and callback_ that this object may be deleted in them. * I'm confused by the friend class decls in DataAccess. Those classes are the only things that have access to a DataAccess object (?), so I would assume that the interface on the DataAccess object was designed for the needs of those classes. Given that, why declare those classes as friends? To put it somewhat differently, I'm not sure what the point of having a private: decl is if that friend declaration is necessary (but I suspect it's not, with the appropriate interface tweaks). - The only reason I kept the friend class decls in DataAccess was to directly access data_access_->network_transaction_ from SW and entry_->shared_writers->data_access_->network_transaction_ from HC::T instead of calling a network_transaction() function or making the network_transaction_ public. Would you suggest the latter? - On second thoughts, I think keeping a network_transaction() function should be fine.
On 2017/01/19 21:38:37, shivanisha wrote: > Thanks Randy for the initial comments. Responses to the high level comments are > inline and I have responded to some of the low level comments in an earlier > message and will get to the rest. > > Shivani: I think the biggest bang for the buck for me on this review is to try > to orient towards high level comments this week. There are a lot of nits below, > but you should feel free to ignore them, as I suspect the overall shape of the > code will change enough that a lot of them won't be relevant anymore once we get > down to the home stretch. (Having said that, it's probably worth *reading* > them, as I do make some general suggestions in them that you might want to > consider.) > > The problem with high level comments, of course, is that there may well be some > problems in the details that I'm not seeing that will render the high level > ideas broken. So please take all of the following as being proposals for which > I'm interested in your evaluation. > > * As discussed by VC, I'm concerned by SharedWriters having control of its own > destruction timing--I think it makes reasoning about shutdown and when we can > assume all resources associated with a set of requests have been torn down much > harder (not impossible, but harder). To summarize the VC conversation for other > folks: The reason why shared writers needs to handle its own destruction timing > is that it's making fixed callbacks into the HttpCache (which may result in > triggering its own destruction) but needs to call the callback stored by a > HttpCache::Transaction consumer after it comes out of the DoLoop(). So > basically, it's got two consumers, one of which it handles abstractly by that > consumer passing in a callback, and the other (which controls its lifetime, > conceptually at least) it calls concrete methods on. The way to handle this we > came up with was to wrap the HC::T provided callback with a static function that > would call into the HC if calls into the HC that might result in deletion > occurred. That would allow the SharedWriters object to simply have to protect > itself against deletion after the callback upcall, which is an unfortunately > common idiom of the network stack. > - Will work on this and update on how it goes. > > * Would it be possible to encapsulate the validation processing on the > SharedWriters object? It feels like a strange split of responsibility to have > each HC::T responsible for doing its own validation, but SharedWriters > responsible for ordering those validations. > - Moving the validation work would mean moving the logic of states between > STATE_ADD_TO_ENTRY_COMPLETE and STATE_NETWORK_READ to SW. We do not want all > that to be part of SW. Conceptually I look at SW as mainly pertaining to > "response body" read/write but it also needs to keep track of any simultaneous > transaction in the "headers" phase to enable that transaction to ultimately join > the shared writing. > > * DataAccess seems like it does very few things: It owns the > HttpNetworkTransaction, it provides a simple async callback wrapper around > reading from the network transaction, and it provides a simple async callback > interface around writing to the cache, and it has DCHECKs in place to make sure > that only one of those two things is ever happening at any time. Especially > given that as best I can tell all the states for each of those things are > present in SharedWriters, it seems like the code would be noticeably shorter and > somewhat simpler if SharedWriters just included the functionality of DataAccess. > - DataAccess needs to be a separate class to enable either HC::T (for non shared > transactions) or SW (for shared transactions) to be its owner and consumer. It's > a thin layer meant to encapsulate the network transaction, entry_ and the > functionalities on them for response body reading and cache writing so it > basically just needs to keep the network transaction, entry and any other > transient state like read buffer, buffer length etc. But HC::T and SW both have the entry_ already, and the functionalities you're talking about are very thin wrappers. Why can this not be handled by just passing the network transaction ownership where you would pass the DA ownership? > * As I mention below, I'd like to explore whether it makes sense to encapsulate > writing to the cache inside of SharedWriters rather than having the > current_writer_ (if there is one) call back in to write to the cache. Why isn't > the interface between HC::T and SharedWriters purely a "Read()" (sync/async as > usual) interface which either reads from the network, writes to the cache, and > returns/calls back, or waits for the currently running network read to complete, > copies the data across, and calls back? > - It would work for success cases but for failure case, HC::T would not know if > it was a network read failure or cache write failure. It needs to handle both > failures differently. In case of network read failure > , it should consider it as a failure and return to consumer and in case of > cache write failure, it doesn't tell the consumer about it since it can continue > reading the response from the network. Got it. That makes sense. Drat. > > * Currently HC::T sometimes calls into HttpCache and sometimes calls into > SharedWriters (indirecting through the active entry). That makes sense to me, > and I don't think we have a good way around it. But I think it would be a > useful encapsulation invariant if, if the HC::T was in the shared state, it > always called methods on the SharedWriters object, and that object managed the > interactions with the cache. Would that work? > - That would work, the only thing is that in some cases SW will be reset just > before the SW function returned. Gotcha. Up to you--I still like the encapsulation invariant, but I don't like the image of our accessing entry_->shared_writers at one point in a function and that indirection being invalid at a different point in that same function. I think both patterns increase the maintenance burden. I wish we could come up with a better encapsulation/layering/framing. > * Related to the above, I find myself wondering if there's a better way to > express the functionality on HttpCache embodied by the *SharedWriters() methods > that have been added. Some of those methods are called from HC::T when it's in > the Shared state (so I find myself wondering if they couldn't be on the > SharedWriters object), and the ones that aren't appropriate for the > SharedWriters object presumably are manipulating the state of the ActiveEntry or > some other aspect of the cache, and so could be described without reference to > SharedWriters? > - As mentioned in the above response all HC::*SharedWriters functions can be > invoked from SW and HC::T always invokes the SW function, given that we are fine > with SW getting reset at the end of the function. The > only function that would remain in HC will be CreateSharedWriters which should > be fine. Probably that's the right way to go. I'd recommend including comments about the places where entry_->shared_writers may become null. > * I'm confused by the friend class decls in DataAccess. Those classes are the > only things that have access to a DataAccess object (?), so I would assume that > the interface on the DataAccess object was designed for the needs of those > classes. Given that, why declare those classes as friends? To put it somewhat > differently, I'm not sure what the point of having a private: decl is if that > friend declaration is necessary (but I suspect it's not, with the appropriate > interface tweaks). > - The only reason I kept the friend class decls in DataAccess was to directly > access data_access_->network_transaction_ from SW and > entry_->shared_writers->data_access_->network_transaction_ from HC::T instead > of calling a network_transaction() function or making the network_transaction_ > public. Would you suggest the latter? I'd actually suggest just wrapping it in an accessor. The interface on DA is for HC::T and SW, so there's no reason to include friend declarations; just make the interface whatever the consumers need.
https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache.h File net/http/http_cache.h (right): https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache.h#... net/http/http_cache.h:23: #include <utility> On 2017/01/19 21:13:06, shivanisha wrote: > On 2017/01/19 at 00:53:42, Randy Smith - Not in Mondays wrote: > > What is this include for? I think of <utility> as being for algorithms, which > I wouldn't think would be needed in the header. > > Included this as per git cl lint. I agree it's not needed, though. Do you > suggest to ignore those lint messages? Weird. No, I don't think it's overriding an automated tool, and hey, maybe it knows something I don't. If you feel inspired, you could file a bug on the tool, but that's up to you :-}. https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_sh... File net/http/http_cache_shared_writers.cc (right): https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_sh... net/http/http_cache_shared_writers.cc:51: validating_transaction_ = transaction; On 2017/01/19 21:13:06, shivanisha wrote: > On 2017/01/19 at 00:53:43, Randy Smith - Not in Mondays wrote: > > How hard would it be to move the validation work into SharedWriters? It feels > weird to have SharedWriters tracking the validation queue, but not actually > manage the validation. > > Moving the validation work would mean moving the logic of states after > STATE_ADD_TO_ENTRY_COMPLETE and before STATE_NETWORK_READ. We probably do not > want all that to be part of SW anyways. Conceptually I look at SW as mainly > pertaining to response body read/write but it also needs to keep track of any > simultaneous transaction in the headers phase to enable that transaction to > ultimately join the shared writing. Ok .... I can only repeat that this feels weird to me, since it means the responsibilities between the classes aren't sharply defined (SW has a synchronization responsibility around the validation requests but doesn't actually do the validation requests, but it does manage the network transaction for the read/write portion of the process). I'm not going to be in a position to dig down into what the implementation is and should be, so I won't make any definitive statements as to what the right thing to do is, but I'd recommend looking at pulling the synchronization among the validation requests out of SharedWriters (can ActiveEntry do it? Does it need to be serialized, or can you simply race many different network transactions?) or moving the managing of the validation into SharedWriters. But up to you and the other reviewers. https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_sh... net/http/http_cache_shared_writers.cc:123: *read_in_progress = true; On 2017/01/19 21:13:06, shivanisha wrote: > On 2017/01/19 at 00:53:43, Randy Smith - Not in Mondays wrote: > > So this strikes me as problematic API design--along one asynchronous path out > of this routine (no current_writer_), the callback passed in is called, and > along the other (this one) we instead delegate to the cache to call a callback > associated with the transaction. Both pathways should use the same callback > mechanism; they're both telling the consumer "Your data is ready". > > The difference in the design between the 2 cases is because in this case, its > possible that SW is destroyed after posting the tasks. Since we can be sure of > HttpCache being alive the responsibility is delegated to it. I wanted to avoid > invoking callbacks as a function call instead of posting individual tasks for > all transactions to avoid the complexities of going through the state flows of > various transactions in a single control flow. Let's talk this through in a VC--I'm having a lot of reactions, the #1 being that it's still really not the right API and we should find a way to fix it. But the SW must be alive at the time of ProcessWaitingWriters, and if it posts a task consisting of just the passed in callback (i.e. one that isn't dependent on a SW weak pointer), there's no reason that shouldn't succeed. So there's something about this response I'm missing. https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_tr... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:496: cache_->StopCachingSharedWriters(this, entry_); On 2017/01/19 21:13:06, shivanisha wrote: > On 2017/01/19 at 00:53:43, Randy Smith - Not in Mondays wrote: > > Why not delegate to entry_->shared_writers? That seems like it's better > encapsulation than going through the cache. > > Going through HttpCache in this case basically ensures simplicity of destruction > of SW (reset invoked by the owner HttpCache::ActiveEntry). For consistency, > though, I can see it going through SW and before it returns, it will be reset by > its owner. Meaning that StopCachingSharedWriters() might destroy the SW? What I'm reaching for is restricted, minimal interfaces. Because we're layering HC::T on top of SW and it calls directly in in some cases, I'm trying to avoid it making calls related to SW directly to the cache in other cases. The smaller and more localized (to one object) the interface used by HC::T to manipulate SW is, the easier it'll be to reason about. Having said that, I'm not wedded to calling SW directly. And I won't be around to work through the details, so I'll leave them to your other reviewers. But I'd recommend trying to find some way to narrow the interface that HC::T uses to manipulate SW (e.g. always call methods on ActiveEntry, have a set of routines on HC that are called to manipulate SW (though I don't like that just because HC's a global), always call SW directly and make sure to handle destruction possibilities carefully).
The latest patch removes the need for Shared Writers to delete itself. Functions invoked on cache_ can destroy it whenever needed. I have added comments for both SW::Do* functions and callback_ to be able to destroy this object. Also, the Do* functions make it clear upfront as to which ones can delete this object using a boolean argument and that variable should be checked if any member variables are accessed after the state loop.
On 2017/01/20 20:07:30, shivanisha wrote: > The latest patch removes the need for Shared Writers to delete itself. Functions > invoked on cache_ can destroy it whenever needed. I have added comments for both > SW::Do* functions and callback_ to be able to destroy this object. Also, the Do* > functions make it clear upfront as to which ones can delete this object using a > boolean argument and that variable should be checked if any member variables are > accessed after the state loop. Rather than use the |bool *destroyed| woven throughout the code, how about expanding the state machine so that states in which destruction happens are well defined & terminal (i.e. have a next_state_ = DONE in them). Shift the model from "destruction happens" to "notify the cache that the state machine has completed" and do that by assigning to a normally null callback the call into the HttpCache to notify it that completion has occurred. On the way out of the DoLoop, save both the consumer callback and that callback in local variables, and then call the cache callback if non-null (with a comment that this may destroy the object), then call the consumer callback. I think that'll produce much cleaner code; specifically, there will be a much smaller path length (2 lines?) over which the code inside the class needed to deal with the possibility it's destroyed.
> * DataAccess seems like it does very few things: It owns the > HttpNetworkTransaction, it provides a simple async callback wrapper around > reading from the network transaction, and it provides a simple async callback > interface around writing to the cache, and it has DCHECKs in place to make sure > that only one of those two things is ever happening at any time. Especially > given that as best I can tell all the states for each of those things are > present in SharedWriters, it seems like the code would be noticeably shorter and > somewhat simpler if SharedWriters just included the functionality of DataAccess. > - DataAccess needs to be a separate class to enable either HC::T (for non shared > transactions) or SW (for shared transactions) to be its owner and consumer. It's > a thin layer meant to encapsulate the network transaction, entry_ and the > functionalities on them for response body reading and cache writing so it > basically just needs to keep the network transaction, entry and any other > transient state like read buffer, buffer length etc. But HC::T and SW both have the entry_ already, and the functionalities you're talking about are very thin wrappers. Why can this not be handled by just passing the network transaction ownership where you would pass the DA ownership? Ah I see , you mean in shared flows the read happens through HC::T invoking SW::Read and in non shared flows, it happens as it does now directly through HC::T invoking Read on network transaction. That sounds good to me especially because its going to remove one layer completely. (A little bit of history: The motivation for it came during our initial discussions of removing Orphan API and having more abstraction. My thought was that this object will be able to completely encapsulate the ::Read call uptill writing to the cache but due to reasons mentioned in earlier messages we need to have all the states in HCT and SW anyways, so this layer is not giving us much.)
On 2017/01/20 20:07:30, shivanisha wrote: > The latest patch removes the need for Shared Writers to delete itself. Functions > invoked on cache_ can destroy it whenever needed. I have added comments for both > SW::Do* functions and callback_ to be able to destroy this object. Also, the Do* > functions make it clear upfront as to which ones can delete this object using a > boolean argument and that variable should be checked if any member variables are > accessed after the state loop. Rather than use the |bool *destroyed| woven throughout the code, how about expanding the state machine so that states in which destruction happens are well defined & terminal (i.e. have a next_state_ = DONE in them). Shift the model from "destruction happens" to "notify the cache that the state machine has completed" and do that by assigning to a normally null callback the call into the HttpCache to notify it that completion has occurred. On the way out of the DoLoop, save both the consumer callback and that callback in local variables, and then call the cache callback if non-null (with a comment that this may destroy the object), then call the consumer callback. I think that'll produce much cleaner code; specifically, there will be a much smaller path length (2 lines?) over which the code inside the class needed to deal with the possibility it's destroyed. Sounds good.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache.h File net/http/http_cache.h (right): https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache.h#... net/http/http_cache.h:23: #include <utility> Removed the include. https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache.h#... net/http/http_cache.h:438: // N/A due to recent changes. https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_da... File net/http/http_cache_data_access.cc (right): https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_da... net/http/http_cache_data_access.cc:69: DCHECK(callback_.is_null()); N/A since DataAccess class no longer exists. https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_da... net/http/http_cache_data_access.cc:71: read_buf_ = std::move(buf); N/A since DataAccess class no longer exists. https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_da... net/http/http_cache_data_access.cc:90: return result; N/A since DataAccess class no longer exists. https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_da... File net/http/http_cache_data_access.h (right): https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_da... net/http/http_cache_data_access.h:31: ActiveEntry* entry); On 2017/01/19 at 00:53:42, Randy Smith (OOO 1-21 - 2-10) wrote: > I think it's important to document the lifetime requirements on the consumer of objects passed by raw pointers and stored in a persistent object. "Consumer must ensure that |*entry| outlives this." is plenty. N/A here since DataAccess class no longer exists. Added similar comment in SharedWriters constructor. https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_da... net/http/http_cache_data_access.h:64: CompletionCallback io_callback_; N/A since DataAccess class no longer exists. https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_sh... File net/http/http_cache_shared_writers.cc (right): https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_sh... net/http/http_cache_shared_writers.cc:51: validating_transaction_ = transaction; I hear your concerns and that this means SW has an additional responsibility apart from synchronizing and managing the response body read. The validations need to be done one at a time so that one transaction can take the responsibility of dooming the entry and create a new SharedWriters object and subsequent transactions can be added to it as well. Having one transaction is in line with the invariant that the lock still exists till the headers are read. ActiveEntry has an invariant of at most 1 writer and moving this to ActiveEntry will violate that. It's also important for SW to be aware of these transactions so it can take certain decisions like whether to remain alive or not when the response is completely written based on whether there is a validating transaction or not. https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_sh... net/http/http_cache_shared_writers.cc:123: *read_in_progress = true; Done. Discussed this with Randy over VC that it is possible to use a CompletionCallback with PostTask. https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_sh... net/http/http_cache_shared_writers.cc:205: destroy_ = true; Yup, simplified the destruction logic s.t. destroy_ is no longer needed. https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_sh... net/http/http_cache_shared_writers.cc:605: SelfDestroy(); N/A now that self destruction is not needed. https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_sh... net/http/http_cache_shared_writers.cc:608: void HttpCache::SharedWriters::SelfDestroy() { On 2017/01/19 at 00:53:43, Randy Smith (OOO 1-21 - 2-10) wrote: > I don't think there's a need for a separate function. N/A now that self destruction is not needed. https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_sh... File net/http/http_cache_shared_writers.h (right): https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_sh... net/http/http_cache_shared_writers.h:36: // - StopCaching API is invoked and caching was successfully stopped. Agree, thanks. Updated the comments in this file to be independent of the implementation. https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_sh... net/http/http_cache_shared_writers.h:46: std::unique_ptr<HttpTransaction> network_transaction); On 2017/01/19 at 00:53:43, Randy Smith (OOO 1-21 - 2-10) wrote: > Recommend documenting lifetimes for raw pointers (I.e. requirements the class has on its consumer). done. https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_sh... net/http/http_cache_shared_writers.h:228: State next_state_ = STATE_NONE; I have tried to keep all simple initializations here and all object/collections initialization in the constructor. I prefer this approach since looking in the header file at the point of declaration is sufficient to make sure no variable is left uninitialized since complex types are initialized by their own constructors, anyways. Looking at the discussion here https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/zqB-DySA4V0, this seems inline with the official wording of google style guide "Use in-class member initialization for simple initializations, especially when a member variable must be initialized the same way in more than one constructor." Having said that, I am open to changing that if any of us strongly feel towards the other approach. https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_sh... net/http/http_cache_shared_writers.h:275: WaitingWritersList waiting_writers_; Done. https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_tr... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:496: cache_->StopCachingSharedWriters(this, entry_); Done. (Response in detail in the higher level feedback discussion)
https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_sh... File net/http/http_cache_shared_writers.cc (right): https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_sh... net/http/http_cache_shared_writers.cc:51: validating_transaction_ = transaction; Other reviewers, any thoughts on this?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
> * As I mention below, I'd like to explore whether it makes sense to encapsulate > writing to the cache inside of SharedWriters rather than having the > current_writer_ (if there is one) call back in to write to the cache. Why isn't > the interface between HC::T and SharedWriters purely a "Read()" (sync/async as > usual) interface which either reads from the network, writes to the cache, and > returns/calls back, or waits for the currently running network read to complete, > copies the data across, and calls back? > - It would work for success cases but for failure case, HC::T would not know if > it was a network read failure or cache write failure. It needs to handle both > failures differently. In case of network read failure > , it should consider it as a failure and return to consumer and in case of > cache write failure, it doesn't tell the consumer about it since it can continue > reading the response from the network. | Got it. That makes sense. Drat. Is it possible for the Read() call to return a useful error code to differentiate between network failure and cache failure? On Wed, Jan 25, 2017 at 11:50 AM <shivanisha@chromium.org> wrote: > > > https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_sh... > File net/http/http_cache_shared_writers.cc (right): > > > https://codereview.chromium.org/2519473002/diff/220001/net/http/http_cache_sh... > net/http/http_cache_shared_writers.cc:51: validating_transaction_ = > transaction; > Other reviewers, any thoughts on this? > > https://codereview.chromium.org/2519473002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/02/01 at 17:29:25, jkarlin wrote: > > * As I mention below, I'd like to explore whether it makes sense to > encapsulate > > writing to the cache inside of SharedWriters rather than having the > > current_writer_ (if there is one) call back in to write to the cache. Why > isn't > > the interface between HC::T and SharedWriters purely a "Read()" > (sync/async as > > usual) interface which either reads from the network, writes to the > cache, and > > returns/calls back, or waits for the currently running network read to > complete, > > copies the data across, and calls back? > > - It would work for success cases but for failure case, HC::T would not > know > if > > it was a network read failure or cache write failure. It needs to handle > both > > failures differently. In case of network read failure > > , it should consider it as a failure and return to consumer and in case of > > cache write failure, it doesn't tell the consumer about it since it can > continue > > reading the response from the network. > > | Got it. That makes sense. Drat. > > > Is it possible for the Read() call to return a useful error code to > differentiate between network failure and cache failure? > Network failure is currently result < 0 and cache failure is result != write_len_. In the cache failure case HCT needs to set result = write_len_ before returning to the consumer. So even if SharedWriters::Read() returns 2 separate error codes for the 2 cases, HCT will still need to know the write_len_.
Read() can be defined however we want right? It doesn't need to just return a single integer in its callback. On Wed, Feb 1, 2017 at 9:50 AM <shivanisha@chromium.org> wrote: > On 2017/02/01 at 17:29:25, jkarlin wrote: > > > * As I mention below, I'd like to explore whether it makes sense to > > encapsulate > > > writing to the cache inside of SharedWriters rather than having the > > > current_writer_ (if there is one) call back in to write to the cache. > Why > > isn't > > > the interface between HC::T and SharedWriters purely a "Read()" > > (sync/async as > > > usual) interface which either reads from the network, writes to the > > cache, and > > > returns/calls back, or waits for the currently running network read to > > complete, > > > copies the data across, and calls back? > > > - It would work for success cases but for failure case, HC::T would not > > know > > if > > > it was a network read failure or cache write failure. It needs to > handle > > both > > > failures differently. In case of network read failure > > > , it should consider it as a failure and return to consumer and in > case of > > > cache write failure, it doesn't tell the consumer about it since it can > > continue > > > reading the response from the network. > > > > | Got it. That makes sense. Drat. > > > > > > Is it possible for the Read() call to return a useful error code to > > differentiate between network failure and cache failure? > > > Network failure is currently result < 0 and cache failure is result != > write_len_. In the cache failure case HCT needs to set result = write_len_ > before returning to the consumer. So even if SharedWriters::Read() returns > 2 > separate error codes for the 2 cases, HCT will still need to know the > write_len_. > > > https://codereview.chromium.org/2519473002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/02/01 at 17:53:37, jkarlin wrote: > Read() can be defined however we want right? It doesn't need to just return > a single integer in its callback. > That would be deviating from the current pattern of HCT's io callback always being HttpCache::Transaction::OnIOComplete(int result) which invokes DoLoop(result) also with a single parameter.
On 2017/02/01 17:58:21, shivanisha wrote: > On 2017/02/01 at 17:53:37, jkarlin wrote: > > Read() can be defined however we want right? It doesn't need to just return > > a single integer in its callback. > > > That would be deviating from the current pattern of HCT's io callback always > being HttpCache::Transaction::OnIOComplete(int result) which invokes > DoLoop(result) also with a single parameter. We could also pass an output param to Read() that it writes to.
On 2017/02/01 at 18:42:54, jkarlin wrote: > On 2017/02/01 17:58:21, shivanisha wrote: > > On 2017/02/01 at 17:53:37, jkarlin wrote: > > > Read() can be defined however we want right? It doesn't need to just return > > > a single integer in its callback. > > > > > That would be deviating from the current pattern of HCT's io callback always > > being HttpCache::Transaction::OnIOComplete(int result) which invokes > > DoLoop(result) also with a single parameter. > > We could also pass an output param to Read() that it writes to. But SharedWriters::Read() will lead to asynchronous operations for network read and cache write, thus having an output parameter will not help.
Some early notes from a previous patchset. Sending before starting on the latest patchset. https://codereview.chromium.org/2519473002/diff/260001/net/base/load_states_l... File net/base/load_states_list.h (right): https://codereview.chromium.org/2519473002/diff/260001/net/base/load_states_l... net/base/load_states_list.h:42: // same resource, the requests eligible for shared writing among them will be I don't know that we want to mention shared writers in this file, as it's a low level detail. Perhaps: If multiple requests are made for the same resource, they'll typically run in parallel but in some circumstances they may have to defer until a previous request has completed. https://codereview.chromium.org/2519473002/diff/260001/net/http/http_cache.cc File net/http/http_cache.cc (right): https://codereview.chromium.org/2519473002/diff/260001/net/http/http_cache.cc... net/http/http_cache.cc:346: entry->shared_writers.reset(); Is this necessary? https://codereview.chromium.org/2519473002/diff/260001/net/http/http_cache.cc... net/http/http_cache.cc:848: // This is not expected to be called for shared writing transactions. Comment unnecessary https://codereview.chromium.org/2519473002/diff/260001/net/http/http_cache.h File net/http/http_cache.h (right): https://codereview.chromium.org/2519473002/diff/260001/net/http/http_cache.h#... net/http/http_cache.h:237: typedef std::unordered_set<Transaction*> TransactionSet; This and above should be using instead of typedef https://codereview.chromium.org/2519473002/diff/260001/net/http/http_cache.h#... net/http/http_cache.h:311: // null and containers to be empty: writer, shared_writers, pending_queue s/null/nullptr/ https://codereview.chromium.org/2519473002/diff/260001/net/http/http_cache_sh... File net/http/http_cache_shared_writers.h (right): https://codereview.chromium.org/2519473002/diff/260001/net/http/http_cache_sh... net/http/http_cache_shared_writers.h:175: enum State { The enum declaration should come before the private method declarations. Also, prefer enum class to enum for type safety, unless you need it to work with another enum or write it to a histogram. https://codereview.chromium.org/2519473002/diff/260001/net/http/http_cache_sh... net/http/http_cache_shared_writers.h:217: struct WaitingForRead { struct should be at beginning of private: https://codereview.chromium.org/2519473002/diff/260001/net/http/http_cache_sh... net/http/http_cache_shared_writers.h:230: typedef std::list<WaitingForRead> WaitingForReadList; using WaitingForReadList = std::list<WaitingForRead>; We avoid typedef in new code. https://codereview.chromium.org/2519473002/diff/260001/net/http/http_cache_tr... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/2519473002/diff/260001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:250: next_state_ <= STATE_ADD_TO_ENTRY_COMPLETE) { Any chance these conditionals could be moved into a switch statement? That way if anyone changes the states in the future they'll have to update the switch as well which is safer. https://codereview.chromium.org/2519473002/diff/260001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:2061: // Consumer need not know that it was a failure since this transaction can Not a sentence. https://codereview.chromium.org/2519473002/diff/260001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:2077: // If its a cache write success, read_buf_ would have been filled Fix comment formatting https://codereview.chromium.org/2519473002/diff/260001/net/http/http_cache_tr... File net/http/http_cache_transaction.h (right): https://codereview.chromium.org/2519473002/diff/260001/net/http/http_cache_tr... net/http/http_cache_transaction.h:120: base::WeakPtr<Transaction> GetWeakPtr() { return weak_factory_.GetWeakPtr(); } Chrome convention is to call it AsWeakPtr() https://codereview.chromium.org/2519473002/diff/260001/net/http/http_cache_tr... net/http/http_cache_transaction.h:179: Note that methods in .cc file should be defined in the same order that they're declared in the header. I see that they don't match here (e.g., I see ResetShared is defined earlier in the cc than SetShared). https://codereview.chromium.org/2519473002/diff/260001/net/http/http_cache_tr... net/http/http_cache_transaction.h:192: void SetSharedWritingFailState(int result); It sounds like (from the comment) that this is a setter for next_state_, but it does more than that. https://codereview.chromium.org/2519473002/diff/260001/net/http/http_cache_tr... net/http/http_cache_transaction.h:251: // Update the value if another state becomes the last state of the start Better to put this comment on top of the enum. People are more likely to look there. https://codereview.chromium.org/2519473002/diff/260001/net/http/http_response... File net/http/http_response_info.h (right): https://codereview.chromium.org/2519473002/diff/260001/net/http/http_response... net/http/http_response_info.h:80: // Request joins the already created SharedWriters. Do we need these two new states? They don't seem congruous with the rest of the enum. IE ENTRY_JOIN_SHARED_WRITING essentially is a subset of ENTRY_USED? https://codereview.chromium.org/2519473002/diff/260001/net/http/http_response... net/http/http_response_info.h:84: ENTRY_DOOM_SHARED_WRITING, If you edit this enum you need to edit the enum in histograms.xml accordingly. https://codereview.chromium.org/2519473002/diff/260001/net/http/http_transact... File net/http/http_transaction.h (right): https://codereview.chromium.org/2519473002/diff/260001/net/http/http_transact... net/http/http_transaction.h:123: // If there are multiple requests for this resource that are writing to the I don't think this addition is necessary. It already states that this request won't continue to write to the cache. It doesn't say that others won't.
https://codereview.chromium.org/2519473002/diff/260001/net/base/load_states_l... File net/base/load_states_list.h (right): https://codereview.chromium.org/2519473002/diff/260001/net/base/load_states_l... net/base/load_states_list.h:42: // same resource, the requests eligible for shared writing among them will be Sounds good. https://codereview.chromium.org/2519473002/diff/260001/net/http/http_cache.cc File net/http/http_cache.cc (right): https://codereview.chromium.org/2519473002/diff/260001/net/http/http_cache.cc... net/http/http_cache.cc:346: entry->shared_writers.reset(); DeactivateEntry has multiple dchecks to assert that entry does not contain any non null transaction pointers (including shared_writers) since DeactivateEntry is invoked at multiple places, not just in the destructor. https://codereview.chromium.org/2519473002/diff/260001/net/http/http_cache.cc... net/http/http_cache.cc:848: // This is not expected to be called for shared writing transactions. Removed https://codereview.chromium.org/2519473002/diff/260001/net/http/http_cache.h File net/http/http_cache.h (right): https://codereview.chromium.org/2519473002/diff/260001/net/http/http_cache.h#... net/http/http_cache.h:237: typedef std::unordered_set<Transaction*> TransactionSet; done. https://codereview.chromium.org/2519473002/diff/260001/net/http/http_cache.h#... net/http/http_cache.h:311: // null and containers to be empty: writer, shared_writers, pending_queue done. https://codereview.chromium.org/2519473002/diff/260001/net/http/http_cache_sh... File net/http/http_cache_shared_writers.h (right): https://codereview.chromium.org/2519473002/diff/260001/net/http/http_cache_sh... net/http/http_cache_shared_writers.h:175: enum State { done https://codereview.chromium.org/2519473002/diff/260001/net/http/http_cache_sh... net/http/http_cache_shared_writers.h:217: struct WaitingForRead { done https://codereview.chromium.org/2519473002/diff/260001/net/http/http_cache_sh... net/http/http_cache_shared_writers.h:230: typedef std::list<WaitingForRead> WaitingForReadList; done https://codereview.chromium.org/2519473002/diff/260001/net/http/http_cache_tr... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/2519473002/diff/260001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:250: next_state_ <= STATE_ADD_TO_ENTRY_COMPLETE) { That's true. Changed to switch with only *Complete states as valid states when the destructor can be invoked. https://codereview.chromium.org/2519473002/diff/260001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:2061: // Consumer need not know that it was a failure since this transaction can Restored to the original comment. https://codereview.chromium.org/2519473002/diff/260001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:2077: // If its a cache write success, read_buf_ would have been filled done https://codereview.chromium.org/2519473002/diff/260001/net/http/http_cache_tr... File net/http/http_cache_transaction.h (right): https://codereview.chromium.org/2519473002/diff/260001/net/http/http_cache_tr... net/http/http_cache_transaction.h:120: base::WeakPtr<Transaction> GetWeakPtr() { return weak_factory_.GetWeakPtr(); } In net/ I see many instances of GetWeakPtr but none for AsWeakPtr, although in the rest of the chrome code base there are many instances of AsWeakPtr. Keeping it as GetWeakPtr for consistency within net. https://codereview.chromium.org/2519473002/diff/260001/net/http/http_cache_tr... net/http/http_cache_transaction.h:179: Re-ordered https://codereview.chromium.org/2519473002/diff/260001/net/http/http_cache_tr... net/http/http_cache_transaction.h:192: void SetSharedWritingFailState(int result); Updated the comment. https://codereview.chromium.org/2519473002/diff/260001/net/http/http_cache_tr... net/http/http_cache_transaction.h:251: // Update the value if another state becomes the last state of the start Don't need this anymore since RemoveTransactionFromSharedWriters is now using a switch statement. https://codereview.chromium.org/2519473002/diff/260001/net/http/http_response... File net/http/http_response_info.h (right): https://codereview.chromium.org/2519473002/diff/260001/net/http/http_response... net/http/http_response_info.h:80: // Request joins the already created SharedWriters. ENTRY_JOIN_SHARED_WRITING is different from ENTRY_USED because ENTRY_USED signifies that the complete response was read from the cache and network was not accessed. Sum of HttpCache.AccessToDone.JoinSharedWriting and HttpCache.AccessToDone.InitiateSharedWriting will give the count of shared writing transactions out of a total of "not cached" and "updated". ENTRY_DOOM_SHARED_WRITING is different from ENTRY_UPDATED because it means that some transactions are still reading from the shared writing entry while these transactions access the response from the network. https://codereview.chromium.org/2519473002/diff/260001/net/http/http_response... net/http/http_response_info.h:84: ENTRY_DOOM_SHARED_WRITING, I cannot find the definition of HttpCache.AccessToDone being associated with an enum in histograms.xml. Am I missing something? https://codereview.chromium.org/2519473002/diff/260001/net/http/http_transact... File net/http/http_transaction.h (right): https://codereview.chromium.org/2519473002/diff/260001/net/http/http_transact... net/http/http_transaction.h:123: // If there are multiple requests for this resource that are writing to the Actually, if there are multiple transactions then its a no-op. Since every request in SharedWriters need to be able to write to the cache, one of them cannot stop writing.
Here are a few more https://codereview.chromium.org/2519473002/diff/280001/net/http/http_cache.cc File net/http/http_cache.cc (right): https://codereview.chromium.org/2519473002/diff/280001/net/http/http_cache.cc... net/http/http_cache.cc:816: if (entry->shared_writers->AddTransaction(trans)) { Perhaps SharedWriters::AddTransaction should return OK or ERR_IO_PENDING like HttpCache::AddTransactionToEntry so that you can just return entry->shared_writers->AddTransaction(trans); https://codereview.chromium.org/2519473002/diff/280001/net/http/http_cache.cc... net/http/http_cache.cc:1260: new HttpCache::SharedWriters(this, entry, cache_transaction, priority, How about a static SharedWriters::Create method to create the SharedWriters object and run Init? The Init function could call MoveFromPendingQueue and ProcessFirstWaitingValidation. That way MoveFromPendingQueue and ProcessFirstWaitingValidation wouldn't have to be public members of SharedWriters and you'd be guaranteed that the SharedWriters is properly initialized. https://codereview.chromium.org/2519473002/diff/280001/net/http/http_cache_sh... File net/http/http_cache_shared_writers.cc (right): https://codereview.chromium.org/2519473002/diff/280001/net/http/http_cache_sh... net/http/http_cache_shared_writers.cc:173: if (cache_->IsResponseCompleted(entry_, This checking if the response is completed is new isn't it? I'm not sure why this is necessary or correct. https://codereview.chromium.org/2519473002/diff/280001/net/http/http_cache_sh... net/http/http_cache_shared_writers.cc:509: transaction = validating_transaction_; Seems like you could cut out two lines: Transaction* transaction = validating_transaction_; if (!transaction) { .. } https://codereview.chromium.org/2519473002/diff/280001/net/http/http_cache_sh... File net/http/http_cache_shared_writers.h (right): https://codereview.chromium.org/2519473002/diff/280001/net/http/http_cache_sh... net/http/http_cache_shared_writers.h:13: It'd be good to have an overview of how SharedWriting works here. Not the SharedWriters class particularly but what the general flow is. https://codereview.chromium.org/2519473002/diff/280001/net/http/http_cache_sh... net/http/http_cache_shared_writers.h:59: // transferred to SharedWriters. But if there are other transactions, entry s/entry/the entry/ https://codereview.chromium.org/2519473002/diff/280001/net/http/http_cache_sh... net/http/http_cache_shared_writers.h:87: bool RemoveWaitingTransaction(Transaction* transaction); Looks like this is only for waiting for validation transactions. Can you rename this to be more clear? E.g., RemoveWaitingForValidationTransaction. https://codereview.chromium.org/2519473002/diff/280001/net/http/http_cache_sh... File net/http/http_cache_shared_writers_unittest.cc (right): https://codereview.chromium.org/2519473002/diff/280001/net/http/http_cache_sh... net/http/http_cache_shared_writers_unittest.cc:15: The helper functions should be under namespace { so, something like: namespace net { namespace { .. helper functions .. } // namespace tests here } // namespace net https://codereview.chromium.org/2519473002/diff/280001/net/http/http_cache_tr... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/2519473002/diff/280001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:817: // Delete line https://codereview.chromium.org/2519473002/diff/280001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:1903: // or if its a no-store response. Comment should be a full sentence. https://codereview.chromium.org/2519473002/diff/280001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:1912: // are initiating Shared Writing. shared writing probably shouldn't be capitalized https://codereview.chromium.org/2519473002/diff/280001/net/http/http_cache_tr... File net/http/http_cache_transaction.h (right): https://codereview.chromium.org/2519473002/diff/280001/net/http/http_cache_tr... net/http/http_cache_transaction.h:120: base::WeakPtr<Transaction> GetWeakPtr() { return weak_factory_.GetWeakPtr(); } Should be named AsWeakPtr() per Chrome convention https://codereview.chromium.org/2519473002/diff/300001/net/http/http_cache_sh... File net/http/http_cache_shared_writers.h (right): https://codereview.chromium.org/2519473002/diff/300001/net/http/http_cache_sh... net/http/http_cache_shared_writers.h:117: STATE_NONE, Now that it's an enum class and you always have to prefix with State:: you could remove the STATE_ part of each state since it's redundant. https://codereview.chromium.org/2519473002/diff/300001/net/http/http_cache_tr... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/2519473002/diff/300001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:281: default: Ah, I'm not sure that I appreciated just how large this enum is. Having the default case somewhat defeats the purpose of having the switch (causing compile errors when people add new states), though I suppose the switch is still more efficient and easier to read.
Addressed Josh's most recent feedback and also re-organized functions in http_cache_shared_writers.cc such that they are in the same order as http_cache_shared_writers.h https://codereview.chromium.org/2519473002/diff/280001/net/http/http_cache.cc File net/http/http_cache.cc (right): https://codereview.chromium.org/2519473002/diff/280001/net/http/http_cache.cc... net/http/http_cache.cc:816: if (entry->shared_writers->AddTransaction(trans)) { Done. https://codereview.chromium.org/2519473002/diff/280001/net/http/http_cache.cc... net/http/http_cache.cc:1260: new HttpCache::SharedWriters(this, entry, cache_transaction, priority, Sounds good. Updated except did not add an Init function and invoked MoveFromPendingQueue and ProcessFirstWaitingValidation from the constructor itself. https://codereview.chromium.org/2519473002/diff/280001/net/http/http_cache_sh... File net/http/http_cache_shared_writers.cc (right): https://codereview.chromium.org/2519473002/diff/280001/net/http/http_cache_sh... net/http/http_cache_shared_writers.cc:173: if (cache_->IsResponseCompleted(entry_, This checking is not new but was being done in HCT state machine in DoCacheWriteDataComplete here https://cs.chromium.org/chromium/src/net/http/http_cache_transaction.cc?q=htt.... I have added this function to do it now so both HCT and SW can call it. https://codereview.chromium.org/2519473002/diff/280001/net/http/http_cache_sh... net/http/http_cache_shared_writers.cc:509: transaction = validating_transaction_; Done. https://codereview.chromium.org/2519473002/diff/280001/net/http/http_cache_sh... File net/http/http_cache_shared_writers.h (right): https://codereview.chromium.org/2519473002/diff/280001/net/http/http_cache_sh... net/http/http_cache_shared_writers.h:13: Done. https://codereview.chromium.org/2519473002/diff/280001/net/http/http_cache_sh... net/http/http_cache_shared_writers.h:59: // transferred to SharedWriters. But if there are other transactions, entry done https://codereview.chromium.org/2519473002/diff/280001/net/http/http_cache_sh... net/http/http_cache_shared_writers.h:87: bool RemoveWaitingTransaction(Transaction* transaction); Done. https://codereview.chromium.org/2519473002/diff/280001/net/http/http_cache_sh... File net/http/http_cache_shared_writers_unittest.cc (right): https://codereview.chromium.org/2519473002/diff/280001/net/http/http_cache_sh... net/http/http_cache_shared_writers_unittest.cc:15: Done https://codereview.chromium.org/2519473002/diff/280001/net/http/http_cache_tr... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/2519473002/diff/280001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:817: // Done https://codereview.chromium.org/2519473002/diff/280001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:1903: // or if its a no-store response. Merged with the above comment. https://codereview.chromium.org/2519473002/diff/280001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:1912: // are initiating Shared Writing. Made lower case. https://codereview.chromium.org/2519473002/diff/280001/net/http/http_cache_tr... File net/http/http_cache_transaction.h (right): https://codereview.chromium.org/2519473002/diff/280001/net/http/http_cache_tr... net/http/http_cache_transaction.h:120: base::WeakPtr<Transaction> GetWeakPtr() { return weak_factory_.GetWeakPtr(); } Since net/ has GetWeakPtr and not AsWeakPtr, should I keep it that for consistency? https://codereview.chromium.org/2519473002/diff/300001/net/http/http_cache_sh... File net/http/http_cache_shared_writers.h (right): https://codereview.chromium.org/2519473002/diff/300001/net/http/http_cache_sh... net/http/http_cache_shared_writers.h:117: STATE_NONE, Done. https://codereview.chromium.org/2519473002/diff/300001/net/http/http_cache_tr... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/2519473002/diff/300001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:281: default: Yes, there are lots of states and this switch only contains *Complete states. Rest all the states and also non-shared states are in default case since they are not valid states in a shared transaction's destructor call. I agree switch is more readable and ensures correctness because anyone adding a new state will be forced to think (due to NOTREACHED in default) where the state should be added.
shivanisha@chromium.org changed reviewers: - davidben@chromium.org
I see a way that we can split this into something more manageable for review. Step 1) Focus on parallelizing validation. E.g., once the active writer has response headers, let the others do their validation. For transactions which can't use the same response, doom the active entry and let them continue. For transactions that can use the same response, make them wait. This work doesn't need any shared writing code and is an improvement over what we have today. Step 2) The transanctions, upon realizing they can use the same response, add themselves to a shared writing object and perform shared writing. One thing that I like about this approach (aside from splitting up the work) is that the shared writing only comes into play after validation by HttpCache::Transaction. So no validation logic is necessary in the SharedWriters. WDYT? https://codereview.chromium.org/2519473002/diff/280001/net/http/http_cache_tr... File net/http/http_cache_transaction.h (right): https://codereview.chromium.org/2519473002/diff/280001/net/http/http_cache_tr... net/http/http_cache_transaction.h:120: base::WeakPtr<Transaction> GetWeakPtr() { return weak_factory_.GetWeakPtr(); } On 2017/02/07 20:56:48, shivanisha wrote: > Since net/ has GetWeakPtr and not AsWeakPtr, should I keep it that for > consistency? > You're right. I'd been told in the past that AsWeakPtr() was the way Chrome does things but grep-wars prove otherwise. GetWeakPtr it is.
On 2017/02/16 at 14:35:08, jkarlin wrote: > I see a way that we can split this into something more manageable for review. > > Step 1) Focus on parallelizing validation. E.g., once the active writer has response headers, let the others do their validation. For transactions which can't use the same response, doom the active entry and let them continue. For transactions that can use the same response, make them wait. This work doesn't need any shared writing code and is an improvement over what we have today. > > Step 2) The transanctions, upon realizing they can use the same response, add themselves to a shared writing object and perform shared writing. > > One thing that I like about this approach (aside from splitting up the work) is that the shared writing only comes into play after validation by HttpCache::Transaction. So no validation logic is necessary in the SharedWriters. > > WDYT? > Splitting this work definitely seems right. Thanks for this suggestion. It also allows the change to be simpler since parallel validations will be allowed for all requests, not just for shared-eligible requests. CL-1 according to above suggestion is here: https://codereview.chromium.org/2721933002
Description was changed from ========== Fixes the cache lock issue such that multiple transactions can start consuming the response data via a single network transaction. Design document: https://docs.google.com/document/d/1KHT2sQ2ASTs-_t1xvptn2anUDx7FOo8Dz8NKpkuWt... BUG=472740 ========== to ========== Fixes the cache lock issue such that multiple transactions can start consuming the response data via a single network transaction. Design document: https://docs.google.com/document/d/1KHT2sQ2ASTs-_t1xvptn2anUDx7FOo8Dz8NKpkuWt... BUG=472740 ==========
jkarlin@chromium.org changed reviewers: - jkarlin@chromium.org
jkarlin@chromium.org changed reviewers: + jkarlin@chromium.org
Removed myself as reviewer since this CL is overcome by events.
On 2017/04/11 at 11:32:59, jkarlin wrote: > Removed myself as reviewer since this CL is overcome by events. Closing this CL since fixing the cache lock is now being done as part of three separate CLs: - Allowing parallel validation for all requests. CL https://codereview.chromium.org/2721933002 - Dooming and creating entry for requests that do not match during validation phase. CL https://codereview.chromium.org/2774603003/ - Allowing parallel writing. (Work in progress) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
