Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1412673006:
Transform ImagePreReader into PreReadFile. (Closed)
DescriptionTransform ImagePreReader into PreReadFile.
We can reduce startup time by pre-reading different non-PE files
(e.g. ChromeDWriteFontCache, GPU cache, resources).
This CL transforms ImagePreReader into PreReadFile, which can
pre-read any file type on Vista+. On XP, PreReadFile can only
pre-read PE files.
BUG=547794
Patch Set 1 #Patch Set 2 : git cl format #
Total comments: 16
Patch Set 3 : rebase #Patch Set 4 : grt comments #3 #Patch Set 5 : fix style #
Total comments: 24
Patch Set 6 : grt comments #9 (style) #Patch Set 7 : fix compile error. #
Total comments: 4
Patch Set 8 : grt comments #12 (nits) #Patch Set 9 : Use LoadLibraryEx on XP. #Patch Set 10 : Fix comments. #
Total comments: 3
Messages
Total messages: 26 (10 generated)
fdoray@chromium.org changed reviewers: + grt@chromium.org
grt@: Could you review this CL? CC: rogerm@, with who I discussed this change offline.
https://codereview.chromium.org/1412673006/diff/20001/chrome/app/file_pre_rea... File chrome/app/file_pre_reader_win.cc (right): https://codereview.chromium.org/1412673006/diff/20001/chrome/app/file_pre_rea... chrome/app/file_pre_reader_win.cc:35: volatile uint8 const* touch_ptr = reinterpret_cast<uint8 const*>(base_addr); uint8_t here and elsewhere https://codereview.chromium.org/1412673006/diff/20001/chrome/app/file_pre_rea... chrome/app/file_pre_reader_win.cc:56: base::win::ScopedHandle file( please add a comment that base::File isn't used because of FILE_FLAG_SEQUENTIAL_SCAN. better yet, add support for FILE_FLAG_SEQUENTIAL_SCAN and use: base::File file(file_path, FLAG_OPEN | FLAG_READ | FLAG_SHARE_DELETE | FLAG_SEQUENTIAL_SCAN); https://codereview.chromium.org/1412673006/diff/20001/chrome/app/file_pre_rea... chrome/app/file_pre_reader_win.cc:74: size_t total_read = 0; file sizes are 64bits on windows, so uint64_t. better yet, this variable is never used. get rid of it? https://codereview.chromium.org/1412673006/diff/20001/chrome/app/file_pre_rea... chrome/app/file_pre_reader_win.cc:86: CHECK(file_memory_map.Initialize(base::FilePath(file_path))); why crash here but not on line 62? https://codereview.chromium.org/1412673006/diff/20001/chrome/app/file_pre_rea... File chrome/app/file_pre_reader_win.h (right): https://codereview.chromium.org/1412673006/diff/20001/chrome/app/file_pre_rea... chrome/app/file_pre_reader_win.h:11: #include "base/basictypes.h" unused https://codereview.chromium.org/1412673006/diff/20001/chrome/app/file_pre_rea... chrome/app/file_pre_reader_win.h:13: class FilePreReader { make this a free function rather than a class static (https://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_a...). https://codereview.chromium.org/1412673006/diff/20001/chrome/app/file_pre_rea... chrome/app/file_pre_reader_win.h:17: // each touched byte. how should the caller pick a value for |step_size|? can you explain this a bit in the comment? https://codereview.chromium.org/1412673006/diff/20001/chrome/app/file_pre_rea... chrome/app/file_pre_reader_win.h:21: static bool PreReadImage(const wchar_t* file_path, size_t step_size); why "Image" in the name when this is file_pre_reader? could this be PreReadFile?
fdoray@chromium.org changed reviewers: + jochen@chromium.org
grt@: All done. Could you take another look? +jochen@: Could you review chrome/BUILD.gn? Thanks. https://codereview.chromium.org/1412673006/diff/20001/chrome/app/file_pre_rea... File chrome/app/file_pre_reader_win.cc (right): https://codereview.chromium.org/1412673006/diff/20001/chrome/app/file_pre_rea... chrome/app/file_pre_reader_win.cc:35: volatile uint8 const* touch_ptr = reinterpret_cast<uint8 const*>(base_addr); On 2015/10/29 19:15:06, grt wrote: > uint8_t here and elsewhere Done. https://codereview.chromium.org/1412673006/diff/20001/chrome/app/file_pre_rea... chrome/app/file_pre_reader_win.cc:56: base::win::ScopedHandle file( On 2015/10/29 19:15:07, grt wrote: > please add a comment that base::File isn't used because of > FILE_FLAG_SEQUENTIAL_SCAN. better yet, add support for FILE_FLAG_SEQUENTIAL_SCAN > and use: > base::File file(file_path, FLAG_OPEN | FLAG_READ | FLAG_SHARE_DELETE | > FLAG_SEQUENTIAL_SCAN); Done. base::File::FLAG_SEQUENTIAL_SCAN was added in this CL https://codereview.chromium.org/1424943006/ https://codereview.chromium.org/1412673006/diff/20001/chrome/app/file_pre_rea... chrome/app/file_pre_reader_win.cc:74: size_t total_read = 0; On 2015/10/29 19:15:06, grt wrote: > file sizes are 64bits on windows, so uint64_t. > better yet, this variable is never used. get rid of it? Done. https://codereview.chromium.org/1412673006/diff/20001/chrome/app/file_pre_rea... chrome/app/file_pre_reader_win.cc:86: CHECK(file_memory_map.Initialize(base::FilePath(file_path))); On 2015/10/29 19:15:06, grt wrote: > why crash here but not on line 62? Done. We shouldn't crash. https://codereview.chromium.org/1412673006/diff/20001/chrome/app/file_pre_rea... File chrome/app/file_pre_reader_win.h (right): https://codereview.chromium.org/1412673006/diff/20001/chrome/app/file_pre_rea... chrome/app/file_pre_reader_win.h:11: #include "base/basictypes.h" On 2015/10/29 19:15:07, grt wrote: > unused Done. https://codereview.chromium.org/1412673006/diff/20001/chrome/app/file_pre_rea... chrome/app/file_pre_reader_win.h:13: class FilePreReader { On 2015/10/29 19:15:07, grt wrote: > make this a free function rather than a class static > (https://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_a...). Done. https://codereview.chromium.org/1412673006/diff/20001/chrome/app/file_pre_rea... chrome/app/file_pre_reader_win.h:17: // each touched byte. On 2015/10/29 19:15:07, grt wrote: > how should the caller pick a value for |step_size|? can you explain this a bit > in the comment? Done. Eventually, we should remove the parameter and always use 4 MB. But first, I want to make a Canary experiment to make sure that this is the best value. https://codereview.chromium.org/1412673006/diff/20001/chrome/app/file_pre_rea... chrome/app/file_pre_reader_win.h:21: static bool PreReadImage(const wchar_t* file_path, size_t step_size); On 2015/10/29 19:15:07, grt wrote: > why "Image" in the name when this is file_pre_reader? could this be PreReadFile? Done. https://codereview.chromium.org/1412673006/diff/80001/chrome/app/file_pre_rea... File chrome/app/file_pre_reader_win.h (right): https://codereview.chromium.org/1412673006/diff/80001/chrome/app/file_pre_rea... chrome/app/file_pre_reader_win.h:19: // about 4MB according to local tests), but also the more memory is allocated I plan to do a Canary experiment to find the best value. Once I know the best value, I will remove the |step_size| arg and always use the best value. Note: On a test laptop, files are pre-read ~25% faster when I use 4MB instead of 1MB (the value that we currently use).
Description was changed from ========== Transform ImagePreReader into a FilePreReader. We can reduce startup time by pre-reading different non-PE files (e.g. ChromeDWriteFontCache, GPU cache, resources). This CL transforms ImagePreReader into a FilePreReader, which can pre-read any file type. BUG=547794 ========== to ========== Transform ImagePreReader into a PreReadFile. We can reduce startup time by pre-reading different non-PE files (e.g. ChromeDWriteFontCache, GPU cache, resources). This CL transforms ImagePreReader into FilePreRead, which can pre-read any file type. BUG=547794 ==========
Description was changed from ========== Transform ImagePreReader into a PreReadFile. We can reduce startup time by pre-reading different non-PE files (e.g. ChromeDWriteFontCache, GPU cache, resources). This CL transforms ImagePreReader into FilePreRead, which can pre-read any file type. BUG=547794 ========== to ========== Transform ImagePreReader into a PreReadFile. We can reduce startup time by pre-reading different non-PE files (e.g. ChromeDWriteFontCache, GPU cache, resources). This CL transforms ImagePreReader into PreReadFile, which can pre-read any file type. BUG=547794 ==========
Description was changed from ========== Transform ImagePreReader into a PreReadFile. We can reduce startup time by pre-reading different non-PE files (e.g. ChromeDWriteFontCache, GPU cache, resources). This CL transforms ImagePreReader into PreReadFile, which can pre-read any file type. BUG=547794 ========== to ========== Transform ImagePreReader into PreReadFile. We can reduce startup time by pre-reading different non-PE files (e.g. ChromeDWriteFontCache, GPU cache, resources). This CL transforms ImagePreReader into PreReadFile, which can pre-read any file type. BUG=547794 ==========
looks great. down to nits. https://codereview.chromium.org/1412673006/diff/80001/chrome/app/file_pre_rea... File chrome/app/file_pre_reader_win.cc (right): https://codereview.chromium.org/1412673006/diff/80001/chrome/app/file_pre_rea... chrome/app/file_pre_reader_win.cc:20: DCHECK(base_addr != NULL); DCHECK(base_addr); https://codereview.chromium.org/1412673006/diff/80001/chrome/app/file_pre_rea... chrome/app/file_pre_reader_win.cc:21: DCHECK(length > 0); DCHECK_GT https://codereview.chromium.org/1412673006/diff/80001/chrome/app/file_pre_rea... chrome/app/file_pre_reader_win.cc:41: while (touch_ptr < final_touch_ptr) { for (; touch_ptr < final_touch_ptr; touch_ptr += system_info.dwPageSize) dummy = *touch_ptr; https://codereview.chromium.org/1412673006/diff/80001/chrome/app/file_pre_rea... chrome/app/file_pre_reader_win.cc:51: DCHECK(step_size > 0); DCHECK_GT https://codereview.chromium.org/1412673006/diff/80001/chrome/app/file_pre_rea... chrome/app/file_pre_reader_win.cc:62: LPVOID buffer = ::VirtualAlloc(NULL, static_cast<DWORD>(step_size), nullptr https://codereview.chromium.org/1412673006/diff/80001/chrome/app/file_pre_rea... chrome/app/file_pre_reader_win.cc:64: if (buffer == NULL) nullptr or "if (!buffer)" https://codereview.chromium.org/1412673006/diff/80001/chrome/app/file_pre_rea... chrome/app/file_pre_reader_win.cc:67: while (file.ReadAtCurrentPos(buffer, step_size) > 0) { nit: while(...); https://codereview.chromium.org/1412673006/diff/80001/chrome/app/file_pre_rea... chrome/app/file_pre_reader_win.cc:77: if (!file_memory_map.Initialize(base::FilePath(file_path))) omit base::FilePath https://codereview.chromium.org/1412673006/diff/80001/chrome/app/file_pre_rea... File chrome/app/file_pre_reader_win.h (right): https://codereview.chromium.org/1412673006/diff/80001/chrome/app/file_pre_rea... chrome/app/file_pre_reader_win.h:13: #include "base/files/file_path.h" forward declare class FilePath (http://www.chromium.org/developers/coding-style#TOC-Forward-Declarations-vs.-...) https://codereview.chromium.org/1412673006/diff/80001/chrome/app/file_pre_rea... chrome/app/file_pre_reader_win.h:15: // Reads the file passed in to avoid touching the disk when the file is actually nit: "Reads |file_path| to avoid..." https://codereview.chromium.org/1412673006/diff/80001/chrome/app/file_pre_rea... chrome/app/file_pre_reader_win.h:21: bool PreReadFile(const base::FilePath& file_path, size_t step_size); size_t -> int as per .cc file
BUILD.gn lgtm
grt@: Done. Could you take another look? Thanks. https://codereview.chromium.org/1412673006/diff/80001/chrome/app/file_pre_rea... File chrome/app/file_pre_reader_win.cc (right): https://codereview.chromium.org/1412673006/diff/80001/chrome/app/file_pre_rea... chrome/app/file_pre_reader_win.cc:20: DCHECK(base_addr != NULL); On 2015/11/03 04:02:15, grt wrote: > DCHECK(base_addr); Done. https://codereview.chromium.org/1412673006/diff/80001/chrome/app/file_pre_rea... chrome/app/file_pre_reader_win.cc:21: DCHECK(length > 0); On 2015/11/03 04:02:15, grt wrote: > DCHECK_GT Done. https://codereview.chromium.org/1412673006/diff/80001/chrome/app/file_pre_rea... chrome/app/file_pre_reader_win.cc:41: while (touch_ptr < final_touch_ptr) { On 2015/11/03 04:02:15, grt wrote: > for (; touch_ptr < final_touch_ptr; touch_ptr += system_info.dwPageSize) > dummy = *touch_ptr; Done. https://codereview.chromium.org/1412673006/diff/80001/chrome/app/file_pre_rea... chrome/app/file_pre_reader_win.cc:51: DCHECK(step_size > 0); On 2015/11/03 04:02:14, grt wrote: > DCHECK_GT Done. https://codereview.chromium.org/1412673006/diff/80001/chrome/app/file_pre_rea... chrome/app/file_pre_reader_win.cc:62: LPVOID buffer = ::VirtualAlloc(NULL, static_cast<DWORD>(step_size), On 2015/11/03 04:02:15, grt wrote: > nullptr Done. https://codereview.chromium.org/1412673006/diff/80001/chrome/app/file_pre_rea... chrome/app/file_pre_reader_win.cc:64: if (buffer == NULL) On 2015/11/03 04:02:15, grt wrote: > nullptr or "if (!buffer)" Done. https://codereview.chromium.org/1412673006/diff/80001/chrome/app/file_pre_rea... chrome/app/file_pre_reader_win.cc:67: while (file.ReadAtCurrentPos(buffer, step_size) > 0) { On 2015/11/03 04:02:15, grt wrote: > nit: while(...); while(); generates a pre-submit warning. "Empty loop bodies should use {} or continue, but not a single semicolon." https://google.github.io/styleguide/cppguide.html#Loops_and_Switch_Statements https://codereview.chromium.org/1412673006/diff/80001/chrome/app/file_pre_rea... chrome/app/file_pre_reader_win.cc:77: if (!file_memory_map.Initialize(base::FilePath(file_path))) On 2015/11/03 04:02:15, grt wrote: > omit base::FilePath Done. https://codereview.chromium.org/1412673006/diff/80001/chrome/app/file_pre_rea... File chrome/app/file_pre_reader_win.h (right): https://codereview.chromium.org/1412673006/diff/80001/chrome/app/file_pre_rea... chrome/app/file_pre_reader_win.h:13: #include "base/files/file_path.h" On 2015/11/03 04:02:15, grt wrote: > forward declare class FilePath > (http://www.chromium.org/developers/coding-style#TOC-Forward-Declarations-vs.-...) Done. https://codereview.chromium.org/1412673006/diff/80001/chrome/app/file_pre_rea... chrome/app/file_pre_reader_win.h:15: // Reads the file passed in to avoid touching the disk when the file is actually On 2015/11/03 04:02:15, grt wrote: > nit: "Reads |file_path| to avoid..." Done. https://codereview.chromium.org/1412673006/diff/80001/chrome/app/file_pre_rea... chrome/app/file_pre_reader_win.h:21: bool PreReadFile(const base::FilePath& file_path, size_t step_size); On 2015/11/03 04:02:15, grt wrote: > size_t -> int as per .cc file Done.
lgtm w/ a nit and a question https://codereview.chromium.org/1412673006/diff/80001/chrome/app/file_pre_rea... File chrome/app/file_pre_reader_win.cc (right): https://codereview.chromium.org/1412673006/diff/80001/chrome/app/file_pre_rea... chrome/app/file_pre_reader_win.cc:67: while (file.ReadAtCurrentPos(buffer, step_size) > 0) { On 2015/11/03 21:23:39, fdoray wrote: > On 2015/11/03 04:02:15, grt wrote: > > nit: while(...); > > while(); generates a pre-submit warning. > > "Empty loop bodies should use {} or continue, but not a single semicolon." > https://google.github.io/styleguide/cppguide.html#Loops_and_Switch_Statements ah, i didn't know that. thanks. https://codereview.chromium.org/1412673006/diff/120001/chrome/app/file_pre_re... File chrome/app/file_pre_reader_win.cc (right): https://codereview.chromium.org/1412673006/diff/120001/chrome/app/file_pre_re... chrome/app/file_pre_reader_win.cc:65: while (file.ReadAtCurrentPos(reinterpret_cast<char*>(buffer), step_size) > does this end up looking better if buffer is a char* (with the cast on line 60)? https://codereview.chromium.org/1412673006/diff/120001/chrome/app/file_pre_re... File chrome/app/file_pre_reader_win.h (right): https://codereview.chromium.org/1412673006/diff/120001/chrome/app/file_pre_re... chrome/app/file_pre_reader_win.h:11: #include <stddef.h> unused
grt@: Done. Thanks for the review. Committing... https://codereview.chromium.org/1412673006/diff/120001/chrome/app/file_pre_re... File chrome/app/file_pre_reader_win.cc (right): https://codereview.chromium.org/1412673006/diff/120001/chrome/app/file_pre_re... chrome/app/file_pre_reader_win.cc:65: while (file.ReadAtCurrentPos(reinterpret_cast<char*>(buffer), step_size) > On 2015/11/03 21:32:50, grt wrote: > does this end up looking better if buffer is a char* (with the cast on line 60)? Yes, I think it looks better with the cast on line 60. https://codereview.chromium.org/1412673006/diff/120001/chrome/app/file_pre_re... File chrome/app/file_pre_reader_win.h (right): https://codereview.chromium.org/1412673006/diff/120001/chrome/app/file_pre_re... chrome/app/file_pre_reader_win.h:11: #include <stddef.h> On 2015/11/03 21:32:50, grt wrote: > unused Done.
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, grt@chromium.org Link to the patchset: https://codereview.chromium.org/1412673006/#ps140001 (title: "grt comments #12 (nits)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1412673006/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1412673006/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/031ed3ba5d22e04d7f8ad3641aed82bc9c17f8ae Cr-Commit-Position: refs/heads/master@{#357689}
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:140001) has been created in https://codereview.chromium.org/1411683010/ by fdoray@chromium.org. The reason for reverting is: This patch increases cold startup time on XP (crbug.com/551488). Local tests were done on Win8 only, so we didn't catch that regression before submitting the patch. It's strange that touching pages of a PE file loaded with ::LoadLibraryExW doesn't do the same thing as touching pages of a memory-mapped PE file..
Message was sent while issue was closed.
Description was changed from ========== Transform ImagePreReader into PreReadFile. We can reduce startup time by pre-reading different non-PE files (e.g. ChromeDWriteFontCache, GPU cache, resources). This CL transforms ImagePreReader into PreReadFile, which can pre-read any file type. BUG=547794 Committed: https://crrev.com/031ed3ba5d22e04d7f8ad3641aed82bc9c17f8ae Cr-Commit-Position: refs/heads/master@{#357689} ========== to ========== Transform ImagePreReader into PreReadFile. We can reduce startup time by pre-reading different non-PE files (e.g. ChromeDWriteFontCache, GPU cache, resources). This CL transforms ImagePreReader into PreReadFile, which can pre-read any file type. BUG=547794 Committed: https://crrev.com/031ed3ba5d22e04d7f8ad3641aed82bc9c17f8ae Cr-Commit-Position: refs/heads/master@{#357689} ==========
Description was changed from ========== Transform ImagePreReader into PreReadFile. We can reduce startup time by pre-reading different non-PE files (e.g. ChromeDWriteFontCache, GPU cache, resources). This CL transforms ImagePreReader into PreReadFile, which can pre-read any file type. BUG=547794 Committed: https://crrev.com/031ed3ba5d22e04d7f8ad3641aed82bc9c17f8ae Cr-Commit-Position: refs/heads/master@{#357689} ========== to ========== Transform ImagePreReader into PreReadFile. We can reduce startup time by pre-reading different non-PE files (e.g. ChromeDWriteFontCache, GPU cache, resources). This CL transforms ImagePreReader into PreReadFile, which can pre-read any file type on Vista+. On XP, PreReadFile can only pre-read PE files. This patch was already submitted as revision 357689. It was reverted because of an XP-only perf regression (crbug.com/551488). The new version of the CL loads files in memory using ::LoadLibraryExW instead of base::MemoryMappedFile. This was the existing behavior of ImagePreReader, so it should no longer cause a perf regression. BUG=547794 ==========
grt@: Could you review my changes to this CL? Thanks. The first version caused a perf regression on XP (crbug.com/551488). Unfortunately, I wasn't able to reproduce the regression on a test XP machine and chrome-perf-infra@ confirmed that we cannot submit perf try jobs to XP bots. In this new version of the CL, I decided to keep the XP branch of PreReadFile identical to what it was before. Hopefully, there won't be a perf regression on the bots with this code... Keeping the old implementation means that only PE can be pre-read on XP. At some points, I thought that the perf regression might have been caused by the for loop in TouchPagesInRange() being optimized when it wasn't before, but I confirmed that it is not the case. Let me know what you think of this new implementation.
looks good. one question and a pair of suggestions. https://codereview.chromium.org/1412673006/diff/180001/chrome/app/file_pre_re... File chrome/app/file_pre_reader_win.cc (right): https://codereview.chromium.org/1412673006/diff/180001/chrome/app/file_pre_re... chrome/app/file_pre_reader_win.cc:75: UINT previous_error_mode = ::SetErrorMode(SEM_FAILCRITICALERRORS); does the pre-read happen before ChromeMainDelegate::SandboxInitialized runs? i see that it adds in FAILCRITICALERRORS. https://codereview.chromium.org/1412673006/diff/180001/chrome/app/file_pre_re... chrome/app/file_pre_reader_win.cc:78: HMODULE dll_module = ::LoadLibraryExW( use ScopedNativeLibrary dll_module(::LoadLibraryExW(...)); to avoid the leak on line 90 (and remove the call to FreeLibrary on line 100). https://codereview.chromium.org/1412673006/diff/180001/chrome/app/file_pre_re... chrome/app/file_pre_reader_win.cc:95: const size_t dll_module_length = SizeOfImage is a 32-bit unsigned int for both 32-bit and 64-bit images. Can you get rid of this cast if you change TouchPagesInRange so that it takes the length as uint32_t?
could you please use a new CL for the reland? That way, the tool can properly track the l-g-t-ms etc Just upload the first version as initial patchset, and then the fixed version as second patchset.
Description was changed from ========== Transform ImagePreReader into PreReadFile. We can reduce startup time by pre-reading different non-PE files (e.g. ChromeDWriteFontCache, GPU cache, resources). This CL transforms ImagePreReader into PreReadFile, which can pre-read any file type on Vista+. On XP, PreReadFile can only pre-read PE files. This patch was already submitted as revision 357689. It was reverted because of an XP-only perf regression (crbug.com/551488). The new version of the CL loads files in memory using ::LoadLibraryExW instead of base::MemoryMappedFile. This was the existing behavior of ImagePreReader, so it should no longer cause a perf regression. BUG=547794 ========== to ========== Transform ImagePreReader into PreReadFile. We can reduce startup time by pre-reading different non-PE files (e.g. ChromeDWriteFontCache, GPU cache, resources). This CL transforms ImagePreReader into PreReadFile, which can pre-read any file type on Vista+. On XP, PreReadFile can only pre-read PE files. BUG=547794 ==========
Message was sent while issue was closed.
Closing the CL. Continuing the review on a new CL, as requested by jochen@: https://codereview.chromium.org/1432973004/ |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
