Avoid fragmenting the heap too much while reconstructing the index.

Avoid fragmenting the heap too much while reconstructing the index.

Memory analysis exposed that FileEnumerator doesn't scale well on very large
directories (e.g. ~80 000 entries) due to the high amount of heap allocations
it is performing causing unexpectedly high heap fragmentation (e.g.  ~5 MBytes
with a 80 000 files directory).

This CL makes SimpleIndexFile::SyncRestoreFromDisk() use directly readdir_r()
on POSIX and fallback to FileEnumerator on Windows. The code path triggered for
reconstructing the index from disk now causes a 40 KBytes RAM overhead (on the
cache that was used for testing) vs the 5 initial MBytes.

BUG=272062
R=gavinp@chromium.org, pasko@chromium.org

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

[gavinp, 2013-08-20 18:53:23]

https://chromiumcodereview.appspot.com/22927018/diff/9001/net/disk_cache/simp...
File net/disk_cache/simple/simple_index_file.h (right):

https://chromiumcodereview.appspot.com/22927018/diff/9001/net/disk_cache/simp...
net/disk_cache/simple/simple_index_file.h:143: std::string* buffer,
I'm a bit confused at this buffer being a std::string, it's possible that the
std::string will reallocate on every call to std::string::append or
std::string::assign, isn't it?

[gavinp, 2013-08-20 20:13:45]

https://codereview.chromium.org/22927018/diff/9001/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_index_file.cc (right):

https://codereview.chromium.org/22927018/diff/9001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index_file.cc:374: buffer->append(file_name);
I know that we've set capacity(), but I'm still scared that these are
potentially allocating, as the standard allows iterator invalidation on ANY
nonconst method on string, see ISO/IEC 14882:1998(E), 21.3(5).

https://codereview.chromium.org/22927018/diff/9001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index_file.cc:376: base::FilePath
file_path(*buffer);
Does this compile on Win32?

More concerning: this is potentially allocating. base::FilePath is just a
wrapper around std::string or std::wstring. In glibc, the copy constructor is
not allocating, so as long as no '\0' character is in the string, this isn't
allocating. However, in C++11, it's difficult to imagine a std::string copy
constructor that isn't allocating, and worse, we can't pass an allocator in to
base::FilePath.

https://codereview.chromium.org/22927018/diff/9001/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_index_file.h (right):

