Reporting: Cap number of clients in cache.

Reporting: Cap number of clients in cache.

BUG=704259
Review-Url: https://codereview.chromium.org/2851603002
Cr-Commit-Position: refs/heads/master@{#471071}
Committed: https://chromium.googlesource.com/chromium/src/+/86cb6592130b35de26282d8855c135d550410f61

[Julia Tuttle, 2017-04-28 18:25:46]

The CQ bit was checked by juliatuttle@chromium.org to run a CQ dry run

[Julia Tuttle, 2017-04-28 18:26:12]

juliatuttle@chromium.org changed reviewers:
+ shivanisha@chromium.org

[Julia Tuttle, 2017-04-28 18:26:13]

PTAL, shivanisha.

[commit-bot: I haz the power, 2017-04-28 18:26:48]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-28 20:14:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-28 20:14:38]

Dry run: This issue passed the CQ dry run.

[shivanisha, 2017-05-03 19:00:01]

https://codereview.chromium.org/2851603002/diff/20001/net/reporting/reporting...
File net/reporting/reporting_cache.cc (right):

https://codereview.chromium.org/2851603002/diff/20001/net/reporting/reporting...
net/reporting/reporting_cache.cc:202: }
May be not have old_client here at all.
AddClient can then use [] operator to add the client which will make sure to
replace if it exists and insert if it doesn't.
You can also then remove the DCHECK(!old_client) in line 213. To me it seems
simpler to follow unless we are losing anything with this approach?

https://codereview.chromium.org/2851603002/diff/20001/net/reporting/reporting...
net/reporting/reporting_cache.cc:217:
RemoveClient(FindClientToEvict(tick_clock()->NowTicks()));
RemoveClient(to_evict)?

https://codereview.chromium.org/2851603002/diff/20001/net/reporting/reporting...
net/reporting/reporting_cache.cc:374: for (const auto& it : client_last_used_) {
How does this ensure that clients which have pending reports are not evicted?

[Julia Tuttle, 2017-05-04 16:50:22]

PTAL, shivanisha.

https://codereview.chromium.org/2851603002/diff/20001/net/reporting/reporting...
File net/reporting/reporting_cache.cc (right):

https://codereview.chromium.org/2851603002/diff/20001/net/reporting/reporting...
net/reporting/reporting_cache.cc:202: }
On 2017/05/03 19:00:00, shivanisha wrote:
> May be not have old_client here at all.
> AddClient can then use [] operator to add the client which will make sure to
> replace if it exists and insert if it doesn't.
> You can also then remove the DCHECK(!old_client) in line 213. To me it seems
> simpler to follow unless we are losing anything with this approach?

There's more going on in RemoveClient than just removing the client; it also has
to remove it from the wildcard index and last used map. We don't *need* the
DCHECK on line 213, though -- it was just a sanity check.

https://codereview.chromium.org/2851603002/diff/20001/net/reporting/reporting...
net/reporting/reporting_cache.cc:217:
RemoveClient(FindClientToEvict(tick_clock()->NowTicks()));
On 2017/05/03 19:00:00, shivanisha wrote:
> RemoveClient(to_evict)?

...oops, done.

https://codereview.chromium.org/2851603002/diff/20001/net/reporting/reporting...
net/reporting/reporting_cache.cc:374: for (const auto& it : client_last_used_) {
On 2017/05/03 19:00:00, shivanisha wrote:
> How does this ensure that clients which have pending reports are not evicted?

It doesn't! The cache has no understanding of the relationship between reports
and clients.

I'm not sure if I should be worried about this; for sites that regularly send
reports, LRU will keep them from being evicted, but for sites that don't,
there's no way to know which clients will be needed later when picking one to
evict.

[shivanisha, 2017-05-05 19:24:48]

https://codereview.chromium.org/2851603002/diff/20001/net/reporting/reporting...
File net/reporting/reporting_cache.cc (right):

https://codereview.chromium.org/2851603002/diff/20001/net/reporting/reporting...
net/reporting/reporting_cache.cc:202: }
On 2017/05/04 at 16:50:21, Julia Tuttle wrote:
> On 2017/05/03 19:00:00, shivanisha wrote:
> > May be not have old_client here at all.
> > AddClient can then use [] operator to add the client which will make sure to
> > replace if it exists and insert if it doesn't.
> > You can also then remove the DCHECK(!old_client) in line 213. To me it seems
> > simpler to follow unless we are losing anything with this approach?
> 
> There's more going on in RemoveClient than just removing the client; it also
has to remove it from the wildcard index and last used map. We don't *need* the
DCHECK on line 213, though -- it was just a sanity check.

Acknowledged.

