Make synchronous methods on disk_cache::SimpleEntryImpl() reentrant.

Make synchronous methods on disk_cache::SimpleEntryImpl() reentrant.

For passing browser tests and unit tests, it turns out that the synchronous operations on disk_cache::Entry are called quite a bit. As I was trying to get us to the point that this cache can pass browser tests, I found this was necessary.

Note: this CL is based on https://codereview.chromium.org/12192005/ (Add simple cache backend) , and must land after it.

This CL is the basis of https://codereview.chromium.org/12223075/ (Make Doom asynchronous), and so it must land before that issue.

R=rvargas@chromium.org, felipeg@chromium.org, pasko@chromium.org, willchan@chromium.org
BUG=173392
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=184501

[gavinp, 2013-02-11 20:10:42]

PTAL. This is downstream of the (still in review) simple backend CL.

[pasko-google - do not use, 2013-02-12 14:15:10]

lgtm

https://codereview.chromium.org/12226095/diff/1/net/disk_cache/simple/simple_...
File net/disk_cache/simple/simple_disk_format.h (right):

https://codereview.chromium.org/12226095/diff/1/net/disk_cache/simple/simple_...
net/disk_cache/simple/simple_disk_format.h:17: static const int
kSimpleEntryFileCount = 3;
nit: a better name would be kSimpleEntryStreamCount because the SimpleEntryImpl
is abstracted away enough from the implementation detail that streams are
written into separate files in the synchronous entry.

By the way, why is it SimpleEntryImpl and not a more human language name
SimpleCacheEntry?

[felipeg, 2013-02-12 14:47:48]

https://chromiumcodereview.appspot.com/12226095/diff/1/net/disk_cache/simple/...
File net/disk_cache/simple/simple_entry_impl.cc (left):

https://chromiumcodereview.appspot.com/12226095/diff/1/net/disk_cache/simple/...
net/disk_cache/simple/simple_entry_impl.cc:219: return
synchronous_entry_->last_used();
If we never set synchronous_entry_ to NULL as my comment in the other CL (by
adding a boolean IsOperationPending() instead of setting the pointer to NULL).

In that case, we can just call synchronous_entry_->last_used().
And this method must be thread safe by either using a lock
Or by just guaranteeing that the returned value will be either the new value or
the old value, but never a corrupt value, and letting the race happen.
It is ok for this kind of getter...

https://chromiumcodereview.appspot.com/12226095/diff/1/net/disk_cache/simple/...
File net/disk_cache/simple/simple_entry_impl.h (right):

https://chromiumcodereview.appspot.com/12226095/diff/1/net/disk_cache/simple/...
net/disk_cache/simple/simple_entry_impl.h:108: base::Time last_used_;
I don't like the fact that we are keeping a copy of this data on both objects
(here and in the SynchrnounsEntry).
It will be very easy in the future to change something and forget to make a copy
from one side or the other. When we have to maintain a copy on the code, the
data will eventually get inconsistent as we change the code.

I would rather keep it only here and remove it completely from SynchronousEntry.

[pasko-google - do not use, 2013-02-12 15:07:12]

https://chromiumcodereview.appspot.com/12226095/diff/1/net/disk_cache/simple/...
File net/disk_cache/simple/simple_entry_impl.cc (left):

https://chromiumcodereview.appspot.com/12226095/diff/1/net/disk_cache/simple/...
net/disk_cache/simple/simple_entry_impl.cc:219: return
synchronous_entry_->last_used();
On 2013/02/12 14:47:48, felipeg1 wrote:
> If we never set synchronous_entry_ to NULL as my comment in the other CL (by
> adding a boolean IsOperationPending() instead of setting the pointer to NULL).
> 
> In that case, we can just call synchronous_entry_->last_used().
> And this method must be thread safe by either using a lock
> Or by just guaranteeing that the returned value will be either the new value
or
> the old value, but never a corrupt value, and letting the race happen.
> It is ok for this kind of getter...

I disagree. Data races are never a good excuse for any design. Sorry.

Frankly speaking, I have done some designs where data races were "benign", i.e.
in worst case produced only some incorrectly gathered statistics, and even this
sometimes wakes me up as a nightmare. Here we make eviction decisions based on
this data, which makes the matter very complex and hardly reproducible if a data
race resulted in a very unusual code path.

Also, data races will be reported by data race detection tools, which we would
need to suppress using annotations in our code, which is very complicated and
requires a lot of expertise.

In the end, manual data privatization to threads is a commonly used pattern for
optimization. We may incapsulate it into a single class that would be easier to
update. Occasional inconsistencies when updating code may happen, of course, but
at least it would be buried within the same small class.

[felipeg, 2013-02-12 15:13:36]

I agree that data races (even benign) is not a good design choice.
But keeping the copy the way it is, is not good either. The semantics is
currently very confusing just as you described how confusing it will be
with a benign race condition.

I prefer trying to remove all uses of those variables from the SyncEntry
and keep them only on EntryImpl if possible.
Or if not possible, then, keep it on SyncEntry and use a lock to access the
variable from EntryImpl.


