Move Preload() code to MappedFile

Move Preload() code to MappedFile

To heat up the file system caches we load the disk cache index 
into RAM when starting the browser. To make it more obvious
what we're doing, move that to a method named Preload().

BUG=None
R=rvargas@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=260844

[Daniel Bratell, 2014-03-20 14:48:43]

Looking at memory usage I stumbled upon some code in the disk cache that read a
large file (> 500 KB) into memory and then discarded the result without looking
at it.

The code isn't documented at all so it was not obvious if it was an accident or
not.

I fiddled a little with the code to make it not allocate quite so large a buffer
but I don't really know if this is a good idea or not. (there are also some cast
fixes but those are not critical).

gavinp and rvargas, what was the meaning of the code, and is this patch an
improvement or not?

[gavinp, 2014-03-20 16:11:50]

Daniel,

This upload seems broken (click on the view side by side links). Could you
re-upload so I can look at it in my preferred UI?

I know that's a pain. Our code review tool shouldn't suck. There's a few threads
about this in chromium-dev, the answer from the tools team is always "we know it
sucks and at <futuredate> it won't."

[Daniel Bratell, 2014-03-20 16:19:11]

I will try. I kept getting strange errors every upload. Then I thought they had
worked anyway (didn't check the side-by-side).

[Daniel Bratell, 2014-03-20 16:24:59]

On 2014/03/20 16:19:11, Daniel Bratell wrote:
> I will try. I kept getting strange errors every upload. Then I thought they
had
> worked anyway (didn't check the side-by-side).

I think it's better now.

[rvargas (doing something else), 2014-03-20 19:05:18]

https://codereview.chromium.org/206453003/diff/20001/net/disk_cache/blockfile...
File net/disk_cache/blockfile/mapped_file.cc (right):

https://codereview.chromium.org/206453003/diff/20001/net/disk_cache/blockfile...
net/disk_cache/blockfile/mapped_file.cc:40: static const size_t
kPreloadBufferSize = 16384;
The files have a limited size... the index is at most half a meg, and the mapped
section of other files is only two pages. Are you really concerned about
alloc/use/free of half a meg? I think it is much better for startup performance
to perform a single read than 32 disk reads.

[gavinp, 2014-03-21 10:20:24]

Please update the description to say "BUG=None" if you haven't created a
tracking bug for this. I suspect this issue doesn't need a tracking bug.

I also don't know how to test the performance impact of this change, because
it's going to slow down startup, but save early jank on the IO thread. The net
perf tests aren't going to capture it because they won't affect startup. This
preload could make startup slower overall, but still be worthwhile if it reduces
jank. I have no idea how to test any of this.

Besides startup churn, I do know that heap is very important on mobile
platforms, and even a small 512k allocation can contribute to private dirty
pages on Android, so it is best practice to avoid the superfluous allocation.

Architecturally, I think the approach of using calls to Read to populate the map
is funny. Ricardo notes that system calls have overhead. On Posix, the
implementation could just call madvise() with MADV_WILLNEED. On Windows, I think
a single call to ReadFileEx will have the desired result, and it seems lpBuffer
is an optional parameter. You might consider using the mmap argument
MAP_POPULATE, however it's non-Posix (i.e. not on MacOS) and that makes me sad.
madvise is POSIX. By using these platform methods we'll avoid a superfluous
memory to memory copy into user space, but still heat up the buffer cache so
accesses to the memory map will only incur TLB update costs.

However, I'm not convinced that we need to preload the map at all. It will be
demand paged in if it's needed, and I have absolutely no idea how to determine
if the startup churn is problematic. I suppose there will be some first request
jank, but well, without testing I don't think it's justified.

To put these memory mapped files in context, we use these mapped files in the
blockfile cache from the IO thread. We prohibit IO on the IO thread because we
don't want jank. Reading and writing to a memory map is IO, which is prohibited,
but we say that it's a memory reference thus permitted for these files. More
practically, these files, and these parts of these files, are very heavily used,
and so they're very likely to be in buffer cache. The preload is done out of a
sense of ideological purity, so we can make believe that dereferences inside the
map are not page faults thereafter. I'm unconvinced.

So I think I'd be inclined to LGTM a change that just removed the preload. I'd
also be happy to look at this CL if you advance it. I wish we could test it.

https://codereview.chromium.org/206453003/diff/20001/net/disk_cache/blockfile...
File net/disk_cache/blockfile/backend_impl.cc (right):

https://codereview.chromium.org/206453003/diff/20001/net/disk_cache/blockfile...
net/disk_cache/blockfile/backend_impl.cc:1288: data_ =
static_cast<Index*>(index_->Init(index_name, 0));
Good. It's almost superfluous to this change, but related enough and definitely
an improvement.

https://codereview.chromium.org/206453003/diff/20001/net/disk_cache/blockfile...
File net/disk_cache/blockfile/mapped_file.h (right):

https://codereview.chromium.org/206453003/diff/20001/net/disk_cache/blockfile...
net/disk_cache/blockfile/mapped_file.h:51: bool Preload();
Are there any MappedFile instances that we do not Preload?

[gavinp, 2014-03-21 10:21:11]

Whoops, I said the four letter word in my last comment. I'm going to write !lgtm
here, so that there's no doubt that was an accident. Don't consider this !lgtm
an indictment of your change, rather an indictment of my mistake.

[gavinp, 2014-03-21 10:21:20]

On 2014/03/21 10:21:11, gavinp wrote:
> Whoops, I said the four letter word in my last comment. I'm going to write
!lgtm
> here, so that there's no doubt that was an accident. Don't consider this !lgtm
> an indictment of your change, rather an indictment of my mistake.

Err, notlgtm.

[gavinp, 2014-03-21 10:21:43]

OK not lgtm is the magic phrase.

[Daniel Bratell, 2014-03-21 10:22:41]

On 2014/03/20 19:05:18, rvargas wrote:
>
https://codereview.chromium.org/206453003/diff/20001/net/disk_cache/blockfile...
> File net/disk_cache/blockfile/mapped_file.cc (right):
> 
>
https://codereview.chromium.org/206453003/diff/20001/net/disk_cache/blockfile...
> net/disk_cache/blockfile/mapped_file.cc:40: static const size_t
> kPreloadBufferSize = 16384;
> The files have a limited size... the index is at most half a meg, and the
mapped
> section of other files is only two pages. Are you really concerned about
> alloc/use/free of half a meg? I think it is much better for startup
performance
> to perform a single read than 32 disk reads.

I think this patch is more of a "request for comments" than a sincere suggestion
that this is what the code should look like. Why do we read those files at all?
Is my guess expressed in the comments the right guess?

Will performance be better or worse by doing X reads into the same memory than 1
read into a much larger memory block?

Is it possible to achieve the goal (whatever the goal is) without allocating
memory like this?

The reason I tripped on this code was the usage pattern, memory that is never
read from typically indicate a misuse of RAM. Maybe this isn't a typical case.

[rvargas (doing something else), 2014-03-21 18:06:58]

On 2014/03/21 10:22:41, Daniel Bratell wrote:
> On 2014/03/20 19:05:18, rvargas wrote:
> >
>
https://codereview.chromium.org/206453003/diff/20001/net/disk_cache/blockfile...
> > File net/disk_cache/blockfile/mapped_file.cc (right):
> > 
> >
>
https://codereview.chromium.org/206453003/diff/20001/net/disk_cache/blockfile...
> > net/disk_cache/blockfile/mapped_file.cc:40: static const size_t
> > kPreloadBufferSize = 16384;
> > The files have a limited size... the index is at most half a meg, and the
> mapped
> > section of other files is only two pages. Are you really concerned about
> > alloc/use/free of half a meg? I think it is much better for startup
> performance
> > to perform a single read than 32 disk reads.
> 
> I think this patch is more of a "request for comments" than a sincere
suggestion
> that this is what the code should look like. Why do we read those files at
all?
> Is my guess expressed in the comments the right guess?

This is a pattern that we generally use for things like this: if we know that we
are going to need the data soon, it is better to warm up things reading the
whole thing at once instead of letting a bunch of on-demand small reads to load
it. We use it for pretty much all sqlite databases, for example.

BTW, many moons ago we didn't do that for anything, and the changes were done to
improve startup performance.

In addition, we generally frown upon exception handling and IO errors on memory
mapped sections just generate exceptions. Reading the whole thing makes sure
that at least at startup the disk is actually able to read the file and allow us
to catch errors without crashing.

> 
> Will performance be better or worse by doing X reads into the same memory than
1
> read into a much larger memory block?

Unless the memory used is comparable to the memory required for the whole
application, bigger blocks and less reads should be better. Considering the real
requirements for chromium, I would not attempt to split reads if we are talking
about a few MB.

> Is it possible to achieve the goal (whatever the goal is) without allocating
> memory like this?

At the end, memory has to be used to read from disk, even if the kernel
allocates it and frees it up for us. Things would be much different if we were
holding to that memory for a long time, but doing one allocation for the length
of a single system call (even if that call may block) _should_ not be an issue
unless we push the system close to an out of memory situation.

> The reason I tripped on this code was the usage pattern, memory that is never
> read from typically indicate a misuse of RAM. Maybe this isn't a typical case.

I don't think this is a typical case.

[Daniel Bratell, 2014-03-31 16:17:02]

Removed the buffer shrinkage so left is just the code cleanup. Worth doing?

Please take a look.

[rvargas (doing something else), 2014-03-31 22:50:11]

lgtm

https://codereview.chromium.org/206453003/diff/60001/net/disk_cache/blockfile...
File net/disk_cache/blockfile/mapped_file.h (right):

https://codereview.chromium.org/206453003/diff/60001/net/disk_cache/blockfile...
net/disk_cache/blockfile/mapped_file.h:49: // Heats up the file cache and make
sure the file is fully readable
nit: file cache -> file system cache

[Daniel Bratell, 2014-04-01 08:57:57]

The CQ bit was checked by bratell@opera.com

[commit-bot: I haz the power, 2014-04-01 08:59:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bratell@opera.com/206453003/80001

[commit-bot: I haz the power, 2014-04-01 11:37:17]

Change committed as 260844