https://codereview.chromium.org/2851603002/diff/20001/net/reporting/reporting...
net/reporting/reporting_cache.cc:374: for (const auto& it : client_last_used_) {
On 2017/05/04 at 16:50:21, Julia Tuttle wrote:
> On 2017/05/03 19:00:00, shivanisha wrote:
> > How does this ensure that clients which have pending reports are not
evicted?
> 
> It doesn't! The cache has no understanding of the relationship between reports
and clients.
> 
> I'm not sure if I should be worried about this; for sites that regularly send
reports, LRU will keep them from being evicted, but for sites that don't,
there's no way to know which clients will be needed later when picking one to
evict.

Can we map the elements in pending_reports_ to the client they are going to be
sent to? If yes, then we can avoid evicting those clients.

[Julia Tuttle, 2017-05-08 14:15:57]

https://codereview.chromium.org/2851603002/diff/20001/net/reporting/reporting...
File net/reporting/reporting_cache.cc (right):

https://codereview.chromium.org/2851603002/diff/20001/net/reporting/reporting...
net/reporting/reporting_cache.cc:374: for (const auto& it : client_last_used_) {
On 2017/05/05 19:24:48, shivanisha wrote:
> On 2017/05/04 at 16:50:21, Julia Tuttle wrote:
> > On 2017/05/03 19:00:00, shivanisha wrote:
> > > How does this ensure that clients which have pending reports are not
> evicted?
> > 
> > It doesn't! The cache has no understanding of the relationship between
reports
> and clients.
> > 
> > I'm not sure if I should be worried about this; for sites that regularly
send
> reports, LRU will keep them from being evicted, but for sites that don't,
> there's no way to know which clients will be needed later when picking one to
> evict.
> 
> Can we map the elements in pending_reports_ to the client they are going to be
> sent to? If yes, then we can avoid evicting those clients.

We could do that, but that would make evicting a client an
O(num_clients*num_pending_reports) operation. I'd rather just keep a set of
pending clients as well.

Really, though, I'm not sure what protecting pending clients gains us over just
updating the LRU timestamp when we start a delivery attempt -- if the cache is
that crowded, we're going to need to evict something, and we don't actually *do*
anything with the client when the delivery attempt finishes.

[shivanisha, 2017-05-08 19:53:08]

On 2017/05/08 at 14:15:57, juliatuttle wrote:
>
https://codereview.chromium.org/2851603002/diff/20001/net/reporting/reporting...
> File net/reporting/reporting_cache.cc (right):
> 
>
https://codereview.chromium.org/2851603002/diff/20001/net/reporting/reporting...
> net/reporting/reporting_cache.cc:374: for (const auto& it : client_last_used_)
{
> On 2017/05/05 19:24:48, shivanisha wrote:
> > On 2017/05/04 at 16:50:21, Julia Tuttle wrote:
> > > On 2017/05/03 19:00:00, shivanisha wrote:
> > > > How does this ensure that clients which have pending reports are not
> > evicted?
> > > 
> > > It doesn't! The cache has no understanding of the relationship between
reports
> > and clients.
> > > 
> > > I'm not sure if I should be worried about this; for sites that regularly
send
> > reports, LRU will keep them from being evicted, but for sites that don't,
> > there's no way to know which clients will be needed later when picking one
to
> > evict.
> > 
> > Can we map the elements in pending_reports_ to the client they are going to
be
> > sent to? If yes, then we can avoid evicting those clients.
> 
> We could do that, but that would make evicting a client an
O(num_clients*num_pending_reports) operation. I'd rather just keep a set of
pending clients as well.
> 
> Really, though, I'm not sure what protecting pending clients gains us over
just updating the LRU timestamp when we start a delivery attempt -- if the cache
is that crowded, we're going to need to evict something, and we don't actually
*do* anything with the client when the delivery attempt finishes.

So is my understanding correct in saying that a pending report will always have
had it's client's LRU timestamp updated when it was added to pending_reports_
and so a pending report's client can only get evicted if all clients have
reports that are under delivery attempts. In that case, I think its worth
mentioning this in a comment in FindClientToEvict. 
Also, do you think it can be mentioned in a comment somewhere in the report
delivery logic for the code to be aware that the client may have been evicted.

[Julia Tuttle, 2017-05-08 19:55:17]

On 2017/05/08 19:53:08, shivanisha wrote:
> On 2017/05/08 at 14:15:57, juliatuttle wrote:
> >
>
https://codereview.chromium.org/2851603002/diff/20001/net/reporting/reporting...
> > File net/reporting/reporting_cache.cc (right):
> > 
> >
>
https://codereview.chromium.org/2851603002/diff/20001/net/reporting/reporting...
> > net/reporting/reporting_cache.cc:374: for (const auto& it :
client_last_used_)
> {
> > On 2017/05/05 19:24:48, shivanisha wrote:
> > > On 2017/05/04 at 16:50:21, Julia Tuttle wrote:
> > > > On 2017/05/03 19:00:00, shivanisha wrote:
> > > > > How does this ensure that clients which have pending reports are not
> > > evicted?
> > > > 
> > > > It doesn't! The cache has no understanding of the relationship between
> reports
> > > and clients.
> > > > 
> > > > I'm not sure if I should be worried about this; for sites that regularly
> send
> > > reports, LRU will keep them from being evicted, but for sites that don't,
> > > there's no way to know which clients will be needed later when picking one
> to
> > > evict.
> > > 
> > > Can we map the elements in pending_reports_ to the client they are going
to
> be
> > > sent to? If yes, then we can avoid evicting those clients.
> > 
> > We could do that, but that would make evicting a client an
> O(num_clients*num_pending_reports) operation. I'd rather just keep a set of
> pending clients as well.
> > 
> > Really, though, I'm not sure what protecting pending clients gains us over
> just updating the LRU timestamp when we start a delivery attempt -- if the
cache
> is that crowded, we're going to need to evict something, and we don't actually
> *do* anything with the client when the delivery attempt finishes.
> 
> So is my understanding correct in saying that a pending report will always
have
> had it's client's LRU timestamp updated when it was added to pending_reports_
> and so a pending report's client can only get evicted if all clients have
> reports that are under delivery attempts. In that case, I think its worth
> mentioning this in a comment in FindClientToEvict. 
> Also, do you think it can be mentioned in a comment somewhere in the report
> delivery logic for the code to be aware that the client may have been evicted.

Mostly; reports aren't bound to clients, but whatever client is selected at that
time to upload the report will have the timestamp updated.

I'm not sure it makes sense to mention in FindClientToEvict, since the delivery
code has to be able to deal with all kinds of removals (expiration, network
change, eviction, or explicit removal with max-age: 0).

[shivanisha, 2017-05-08 20:03:46]

On 2017/05/08 at 19:55:17, juliatuttle wrote:
> On 2017/05/08 19:53:08, shivanisha wrote:
> > On 2017/05/08 at 14:15:57, juliatuttle wrote:
> > >
> >
https://codereview.chromium.org/2851603002/diff/20001/net/reporting/reporting...
> > > File net/reporting/reporting_cache.cc (right):
> > > 
> > >
> >
https://codereview.chromium.org/2851603002/diff/20001/net/reporting/reporting...
> > > net/reporting/reporting_cache.cc:374: for (const auto& it :
client_last_used_)
> > {
> > > On 2017/05/05 19:24:48, shivanisha wrote:
> > > > On 2017/05/04 at 16:50:21, Julia Tuttle wrote:
> > > > > On 2017/05/03 19:00:00, shivanisha wrote:
> > > > > > How does this ensure that clients which have pending reports are not
> > > > evicted?
> > > > > 
> > > > > It doesn't! The cache has no understanding of the relationship between
> > reports
> > > > and clients.
> > > > > 
> > > > > I'm not sure if I should be worried about this; for sites that
regularly
> > send
> > > > reports, LRU will keep them from being evicted, but for sites that
don't,
> > > > there's no way to know which clients will be needed later when picking
one
> > to
> > > > evict.
> > > > 
> > > > Can we map the elements in pending_reports_ to the client they are going
to
> > be
> > > > sent to? If yes, then we can avoid evicting those clients.
> > > 
> > > We could do that, but that would make evicting a client an
> > O(num_clients*num_pending_reports) operation. I'd rather just keep a set of
> > pending clients as well.
> > > 
> > > Really, though, I'm not sure what protecting pending clients gains us over
> > just updating the LRU timestamp when we start a delivery attempt -- if the
cache
> > is that crowded, we're going to need to evict something, and we don't
actually
> > *do* anything with the client when the delivery attempt finishes.
> > 
> > So is my understanding correct in saying that a pending report will always
have
> > had it's client's LRU timestamp updated when it was added to
pending_reports_
> > and so a pending report's client can only get evicted if all clients have
> > reports that are under delivery attempts. In that case, I think its worth
> > mentioning this in a comment in FindClientToEvict. 
> > Also, do you think it can be mentioned in a comment somewhere in the report
> > delivery logic for the code to be aware that the client may have been
evicted.
> 
> Mostly; reports aren't bound to clients, but whatever client is selected at
that time to upload the report will have the timestamp updated.
> 
> I'm not sure it makes sense to mention in FindClientToEvict, since the
delivery code has to be able to deal with all kinds of removals (expiration,
network change, eviction, or explicit removal with max-age: 0).

Got it, so does the delivery code documents this uncertainty of removal
somewhere?

[Julia Tuttle, 2017-05-08 20:06:57]

It never really needs to, because it never touches clients directly. It
touches sets of pending endpoints (by URL), pending (origin, group) tuples,
and pending reports, but not clients themselves.

Come to think of it, though, I should probably also cap the number of
endpoints tracked in EndpointManager.

On Mon, May 8, 2017 at 4:03 PM <shivanisha@chromium.org> wrote:

> On 2017/05/08 at 19:55:17, juliatuttle wrote:
> > On 2017/05/08 19:53:08, shivanisha wrote:
> > > On 2017/05/08 at 14:15:57, juliatuttle wrote:
> > > >
> > >
>
>
https://codereview.chromium.org/2851603002/diff/20001/net/reporting/reporting...
> > > > File net/reporting/reporting_cache.cc (right):
> > > >
> > > >
> > >
>
>
https://codereview.chromium.org/2851603002/diff/20001/net/reporting/reporting...
> > > > net/reporting/reporting_cache.cc:374: for (const auto& it :
> client_last_used_)
> > > {
> > > > On 2017/05/05 19:24:48, shivanisha wrote:
> > > > > On 2017/05/04 at 16:50:21, Julia Tuttle wrote:
> > > > > > On 2017/05/03 19:00:00, shivanisha wrote:
> > > > > > > How does this ensure that clients which have pending reports
> are not
> > > > > evicted?
> > > > > >
> > > > > > It doesn't! The cache has no understanding of the relationship
> between
> > > reports
> > > > > and clients.
> > > > > >
> > > > > > I'm not sure if I should be worried about this; for sites that
> regularly
> > > send
> > > > > reports, LRU will keep them from being evicted, but for sites that
> don't,
> > > > > there's no way to know which clients will be needed later when
> picking
> one
> > > to
> > > > > evict.
> > > > >
> > > > > Can we map the elements in pending_reports_ to the client they are
> going
> to
> > > be
> > > > > sent to? If yes, then we can avoid evicting those clients.
> > > >
> > > > We could do that, but that would make evicting a client an
> > > O(num_clients*num_pending_reports) operation. I'd rather just keep a
> set of
> > > pending clients as well.
> > > >
> > > > Really, though, I'm not sure what protecting pending clients gains
> us over
> > > just updating the LRU timestamp when we start a delivery attempt -- if
> the
> cache
> > > is that crowded, we're going to need to evict something, and we don't
> actually
> > > *do* anything with the client when the delivery attempt finishes.
> > >
> > > So is my understanding correct in saying that a pending report will
> always
> have
> > > had it's client's LRU timestamp updated when it was added to
> pending_reports_
> > > and so a pending report's client can only get evicted if all clients
> have
> > > reports that are under delivery attempts. In that case, I think its
> worth
> > > mentioning this in a comment in FindClientToEvict.
> > > Also, do you think it can be mentioned in a comment somewhere in the
> report
> > > delivery logic for the code to be aware that the client may have been
> evicted.
> >
> > Mostly; reports aren't bound to clients, but whatever client is selected
> at
> that time to upload the report will have the timestamp updated.
> >
> > I'm not sure it makes sense to mention in FindClientToEvict, since the
> delivery code has to be able to deal with all kinds of removals
> (expiration,
> network change, eviction, or explicit removal with max-age: 0).
>
> Got it, so does the delivery code documents this uncertainty of removal
> somewhere?
>
> https://codereview.chromium.org/2851603002/
>

-- 
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.

[shivanisha, 2017-05-08 20:35:27]

LGTM.

On 2017/05/08 at 20:06:57, juliatuttle wrote:
> It never really needs to, because it never touches clients directly. It
> touches sets of pending endpoints (by URL), pending (origin, group) tuples,
> and pending reports, but not clients themselves.

Acknowledged.

> 
> Come to think of it, though, I should probably also cap the number of
> endpoints tracked in EndpointManager.
>

[Julia Tuttle, 2017-05-09 16:52:32]

The CQ bit was checked by juliatuttle@chromium.org

[commit-bot: I haz the power, 2017-05-09 16:53:36]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-09 18:01:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-09 18:01:59]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Julia Tuttle, 2017-05-11 19:11:17]

The CQ bit was checked by juliatuttle@chromium.org

[commit-bot: I haz the power, 2017-05-11 19:12:50]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-11 21:12:54]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1494529877837820,
"parent_rev": "0d001e69ad6e768eda29b9c3b7551ffaadcf6de6", "commit_rev":
"86cb6592130b35de26282d8855c135d550410f61"}

[commit-bot: I haz the power, 2017-05-11 21:13:05]

Description was changed from

==========
Reporting: Cap number of clients in cache.

BUG=704259
==========

to

==========
Reporting: Cap number of clients in cache.

BUG=704259

Review-Url: https://codereview.chromium.org/2851603002
Cr-Commit-Position: refs/heads/master@{#471071}
Committed:
https://chromium.googlesource.com/chromium/src/+/86cb6592130b35de26282d8855c1...
==========

[commit-bot: I haz the power, 2017-05-11 21:13:09]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/86cb6592130b35de26282d8855c1...