On Tue, Feb 12, 2013 at 4:07 PM, <pasko@google.com> wrote:

>
> https://chromiumcodereview.**appspot.com/12226095/diff/1/**
>
net/disk_cache/simple/simple_**entry_impl.cc<https://chromiumcodereview.appspot.com/12226095/diff/1/net/disk_cache/simple/simple_entry_impl.cc>
> File net/disk_cache/simple/simple_**entry_impl.cc (left):
>
> https://chromiumcodereview.**appspot.com/12226095/diff/1/**
>
net/disk_cache/simple/simple_**entry_impl.cc#oldcode219<https://chromiumcodereview.appspot.com/12226095/diff/1/net/disk_cache/simple/simple_entry_impl.cc#oldcode219>
> net/disk_cache/simple/simple_**entry_impl.cc:219: return
> synchronous_entry_->last_used(**);
> On 2013/02/12 14:47:48, felipeg1 wrote:
>
>> If we never set synchronous_entry_ to NULL as my comment in the other
>>
> CL (by
>
>> adding a boolean IsOperationPending() instead of setting the pointer
>>
> to NULL).
>
>  In that case, we can just call synchronous_entry_->last_used(**).
>> And this method must be thread safe by either using a lock
>> Or by just guaranteeing that the returned value will be either the new
>>
> value or
>
>> the old value, but never a corrupt value, and letting the race happen.
>> It is ok for this kind of getter...
>>
>
> I disagree. Data races are never a good excuse for any design. Sorry.
>
> Frankly speaking, I have done some designs where data races were
> "benign", i.e. in worst case produced only some incorrectly gathered
> statistics, and even this sometimes wakes me up as a nightmare. Here we
> make eviction decisions based on this data, which makes the matter very
> complex and hardly reproducible if a data race resulted in a very
> unusual code path.
>
> Also, data races will be reported by data race detection tools, which we
> would need to suppress using annotations in our code, which is very
> complicated and requires a lot of expertise.
>
> In the end, manual data privatization to threads is a commonly used
> pattern for optimization. We may incapsulate it into a single class that
> would be easier to update. Occasional inconsistencies when updating code
> may happen, of course, but at least it would be buried within the same
> small class.
>
>
https://chromiumcodereview.**appspot.com/12226095/<https://chromiumcodereview...
>

[gavinp, 2013-02-12 16:17:49]

https://codereview.chromium.org/12226095/diff/10002/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_entry_impl.cc (right):

https://codereview.chromium.org/12226095/diff/10002/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_entry_impl.cc:287: // filesystems not being
accurate.
I am, like pasko, afraid of data races, and that's why I wrote the code to do
copies.

I recognize your points about benign data races; even on late loading platforms
like ARM, by the time it really matters (at callback time), we've passed a
memory barrier and we're fine.

But isn't this early to worry  about this? Most of this data properly belongs in
the index structure, which lives on the IO thread. And you're implementing that,
right? So I wrote a big ol' TODO() for you here.

Ricardo, what do you think? I know in the upstream review (add simple cache
backend), you asked me about the duplication of key_, which was being done for
the same reason (and that was an even more benign race).

[felipeg, 2013-02-12 16:23:08]

lgtm

https://codereview.chromium.org/12226095/diff/10002/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_entry_impl.cc (right):

https://codereview.chromium.org/12226095/diff/10002/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_entry_impl.cc:287: // filesystems not being
accurate.
On 2013/02/12 16:17:49, gavinp wrote:
> I am, like pasko, afraid of data races, and that's why I wrote the code to do
> copies.
> 
> I recognize your points about benign data races; even on late loading
platforms
> like ARM, by the time it really matters (at callback time), we've passed a
> memory barrier and we're fine.
> 
> But isn't this early to worry  about this? Most of this data properly belongs
in
> the index structure, which lives on the IO thread. And you're implementing
that,
> right? So I wrote a big ol' TODO() for you here.
> 
> Ricardo, what do you think? I know in the upstream review (add simple cache
> backend), you asked me about the duplication of key_, which was being done for
> the same reason (and that was an even more benign race).

You are right.
We will need to store this on the index anyway for quick miss operations.

For now, the TODO suffice.

[rvargas (doing something else), 2013-02-13 03:10:36]

lgtm

https://codereview.chromium.org/12226095/diff/15/net/disk_cache/simple/simple...
File net/disk_cache/simple/simple_entry_impl.h (right):

https://codereview.chromium.org/12226095/diff/15/net/disk_cache/simple/simple...
net/disk_cache/simple/simple_entry_impl.h:104: void SetSynchronousData();
I would suggest a different name because it's starting to get confusing what
Synchronous means (as in synchronous_entry_ refers to stuff that happens on a
background thread while here  it means stuff that happens on the IO thread).

SetLocalData()?
UpdateLocalData()?

[rvargas (doing something else), 2013-02-13 03:12:43]

And please point to the relevant bug.

[gavinp, 2013-02-25 22:40:35]

Committed patchset #10 manually as r184501 (presubmit successful).