https://codereview.chromium.org/22927018/diff/9001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index_file.h:142: static void
ProcessEntryFile(const std::string& cache_path,
I suggest you want:

static void ProcessEntryFile(SimpleIndex::EntrySet* entries,
                                          uint64 entry_hash,
                                          const base::PlatformFileInfo&
entry_info);

Which can be for-sure-for-sure not allocating.

[Philippe, 2013-08-21 09:37:33]

Thanks Gavin!

https://codereview.chromium.org/22927018/diff/9001/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_index_file.cc (right):

https://codereview.chromium.org/22927018/diff/9001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index_file.cc:374: buffer->append(file_name);
On 2013/08/20 20:13:45, gavinp wrote:
> I know that we've set capacity(), but I'm still scared that these are
> potentially allocating, as the standard allows iterator invalidation on ANY
> nonconst method on string, see ISO/IEC 14882:1998(E), 21.3(5).

Yeah. I replaced the string with a vector.

https://codereview.chromium.org/22927018/diff/9001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index_file.cc:376: base::FilePath
file_path(*buffer);
On 2013/08/20 20:13:45, gavinp wrote:
> Does this compile on Win32?
> 
> More concerning: this is potentially allocating. base::FilePath is just a
> wrapper around std::string or std::wstring. In glibc, the copy constructor is
> not allocating, so as long as no '\0' character is in the string, this isn't
> allocating. However, in C++11, it's difficult to imagine a std::string copy
> constructor that isn't allocating, and worse, we can't pass an allocator in to
> base::FilePath.

I got rid of FilePath here as we discussed offline. I will run a windows try bot
to see if things compile there.

https://codereview.chromium.org/22927018/diff/9001/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_index_file.h (right):

https://codereview.chromium.org/22927018/diff/9001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index_file.h:142: static void
ProcessEntryFile(const std::string& cache_path,
On 2013/08/20 20:13:45, gavinp wrote:
> I suggest you want:
> 
> static void ProcessEntryFile(SimpleIndex::EntrySet* entries,
>                                           uint64 entry_hash,
>                                           const base::PlatformFileInfo&
> entry_info);
> 
> Which can be for-sure-for-sure not allocating.

I want to keep the hash extraction in this method to avoid duplicating the logic
on Windows and POSIX (unless I'm missing your point). Let me know if you are ok
with the new signature.

https://codereview.chromium.org/22927018/diff/9001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index_file.h:143: std::string* buffer,
On 2013/08/20 18:53:23, gavinp wrote:
> I'm a bit confused at this buffer being a std::string, it's possible that the
> std::string will reallocate on every call to std::string::append or
> std::string::assign, isn't it?

I replaced it with a vector that does now a single resize (and uses std::copy
with a non-back_inserter iterator).

[pasko, 2013-08-21 11:37:56]

https://codereview.chromium.org/22927018/diff/33001/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_index_file.cc (right):

https://codereview.chromium.org/22927018/diff/33001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index_file.cc:359: if (strcmp(file_name, "index")
&& strcmp(file_name, "fake_index"))
"the-real-index", reuse constants please

https://codereview.chromium.org/22927018/diff/33001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index_file.cc:427: const std::string cache_path =
cache_directory.value();
wstring to string conversion would probably not work on Windows. Generally you
should not call .value() on Windows, moving the .value() call to posix version
should be easy.

To avoid the trouble of testing on Windows (we don't want crashing the browser
for ~100 people who enabled Simple Cache with a flag on Windows, right?), I
would suggest using FilePath. This reallocation in the loop of the object(s) of
the same size would not cause heap fragmentation, and we could verify with the
memory benchmark that it is within bounds we want.

Let's not be emotional and too anal about allocations we know about. There are
so many other, bugger opportunities for memory reduction that we should spend
time on.

[gavinp, 2013-08-21 11:49:46]

Egor's elegant prose has convinced me that we can probably use base::FilePath
here.

However, if we don't do that, let's at least keep buffer in a char* or a
std::string. Using vector<char> is halfway and just as troubling.

https://codereview.chromium.org/22927018/diff/33001/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_index_file.cc (right):

https://codereview.chromium.org/22927018/diff/33001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index_file.cc:359: if (strcmp(file_name, "index")
&& strcmp(file_name, "fake_index"))
Nit: I know it's well defined as written, but I can read

  if (strcmp(file_name, "index") != 0 && strcmp(file_name, "fake_index") != 0))
    ...

more easily. While we're here, what's with the vulgar strings? And what's
"fake_index"? There's some constants SimpleIndexFile::kIndexFileName and you
probably want to add kFakeIndexFileName too.

https://codereview.chromium.org/22927018/diff/33001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index_file.cc:378: buffer_data +
file_name_start_pos, kEntryFilesHashLength);
I don't believe a std::vector<> is guaranteed to be stored sequentially in
C++98.

https://codereview.chromium.org/22927018/diff/33001/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_index_file.h (right):

https://codereview.chromium.org/22927018/diff/33001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index_file.h:18: #include "base/platform_file.h"
Pourquoi? Just forward declare base::PlatformFileInfo I say!

https://codereview.chromium.org/22927018/diff/33001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index_file.h:148: static void
ProcessEntryFile(const std::string& cache_path,
Can this method be moved into an anonymous namespace in simple_index_file.cc
easily?

https://codereview.chromium.org/22927018/diff/33001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index_file.h:149: std::vector<char>* buffer,
char*! Let your inner C programmer out.

[Philippe, 2013-08-21 12:41:38]

Thanks guys!

https://codereview.chromium.org/22927018/diff/33001/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_index_file.cc (right):

https://codereview.chromium.org/22927018/diff/33001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index_file.cc:359: if (strcmp(file_name, "index")
&& strcmp(file_name, "fake_index"))
On 2013/08/21 11:37:56, pasko wrote:
> "the-real-index", reuse constants please

Yeah, what a shame, I hadn't seen them (in the same file :)). I moved them to
the anonymous namespace btw.

https://codereview.chromium.org/22927018/diff/33001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index_file.cc:378: buffer_data +
file_name_start_pos, kEntryFilesHashLength);
On 2013/08/21 11:49:47, gavinp wrote:
> I don't believe a std::vector<> is guaranteed to be stored sequentially in
> C++98.

I got rid of the vector.

