Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10454093:
Disk cache: Pre-read file headers when opening files (posix). (Closed)
|
Created:
8 years, 6 months ago by rvargas (doing something else) Modified:
8 years, 6 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionDisk cache: Pre-read file headers when opening files (posix).
BUG=128140
TEST=none
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=141245
Patch Set 1 #
Total comments: 6
Patch Set 2 : #
Total comments: 1
Patch Set 3 : #Messages
Total messages: 11 (0 generated)
http://codereview.chromium.org/10454093/diff/1/net/disk_cache/mapped_file_pos... File net/disk_cache/mapped_file_posix.cc (right): http://codereview.chromium.org/10454093/diff/1/net/disk_cache/mapped_file_pos... net/disk_cache/mapped_file_posix.cc:32: size_t temp_len = size ? size : 4096 * 2; Isn't it just an error if size is 0 here? How would File::GetLength() return 0, but we have a file that allows an 8k read without error? http://codereview.chromium.org/10454093/diff/1/net/disk_cache/mapped_file_win.cc File net/disk_cache/mapped_file_win.cc (right): http://codereview.chromium.org/10454093/diff/1/net/disk_cache/mapped_file_win... net/disk_cache/mapped_file_win.cc:31: size_t temp_len = size ? size : 4096 * 2; This is twice as good! Could we ever have a file less than 8k here?
Thanks. http://codereview.chromium.org/10454093/diff/1/net/disk_cache/mapped_file_pos... File net/disk_cache/mapped_file_posix.cc (right): http://codereview.chromium.org/10454093/diff/1/net/disk_cache/mapped_file_pos... net/disk_cache/mapped_file_posix.cc:32: size_t temp_len = size ? size : 4096 * 2; On 2012/05/31 19:59:50, gavinp wrote: > Isn't it just an error if size is 0 here? How would File::GetLength() return 0, > but we have a file that allows an 8k read without error? Well... that's one of the details that I was hoping you picked when I asked you to look at this function the other day. For some reason, it seems to work "as unexpected". We pass 0 all the time, expecting the whole file to be mapped. If you think that should be changed, we should do that on a separate CL. http://codereview.chromium.org/10454093/diff/1/net/disk_cache/mapped_file_win.cc File net/disk_cache/mapped_file_win.cc (right): http://codereview.chromium.org/10454093/diff/1/net/disk_cache/mapped_file_win... net/disk_cache/mapped_file_win.cc:31: size_t temp_len = size ? size : 4096 * 2; On 2012/05/31 19:59:50, gavinp wrote: > This is twice as good! Could we ever have a file less than 8k here? Nope, the header size is 8k.
http://codereview.chromium.org/10454093/diff/1/net/disk_cache/mapped_file_pos... File net/disk_cache/mapped_file_posix.cc (right): http://codereview.chromium.org/10454093/diff/1/net/disk_cache/mapped_file_pos... net/disk_cache/mapped_file_posix.cc:32: size_t temp_len = size ? size : 4096 * 2; If I missed a request to help earlier, I'm very sorry; but I'm here now! Passing zero should work; I commented the line up above where it gets filled in, and why I think the test here is not required. In the windows version, the test is needed though. On 2012/05/31 20:07:05, rvargas wrote: > On 2012/05/31 19:59:50, gavinp wrote: > > Isn't it just an error if size is 0 here? How would File::GetLength() return > 0, > > but we have a file that allows an 8k read without error? > > Well... that's one of the details that I was hoping you picked when I asked you > to look at this function the other day. > > For some reason, it seems to work "as unexpected". We pass 0 all the time, > expecting the whole file to be mapped. > > If you think that should be changed, we should do that on a separate CL.
woops, one comment didn't make it out in that last mailing http://codereview.chromium.org/10454093/diff/1/net/disk_cache/mapped_file_pos... File net/disk_cache/mapped_file_posix.cc (right): http://codereview.chromium.org/10454093/diff/1/net/disk_cache/mapped_file_pos... net/disk_cache/mapped_file_posix.cc:22: size = GetLength(); Here's where size gets set. GetLength() returns zero on error or a file of length zero. So I think we shouldn't use temp_len below in the posix case.
I apologize for making a mess here. It all boils down to missing a detail when I looked at the code a few weeks ago, and thinking about asking you to take a look but then not doing that :( Please take another look.
Just wondering now about the differences between posix and windows... http://codereview.chromium.org/10454093/diff/11001/net/disk_cache/mapped_file... File net/disk_cache/mapped_file_posix.cc (right): http://codereview.chromium.org/10454093/diff/11001/net/disk_cache/mapped_file... net/disk_cache/mapped_file_posix.cc:33: scoped_array<char> temp(new char[size]); Now there's another difference with windows. When size is passed as 0 to the MappedFile::Init on windows, we just read the 8192 byte header. But for Posix, we read the whole file. Will that slow us down on Posix?
On 2012/06/01 12:00:34, gavinp wrote: > Just wondering now about the differences between posix and windows... > > http://codereview.chromium.org/10454093/diff/11001/net/disk_cache/mapped_file... > File net/disk_cache/mapped_file_posix.cc (right): > > http://codereview.chromium.org/10454093/diff/11001/net/disk_cache/mapped_file... > net/disk_cache/mapped_file_posix.cc:33: scoped_array<char> temp(new char[size]); > Now there's another difference with windows. When size is passed as 0 to the > MappedFile::Init on windows, we just read the 8192 byte header. But for Posix, > we read the whole file. Will that slow us down on Posix? Good point. It shouldn't be slower, but I just went back to 4k instead of the whole file. background: When we pass a size, it is the header size (8K) When we want the whole file, we are talking about the index file. We want to prevent errors reading the header (<4k), and before init completes, the whole file is read in one operation (if everything is ok).
LGTM. I very much like that a change that only touches the posix builds is failing win_rel. That's pretty advanced.
I know it's not the reason, but http://en.wikipedia.org/wiki/Interix On Sat, Jun 2, 2012 at 3:22 PM, <gavinp@chromium.org> wrote: > LGTM. > > I very much like that a change that only touches the posix builds is > failing > win_rel. That's pretty advanced. > > http://codereview.chromium.**org/10454093/<http://codereview.chromium.org/104... >
Indeed. When I think POSIX, I think win32. :D On 2012/06/02 23:57:44, cbentzel wrote: > I know it's not the reason, but http://en.wikipedia.org/wiki/Interix > > On Sat, Jun 2, 2012 at 3:22 PM, <mailto:gavinp@chromium.org> wrote: > > > LGTM. > > > > I very much like that a change that only touches the posix builds is > > failing > > win_rel. That's pretty advanced. > > > > > http://codereview.chromium.**org/10454093/%3Chttp://codereview.chromium.org/1...> > > |
