Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2740833004:
Reporting: Implement garbage collector. (Closed)
|
Created:
3 years, 9 months ago by Julia Tuttle Modified:
3 years, 8 months ago CC:
cbentzel+watch_chromium.org, chromium-reviews, net-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionReporting: Implement garbage collector.
Reporting is a spec for delivering out-of-band reports from various
other parts of the browser. See http://wicg.github.io/reporting/ for
the spec, or https://goo.gl/pygX5I for details of the planned
implementation in Chromium.
This implements the garbage collector, which cleans out reports that
haven't been delivered for too long or through too many attempts (and
also clears reports on network changes, if so configured).
BUG=704259
Review-Url: https://codereview.chromium.org/2740833004
Cr-Commit-Position: refs/heads/master@{#463359}
Committed: https://chromium.googlesource.com/chromium/src/+/9f970c02c1feef9d61eff3fd52db7253eab1d9d9
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : rebase #
Total comments: 4
Patch Set 4 : Make requested changes. #
Total comments: 1
Patch Set 5 : rebase #Patch Set 6 : Move after Context CL. #Patch Set 7 : Add scheduling, etc. #Patch Set 8 : rebase #
Total comments: 1
Patch Set 9 : Make GarbageCollector own Timer. #Patch Set 10 : rebase #
Messages
Total messages: 56 (35 generated)
Description was changed from ========== Reporting: Create GarbageCollector. ========== to ========== Reporting: Create GarbageCollector. Reporting is a spec for delivering out-of-band reports from various other parts of the browser. See http://wicg.github.io/reporting/. This implements the garbage collector, which cleans out reports that haven't been delivered for too long or through too many attempts (and also clears reports on network changes, if so configured). ==========
juliatuttle@chromium.org changed reviewers: + shivanisha@chromium.org
PTAL, shivanisha.
The CQ bit was checked by juliatuttle@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.
The CQ bit was checked by juliatuttle@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...
Description was changed from ========== Reporting: Create GarbageCollector. Reporting is a spec for delivering out-of-band reports from various other parts of the browser. See http://wicg.github.io/reporting/. This implements the garbage collector, which cleans out reports that haven't been delivered for too long or through too many attempts (and also clears reports on network changes, if so configured). ========== to ========== Reporting: Create garbage collector. Reporting is a spec for delivering out-of-band reports from various other parts of the browser. See http://wicg.github.io/reporting/. This implements the garbage collector, which cleans out reports that haven't been delivered for too long or through too many attempts (and also clears reports on network changes, if so configured). ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm % nits https://codereview.chromium.org/2740833004/diff/40001/net/reporting/reporting... File net/reporting/reporting_garbage_collector.cc (right): https://codereview.chromium.org/2740833004/diff/40001/net/reporting/reporting... net/reporting/reporting_garbage_collector.cc:20: bool network_changed) { DCHECK(cache) https://codereview.chromium.org/2740833004/diff/40001/net/reporting/reporting... net/reporting/reporting_garbage_collector.cc:25: cache->RemoveReports(all_reports); Since this policy is for all the reports, how about having a RemoveAllReports API as well which can be invoked here and don't need to invoke GetReports before this call.
The CQ bit was checked by juliatuttle@chromium.org to run a CQ dry run
Thanks, shivanisha! https://codereview.chromium.org/2740833004/diff/40001/net/reporting/reporting... File net/reporting/reporting_garbage_collector.cc (right): https://codereview.chromium.org/2740833004/diff/40001/net/reporting/reporting... net/reporting/reporting_garbage_collector.cc:20: bool network_changed) { On 2017/03/16 00:22:45, shivanisha wrote: > DCHECK(cache) Done. https://codereview.chromium.org/2740833004/diff/40001/net/reporting/reporting... net/reporting/reporting_garbage_collector.cc:25: cache->RemoveReports(all_reports); On 2017/03/16 00:22:45, shivanisha wrote: > Since this policy is for all the reports, how about having a RemoveAllReports > API as well which can be invoked here and don't need to invoke GetReports before > this call. Done.
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...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Description was changed from ========== Reporting: Create garbage collector. Reporting is a spec for delivering out-of-band reports from various other parts of the browser. See http://wicg.github.io/reporting/. This implements the garbage collector, which cleans out reports that haven't been delivered for too long or through too many attempts (and also clears reports on network changes, if so configured). ========== to ========== Reporting: Implement garbage collector. Reporting is a spec for delivering out-of-band reports from various other parts of the browser. See http://wicg.github.io/reporting/ for the spec, or https://goo.gl/pygX5I for details of the planned implementation in Chromium. This implements the garbage collector, which cleans out reports that haven't been delivered for too long or through too many attempts (and also clears reports on network changes, if so configured). ==========
Description was changed from ========== Reporting: Implement garbage collector. Reporting is a spec for delivering out-of-band reports from various other parts of the browser. See http://wicg.github.io/reporting/ for the spec, or https://goo.gl/pygX5I for details of the planned implementation in Chromium. This implements the garbage collector, which cleans out reports that haven't been delivered for too long or through too many attempts (and also clears reports on network changes, if so configured). ========== to ========== Reporting: Implement garbage collector. Reporting is a spec for delivering out-of-band reports from various other parts of the browser. See http://wicg.github.io/reporting/ for the spec, or https://goo.gl/pygX5I for details of the planned implementation in Chromium. This implements the garbage collector, which cleans out reports that haven't been delivered for too long or through too many attempts (and also clears reports on network changes, if so configured). BUG=704259 ==========
On 2017/03/21 at 19:26:57, juliatuttle wrote: > Thanks, shivanisha! > > https://codereview.chromium.org/2740833004/diff/40001/net/reporting/reporting... > File net/reporting/reporting_garbage_collector.cc (right): > > https://codereview.chromium.org/2740833004/diff/40001/net/reporting/reporting... > net/reporting/reporting_garbage_collector.cc:20: bool network_changed) { > On 2017/03/16 00:22:45, shivanisha wrote: > > DCHECK(cache) > > Done. > > https://codereview.chromium.org/2740833004/diff/40001/net/reporting/reporting... > net/reporting/reporting_garbage_collector.cc:25: cache->RemoveReports(all_reports); > On 2017/03/16 00:22:45, shivanisha wrote: > > Since this policy is for all the reports, how about having a RemoveAllReports > > API as well which can be invoked here and don't need to invoke GetReports before > > this call. > > Done. lgtm
juliatuttle@chromium.org changed reviewers: + rdsmith@chromium.org
PTAL, rdsmith
High level design question/clarification. https://codereview.chromium.org/2740833004/diff/60001/net/reporting/reporting... File net/reporting/reporting_garbage_collector.h (right): https://codereview.chromium.org/2740833004/diff/60001/net/reporting/reporting... net/reporting/reporting_garbage_collector.h:19: class NET_EXPORT ReportingGarbageCollector { Why this design? What you've effectively got here is a namespace wrapping a function that something else has to call every once in a while. Why not construct a singleton (which, yes, something else has to own, but can own and forget until teardown) which decides how often it needs to wakeup and GC, and stores the policy (since that's presumably not going to change)?
The CQ bit was checked by juliatuttle@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...
On 2017/03/31 15:36:55, Randy Smith (Not in Mondays) wrote: > High level design question/clarification. > > https://codereview.chromium.org/2740833004/diff/60001/net/reporting/reporting... > File net/reporting/reporting_garbage_collector.h (right): > > https://codereview.chromium.org/2740833004/diff/60001/net/reporting/reporting... > net/reporting/reporting_garbage_collector.h:19: class NET_EXPORT > ReportingGarbageCollector { > Why this design? What you've effectively got here is a namespace wrapping a > function that something else has to call every once in a while. Why not > construct a singleton (which, yes, something else has to own, but can own and > forget until teardown) which decides how often it needs to wakeup and GC, and > stores the policy (since that's presumably not going to change)? Right now, I'm just landing the GC code itself, and it's not called anywhere. Eventually, I'll either add scheduling to it, or call it from an external scheduler. Actually. come to think of it, I could just have a single ReportingScheduler that observes cache updates and triggers the DeliveryAgent, GarbageCollector, Serializer, and so on.
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 juliatuttle@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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2017/03/31 16:43:10, Julia Tuttle wrote: > On 2017/03/31 15:36:55, Randy Smith (Not in Mondays) wrote: > > High level design question/clarification. > > > > > https://codereview.chromium.org/2740833004/diff/60001/net/reporting/reporting... > > File net/reporting/reporting_garbage_collector.h (right): > > > > > https://codereview.chromium.org/2740833004/diff/60001/net/reporting/reporting... > > net/reporting/reporting_garbage_collector.h:19: class NET_EXPORT > > ReportingGarbageCollector { > > Why this design? What you've effectively got here is a namespace wrapping a > > function that something else has to call every once in a while. Why not > > construct a singleton (which, yes, something else has to own, but can own and > > forget until teardown) which decides how often it needs to wakeup and GC, and > > stores the policy (since that's presumably not going to change)? > > Right now, I'm just landing the GC code itself, and it's not called anywhere. > > Eventually, I'll either add scheduling to it, or call it from an external > scheduler. > > Actually. come to think of it, I could just have a single ReportingScheduler > that observes cache updates and triggers the DeliveryAgent, GarbageCollector, > Serializer, and so on. Per discussion with rdsmith@, I'm having the GarbageCollector (and DeliveryAgent and Serializer) each schedule their own work.
PTAL shivanisha -- this has changed substantially.
https://codereview.chromium.org/2740833004/diff/140001/net/reporting/reportin... File net/reporting/reporting_context.h (right): https://codereview.chromium.org/2740833004/diff/140001/net/reporting/reportin... net/reporting/reporting_context.h:83: std::unique_ptr<base::Timer> garbage_collection_timer_; Since the garbage_collector_ object is responsible for scheduling itself, can the ownership of garbage_collection_timer_ be made the responsibility of garbage_collector_ as well? Seems more consistent in terms of keeping everything related to garbage collection in the ReportingGarbageCollector class.
On 2017/04/05 16:44:07, shivanisha wrote: > https://codereview.chromium.org/2740833004/diff/140001/net/reporting/reportin... > File net/reporting/reporting_context.h (right): > > https://codereview.chromium.org/2740833004/diff/140001/net/reporting/reportin... > net/reporting/reporting_context.h:83: std::unique_ptr<base::Timer> > garbage_collection_timer_; > Since the garbage_collector_ object is responsible for scheduling itself, can > the ownership of garbage_collection_timer_ be made the responsibility of > garbage_collector_ as well? > Seems more consistent in terms of keeping everything related to garbage > collection in the ReportingGarbageCollector class. The reason I'm doing it this way is so that tests can inject a MockTimer and easily access it later (by just asking the Context). Another option would be to have tests create and store a raw pointer to the MockTimer, pass a unique_ptr to the Context, and have the Context pass that on to the GarbageCollector. Would you prefer that?
(Also, since you can't build a timer that can fire based on a TickClock, I'm treating Timers as low-level dependencies of the whole Reporting system, like Clock and TickClock.)
juliatuttle@chromium.org changed reviewers: - rdsmith@chromium.org
Removing rdsmith since I'm waiting on shivanisha's review; will add csharrison later for committer signoff if needed.
On 2017/04/06 at 17:53:09, juliatuttle wrote: > On 2017/04/05 16:44:07, shivanisha wrote: > > https://codereview.chromium.org/2740833004/diff/140001/net/reporting/reportin... > > File net/reporting/reporting_context.h (right): > > > > https://codereview.chromium.org/2740833004/diff/140001/net/reporting/reportin... > > net/reporting/reporting_context.h:83: std::unique_ptr<base::Timer> > > garbage_collection_timer_; > > Since the garbage_collector_ object is responsible for scheduling itself, can > > the ownership of garbage_collection_timer_ be made the responsibility of > > garbage_collector_ as well? > > Seems more consistent in terms of keeping everything related to garbage > > collection in the ReportingGarbageCollector class. > > The reason I'm doing it this way is so that tests can inject a MockTimer and easily access it later (by just asking the Context). > > Another option would be to have tests create and store a raw pointer to the MockTimer, pass a unique_ptr to the Context, and have the Context pass that on to the GarbageCollector. > > Would you prefer that? The latter sounds good to me.
On 2017/04/07 16:44:38, shivanisha wrote: > On 2017/04/06 at 17:53:09, juliatuttle wrote: > > On 2017/04/05 16:44:07, shivanisha wrote: > > > > https://codereview.chromium.org/2740833004/diff/140001/net/reporting/reportin... > > > File net/reporting/reporting_context.h (right): > > > > > > > https://codereview.chromium.org/2740833004/diff/140001/net/reporting/reportin... > > > net/reporting/reporting_context.h:83: std::unique_ptr<base::Timer> > > > garbage_collection_timer_; > > > Since the garbage_collector_ object is responsible for scheduling itself, > can > > > the ownership of garbage_collection_timer_ be made the responsibility of > > > garbage_collector_ as well? > > > Seems more consistent in terms of keeping everything related to garbage > > > collection in the ReportingGarbageCollector class. > > > > The reason I'm doing it this way is so that tests can inject a MockTimer and > easily access it later (by just asking the Context). > > > > Another option would be to have tests create and store a raw pointer to the > MockTimer, pass a unique_ptr to the Context, and have the Context pass that on > to the GarbageCollector. > > > > Would you prefer that? > > The latter sounds good to me. Okay, I did that. I'll apply the same change to the DeliveryAgent (previously DeliveryScheduler) and Serializer CLs, since they both have similar timer architectures.
PTAL, shivanisha.
The CQ bit was checked by juliatuttle@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.
On 2017/04/07 at 18:38:50, juliatuttle wrote: > PTAL, shivanisha. lgtm
juliatuttle@chromium.org changed reviewers: + csharrison@chromium.org
PTAL csharrison for committer signoff.
RS LGTM
The CQ bit was checked by juliatuttle@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1491848547756170,
"parent_rev": "91037c1a4a82985c652ac38826fd5b3fc6d56d91", "commit_rev":
"9f970c02c1feef9d61eff3fd52db7253eab1d9d9"}
Message was sent while issue was closed.
Description was changed from ========== Reporting: Implement garbage collector. Reporting is a spec for delivering out-of-band reports from various other parts of the browser. See http://wicg.github.io/reporting/ for the spec, or https://goo.gl/pygX5I for details of the planned implementation in Chromium. This implements the garbage collector, which cleans out reports that haven't been delivered for too long or through too many attempts (and also clears reports on network changes, if so configured). BUG=704259 ========== to ========== Reporting: Implement garbage collector. Reporting is a spec for delivering out-of-band reports from various other parts of the browser. See http://wicg.github.io/reporting/ for the spec, or https://goo.gl/pygX5I for details of the planned implementation in Chromium. This implements the garbage collector, which cleans out reports that haven't been delivered for too long or through too many attempts (and also clears reports on network changes, if so configured). BUG=704259 Review-Url: https://codereview.chromium.org/2740833004 Cr-Commit-Position: refs/heads/master@{#463359} Committed: https://chromium.googlesource.com/chromium/src/+/9f970c02c1feef9d61eff3fd52db... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/9f970c02c1feef9d61eff3fd52db... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