https://codereview.chromium.org/22927018/diff/33001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index_file.cc:427: const std::string cache_path =
cache_directory.value();
On 2013/08/21 11:37:56, pasko wrote:
> wstring to string conversion would probably not work on Windows. Generally you
> should not call .value() on Windows, moving the .value() call to posix version
> should be easy.
> 
> To avoid the trouble of testing on Windows (we don't want crashing the browser
> for ~100 people who enabled Simple Cache with a flag on Windows, right?), I
> would suggest using FilePath. This reallocation in the loop of the object(s)
of
> the same size would not cause heap fragmentation, and we could verify with the
> memory benchmark that it is within bounds we want.
> 
> Let's not be emotional and too anal about allocations we know about. There are
> so many other, bugger opportunities for memory reduction that we should spend
> time on.

Yeah we have been slightly paranoiac on this. Measurements show that there is
only a 8 KBytes overhead when not doing premature optimizations.

https://codereview.chromium.org/22927018/diff/33001/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_index_file.h (right):

https://codereview.chromium.org/22927018/diff/33001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index_file.h:18: #include "base/platform_file.h"
On 2013/08/21 11:49:47, gavinp wrote:
> Pourquoi? Just forward declare base::PlatformFileInfo I say!

Yeah my bad. I used to have GetPlatformFileInfo() return a value-typed
PlatformFileInfo.

https://codereview.chromium.org/22927018/diff/33001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index_file.h:148: static void
ProcessEntryFile(const std::string& cache_path,
On 2013/08/21 11:49:47, gavinp wrote:
> Can this method be moved into an anonymous namespace in simple_index_file.cc
> easily?

Yeah good point.

https://codereview.chromium.org/22927018/diff/33001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index_file.h:149: std::vector<char>* buffer,
On 2013/08/21 11:49:47, gavinp wrote:
> char*! Let your inner C programmer out.

Ahaha :) I got rid of the buffer.

[gavinp, 2013-08-21 15:31:58]

lgtm

https://codereview.chromium.org/22927018/diff/39001/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_index_file.h (right):

https://codereview.chromium.org/22927018/diff/39001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index_file.h:127: int data_len,
Nit: don't do this change. It's acceptable for closely related parameters to be
on the same line.

[Philippe, 2013-08-21 15:37:50]

Thanks Gavin! I'm waiting for you Egor before landing :)

https://codereview.chromium.org/22927018/diff/39001/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_index_file.h (right):

https://codereview.chromium.org/22927018/diff/39001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index_file.h:127: int data_len,
On 2013/08/21 15:31:58, gavinp wrote:
> Nit: don't do this change. It's acceptable for closely related parameters to
be
> on the same line.

Done.

[pasko, 2013-08-21 16:03:17]

some more annoyance from Egor .. :)

https://codereview.chromium.org/22927018/diff/39001/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_index_file.cc (right):

https://codereview.chromium.org/22927018/diff/39001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index_file.cc:86: LOG(ERROR) << "unexpected file "
<< file_name_string;
it would be easy to forget to update this check when updating the structure of
the cache directory with something new, say new directory. So expect firing this
"useful" error message a bit later.

I'd just remove it.

https://codereview.chromium.org/22927018/diff/39001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index_file.cc:98: // TODO(felipeg): Should we
delete the invalid file here ?
I am absolutely sure we must *not* delete the invalid file. Please remove the
TODO.

https://codereview.chromium.org/22927018/diff/39001/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_index_file_posix.cc (right):

https://codereview.chromium.org/22927018/diff/39001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index_file_posix.cc:32: const ScopedDir
dir(opendir(cache_path.value().c_str()));
this would closedir(NULL) if opendir() failed, is it intentional?

https://codereview.chromium.org/22927018/diff/39001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index_file_posix.cc:34: PLOG(ERROR) << "opendir "
<< cache_path.value();
I believe you can do:

PLOG(ERROR) << "opendir";

perror would print the file name (I did not check manually with PLOG though)

https://codereview.chromium.org/22927018/diff/39001/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_index_file_win.cc (right):

https://codereview.chromium.org/22927018/diff/39001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index_file_win.cc:18: const int kFileSuffixLength =
sizeof("_0") - 1;
this constant is unused

also, the operation of the posix/windows functions is different which may break
assumptions that other devs would be doing while looking at only one impl. How
about in the windows version making the "*" pattern and ignoring "." and ".."?

[Philippe, 2013-08-21 16:20:58]

Thanks Egor!

https://codereview.chromium.org/22927018/diff/39001/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_index_file.cc (right):

https://codereview.chromium.org/22927018/diff/39001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index_file.cc:86: LOG(ERROR) << "unexpected file "
<< file_name_string;
On 2013/08/21 16:03:17, pasko wrote:
> it would be easy to forget to update this check when updating the structure of
> the cache directory with something new, say new directory. So expect firing
this
> "useful" error message a bit later.
> 
> I'd just remove it.

Done.

https://codereview.chromium.org/22927018/diff/39001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index_file.cc:98: // TODO(felipeg): Should we
delete the invalid file here ?
On 2013/08/21 16:03:17, pasko wrote:
> I am absolutely sure we must *not* delete the invalid file. Please remove the
> TODO.

Done.

https://codereview.chromium.org/22927018/diff/39001/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_index_file_posix.cc (right):

https://codereview.chromium.org/22927018/diff/39001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index_file_posix.cc:32: const ScopedDir
dir(opendir(cache_path.value().c_str()));
On 2013/08/21 16:03:17, pasko wrote:
> this would closedir(NULL) if opendir() failed, is it intentional?

Yeah, it is :) scoped_ptr<>.reset() only calls the deleter if the pointer is
non-null. But good catch anyway :)

https://codereview.chromium.org/22927018/diff/39001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index_file_posix.cc:34: PLOG(ERROR) << "opendir "
<< cache_path.value();
On 2013/08/21 16:03:17, pasko wrote:
> I believe you can do:
> 
> PLOG(ERROR) << "opendir";
> 
> perror would print the file name (I did not check manually with PLOG though)
> 

Are you sure perror() prints the filename? To me it was just mapping errno to a
string. I'm 99% sure it doesn't print the name. And I did see a few of my
PLOG(ERROR) messages printed correctly (i.e. with a single occurrence of the
file path).

https://codereview.chromium.org/22927018/diff/39001/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_index_file_win.cc (right):

https://codereview.chromium.org/22927018/diff/39001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index_file_win.cc:18: const int kFileSuffixLength =
sizeof("_0") - 1;
On 2013/08/21 16:03:17, pasko wrote:
> this constant is unused
> 
> also, the operation of the posix/windows functions is different which may
break
> assumptions that other devs would be doing while looking at only one impl. How
> about in the windows version making the "*" pattern and ignoring "." and ".."?

Oops :) Yeah good point.

[pasko, 2013-08-21 16:29:27]

lgtm, thanks!

https://codereview.chromium.org/22927018/diff/39001/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_index_file_posix.cc (right):

https://codereview.chromium.org/22927018/diff/39001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index_file_posix.cc:32: const ScopedDir
dir(opendir(cache_path.value().c_str()));
On 2013/08/21 16:20:58, Philippe wrote:
> On 2013/08/21 16:03:17, pasko wrote:
> > this would closedir(NULL) if opendir() failed, is it intentional?
> 
> Yeah, it is :) scoped_ptr<>.reset() only calls the deleter if the pointer is
> non-null. But good catch anyway :)

thanks for explanation!

https://codereview.chromium.org/22927018/diff/39001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index_file_posix.cc:34: PLOG(ERROR) << "opendir "
<< cache_path.value();
On 2013/08/21 16:20:58, Philippe wrote:
> On 2013/08/21 16:03:17, pasko wrote:
> > I believe you can do:
> > 
> > PLOG(ERROR) << "opendir";
> > 
> > perror would print the file name (I did not check manually with PLOG though)
> > 
> 
> Are you sure perror() prints the filename? To me it was just mapping errno to
a
> string. I'm 99% sure it doesn't print the name. And I did see a few of my
> PLOG(ERROR) messages printed correctly (i.e. with a single occurrence of the
> file path).

you are right, it does not print the dir name for opendir failures, ugh

[commit-bot: I haz the power, 2013-08-22 11:09:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pliard@chromium.org/22927018/34008

[Philippe, 2013-08-22 14:24:35]

Committed patchset #8 manually as r219017 (presubmit successful).

[gavinp, 2013-08-22 14:33:17]

+congratbot


On Thu, Aug 22, 2013 at 10:24 AM, <pliard@chromium.org> wrote:

> Committed patchset #8 manually as r219017 (presubmit successful).
>
>
https://codereview.chromium.**org/22927018/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.