Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9235053:
Add a PartialPreReadImage function to file_util (on Windows). (Closed)
|
Created:
8 years, 11 months ago by Roger McFarlane (Chromium) Modified:
5 years, 5 months ago CC:
chromium-reviews, brettw-cc_chromium.org, Evan Martin, darin (slow to review) Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionAdd partial pre-read functionality to browser startup (Windows).
PartialPreReadImage has much the smae interface as PreReadImage except the bytes to read becomes a percentage to read and it reads a certain percentage of each section in the binary.
Also adds some unittest coverage.
BUG=chromium:98508
TEST=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=120371
Patch Set 1 #Patch Set 2 : Fix some 'conversion may lose data' warnings #Patch Set 3 : Export some functions for unit-testing #Patch Set 4 : Use the pe_file interface instead of directly traversing the windows headers, add some DCHECKs #Patch Set 5 : Remove some superfluous/redundant error handling #Patch Set 6 : Fix a type conversion warning from the try bots #Patch Set 7 : Initial CL for Review #
Total comments: 36
Patch Set 8 : Address feedback from siggi and chrisha #
Total comments: 8
Patch Set 9 : Address Robert's feedback and refactor as inspired by Chris #
Total comments: 4
Patch Set 10 : Use PathService and fix a typo #Patch Set 11 : Enable partial pre-read experiment #
Total comments: 8
Patch Set 12 : Address Evan's comments #Patch Set 13 : rebase to master #Patch Set 14 : fix interaction between experiment and registry-forced pre-read settings #Patch Set 15 : rebase #Patch Set 16 : move image pre reader code to chrome/app #Patch Set 17 : Update the unit and perf tests #Patch Set 18 : tiny cleanup of includes and excess whitespace #Patch Set 19 : Rebase #Patch Set 20 : make image_pre_reader project windows only #
Total comments: 40
Patch Set 21 : De-conflate partial-pre-load change from base:file_util cleanup #Patch Set 22 : Address feedback from Chris and Siggi #
Total comments: 1
Messages
Total messages: 33 (1 generated)
PTAL. This CL introduces functions for partial pre-read. A subsequent CL will integrate them for experimentation.
Some comments and questions. https://chromiumcodereview.appspot.com/9235053/diff/6006/base/file_util_unitt... File base/file_util_unittest.cc (right): https://chromiumcodereview.appspot.com/9235053/diff/6006/base/file_util_unitt... base/file_util_unittest.cc:42: size_t percentage, indent - 1 https://chromiumcodereview.appspot.com/9235053/diff/6006/base/file_util_unitt... base/file_util_unittest.cc:706: ASSERT_TRUE(PreReadImage(module_path, file_size * 2, kStepSize)); Is there any reason why these specific parameter values are being tested? Is there any other way to test the behaviour of the of the function besides simply blasting parameters at it and checking true? (Can you mock out some inner function and make sure it's being called appropriately?) https://chromiumcodereview.appspot.com/9235053/diff/6006/base/file_util_unitt... base/file_util_unittest.cc:722: ASSERT_TRUE(PartialPreReadImage(module_path, 150, kStepSize)); Ditto for these tests and others. https://chromiumcodereview.appspot.com/9235053/diff/6006/base/file_util_win.cc File base/file_util_win.cc (right): https://chromiumcodereview.appspot.com/9235053/diff/6006/base/file_util_win.c... base/file_util_win.cc:1176: namespace internal { namespace internal is normally for .h files. Why not simply an anonymous namespace? https://chromiumcodereview.appspot.com/9235053/diff/6006/base/file_util_win.c... base/file_util_win.cc:1208: // by the optimization toolchain. Maybe break this down further? A PreReadChunk function that takes base address, offset and length? This could then be called per section by this wrapper function, and another that looks up percentages from somewhere in the PE file. https://chromiumcodereview.appspot.com/9235053/diff/6006/base/file_util_win.c... base/file_util_win.cc:1274: const IMAGE_SECTION_HEADER* section_headers = IMAGE_FIRST_SECTION(nt_headers); Could break out the header reading functionality to a utility function? https://chromiumcodereview.appspot.com/9235053/diff/6006/base/file_util_win.c... base/file_util_win.cc:1366: bool PartialPreReadImage(const wchar_t* file_path, Confusing function names: PartialPreReadImage vs PartialPreReadPeImage.
Nice. https://chromiumcodereview.appspot.com/9235053/diff/6006/base/file_util.h File base/file_util.h (right): https://chromiumcodereview.appspot.com/9235053/diff/6006/base/file_util.h#new... base/file_util.h:620: bool BASE_EXPORT PartialPreReadImage(const wchar_t* file_path, A bit of documentation? https://chromiumcodereview.appspot.com/9235053/diff/6006/base/file_util_unitt... File base/file_util_unittest.cc (right): https://chromiumcodereview.appspot.com/9235053/diff/6006/base/file_util_unitt... base/file_util_unittest.cc:41: bool PartialPreReadPeFile(const wchar_t* file_path, IMHO it's preferable to declare these in the file_util header. There's plenty of precedent in other headers for declaring and exporting functions from internal namespace: http://code.google.com/p/chromium/source/search?q=%22namespace+internal%22+fi... https://chromiumcodereview.appspot.com/9235053/diff/6006/base/file_util_unitt... base/file_util_unittest.cc:718: ASSERT_TRUE(PartialPreReadImage(module_path, 0, kStepSize)); For these tests, it would be more possible, and more useful to see what effect they have on the process working set, when loading and unloading a DLL that's otherwise not in memory. https://chromiumcodereview.appspot.com/9235053/diff/6006/base/file_util_win.cc File base/file_util_win.cc (right): https://chromiumcodereview.appspot.com/9235053/diff/6006/base/file_util_win.c... base/file_util_win.cc:1220: // On Windows Vista and higher, we sequentially read file contents with an copy/paste? https://chromiumcodereview.appspot.com/9235053/diff/6006/base/file_util_win.c... base/file_util_win.cc:1237: std::string headers(kInitialBufferSize, char()); nit: I like std::vector<uint8> or such for buffers, as this makes it explicit that we don't have string, per-se here. https://chromiumcodereview.appspot.com/9235053/diff/6006/base/file_util_win.c... base/file_util_win.cc:1274: const IMAGE_SECTION_HEADER* section_headers = IMAGE_FIRST_SECTION(nt_headers); You can construct a PEImage instance on the buffer, and use it to access the headers. https://chromiumcodereview.appspot.com/9235053/diff/6006/base/file_util_win.c... base/file_util_win.cc:1316: step_size = std::min(step_size, kMaxStepSize); I don't know that it's worth parametrizing the stride, as we'll always use PAGE_SIZE? https://chromiumcodereview.appspot.com/9235053/diff/6006/base/file_util_win.c... base/file_util_win.cc:1318: // WinXP branch. Here, reading the DLL from disk doesn't do copy/paste? https://chromiumcodereview.appspot.com/9235053/diff/6006/base/file_util_win.c... base/file_util_win.cc:1331: base::win::PEImage pe_image(dll_module); May want to CHECK(pe_image.VerifyMagic())? https://chromiumcodereview.appspot.com/9235053/diff/6006/base/file_util_win.c... base/file_util_win.cc:1338: size_t section_length = Maybe a one-line comment on why we don't only use VirtualSize? https://chromiumcodereview.appspot.com/9235053/diff/6006/base/file_util_win.c... base/file_util_win.cc:1354: touch = std::max(touch + step_size, max_touch); I don't understand, this would always return max_touch?
https://chromiumcodereview.appspot.com/9235053/diff/7007/base/file_util.h File base/file_util.h (right): https://chromiumcodereview.appspot.com/9235053/diff/7007/base/file_util.h#new... base/file_util.h:629: size_t percentage, does percentage need to be a size_t? https://chromiumcodereview.appspot.com/9235053/diff/7007/base/file_util_win.cc File base/file_util_win.cc (right): https://chromiumcodereview.appspot.com/9235053/diff/7007/base/file_util_win.c... base/file_util_win.cc:1151: // the DLL and touching pages at a stride. Consider adding a note indicating that we intentionally disregard step_size in this branch. https://chromiumcodereview.appspot.com/9235053/diff/7007/base/file_util_win.c... base/file_util_win.cc:1228: percentage = std::min(percentage, kOneHundredPercent); This line is redundant with lines 1223-1224. https://chromiumcodereview.appspot.com/9235053/diff/7007/base/file_util_win.c... base/file_util_win.cc:1327: const size_t kOneHundredPercent = 100; consider pulling this up out of the function since it is used in a couple of places
Thanks for the great feedback! PTA(nother)L I've refactored a bit (moved some code into helper functions), added more documentation/comments, and improved the DCHECKs and error handling. My apologies, as this makes the inter-patch diffs appear larger than they actually are. https://chromiumcodereview.appspot.com/9235053/diff/6006/base/file_util.h File base/file_util.h (right): https://chromiumcodereview.appspot.com/9235053/diff/6006/base/file_util.h#new... base/file_util.h:620: bool BASE_EXPORT PartialPreReadImage(const wchar_t* file_path, On 2012/01/26 18:59:20, Ruðrugis wrote: > A bit of documentation? Oops. Done. https://chromiumcodereview.appspot.com/9235053/diff/6006/base/file_util_unitt... File base/file_util_unittest.cc (right): https://chromiumcodereview.appspot.com/9235053/diff/6006/base/file_util_unitt... base/file_util_unittest.cc:41: bool PartialPreReadPeFile(const wchar_t* file_path, On 2012/01/26 18:59:20, Ruðrugis wrote: > IMHO it's preferable to declare these in the file_util header. There's plenty of > precedent in other headers for declaring and exporting functions from internal > namespace: > > http://code.google.com/p/chromium/source/search?q=%2522namespace+internal%252... Done. https://chromiumcodereview.appspot.com/9235053/diff/6006/base/file_util_unitt... base/file_util_unittest.cc:42: size_t percentage, On 2012/01/26 18:23:09, chrisha wrote: > indent - 1 Done. https://chromiumcodereview.appspot.com/9235053/diff/6006/base/file_util_unitt... base/file_util_unittest.cc:706: ASSERT_TRUE(PreReadImage(module_path, file_size * 2, kStepSize)); On 2012/01/26 18:23:09, chrisha wrote: > Is there any reason why these specific parameter values are being tested? Is > there any other way to test the behaviour of the of the function besides simply > blasting parameters at it and checking true? (Can you mock out some inner > function and make sure it's being called appropriately?) All I'm really testing is that the function returns without error for coverage of some "interesting" cases. Where I define interesting as (expressed in percentages). - valid edge cases (0%, 100%) - a valid case somewhere in the middle (25%, 50%)... chosen arbitrarily - a case that's otherwise invalid but should be handled gracefully (some value greater than 100%). Two slight wrinkles for this particular case is (1) there's no way to skip reading the file altogether and (2) specifying to read the full size by giving the file size can results in slightly different paging behaviour on XP vs Vista+, but shouldn't fail. Re: mocking out some interface exercised internally. A recent TOTT (or was it a LOTL?) highlighted this a testing anti-pattern, advocating testing of externally visible state changes instead of, more brittle, internal control-flow/algorithm validation. What I really want to test is the subsequent paging behavior with and without the pre-read. I'll have to investigate how to do that from the unittest (added as a TODO). https://chromiumcodereview.appspot.com/9235053/diff/6006/base/file_util_unitt... base/file_util_unittest.cc:718: ASSERT_TRUE(PartialPreReadImage(module_path, 0, kStepSize)); On 2012/01/26 18:59:20, Ruðrugis wrote: > For these tests, it would be more possible, and more useful to see what effect > they have on the process working set, when loading and unloading a DLL that's > otherwise not in memory. I'll look at doing that. Added a TODO for now. https://chromiumcodereview.appspot.com/9235053/diff/6006/base/file_util_unitt... base/file_util_unittest.cc:722: ASSERT_TRUE(PartialPreReadImage(module_path, 150, kStepSize)); On 2012/01/26 18:23:09, chrisha wrote: > Ditto for these tests and others. Ditto for my previous reply. https://chromiumcodereview.appspot.com/9235053/diff/6006/base/file_util_win.cc File base/file_util_win.cc (right): https://chromiumcodereview.appspot.com/9235053/diff/6006/base/file_util_win.c... base/file_util_win.cc:1176: namespace internal { On 2012/01/26 18:23:09, chrisha wrote: > namespace internal is normally for .h files. Why not simply an anonymous > namespace? Because then they wouldn't be accessible for unittesting (the PartialPreRead* helpers, I mean). The other's could certainly be put into an anonymous namespace. https://chromiumcodereview.appspot.com/9235053/diff/6006/base/file_util_win.c... base/file_util_win.cc:1208: // by the optimization toolchain. On 2012/01/26 18:23:09, chrisha wrote: > Maybe break this down further? A PreReadChunk function that takes base address, > offset and length? This could then be called per section by this wrapper > function, and another that looks up percentages from somewhere in the PE file. Refactored along the lines you suggest. I haven't gone all the way to looking up percentages in the PE file yet (left as a TODO). https://chromiumcodereview.appspot.com/9235053/diff/6006/base/file_util_win.c... base/file_util_win.cc:1220: // On Windows Vista and higher, we sequentially read file contents with an On 2012/01/26 18:59:20, Ruðrugis wrote: > copy/paste? At some point the comment still seemed relevant. Removed. https://chromiumcodereview.appspot.com/9235053/diff/6006/base/file_util_win.c... base/file_util_win.cc:1237: std::string headers(kInitialBufferSize, char()); On 2012/01/26 18:59:20, Ruðrugis wrote: > nit: I like std::vector<uint8> or such for buffers, as this makes it explicit > that we don't have string, per-se here. Done. https://chromiumcodereview.appspot.com/9235053/diff/6006/base/file_util_win.c... base/file_util_win.cc:1274: const IMAGE_SECTION_HEADER* section_headers = IMAGE_FIRST_SECTION(nt_headers); On 2012/01/26 18:59:20, Ruðrugis wrote: > You can construct a PEImage instance on the buffer, and use it to access the > headers. Done. Thanks! https://chromiumcodereview.appspot.com/9235053/diff/6006/base/file_util_win.c... base/file_util_win.cc:1274: const IMAGE_SECTION_HEADER* section_headers = IMAGE_FIRST_SECTION(nt_headers); On 2012/01/26 18:23:09, chrisha wrote: > Could break out the header reading functionality to a utility function? I've done some refactoring, PTAL. https://chromiumcodereview.appspot.com/9235053/diff/6006/base/file_util_win.c... base/file_util_win.cc:1316: step_size = std::min(step_size, kMaxStepSize); On 2012/01/26 18:59:20, Ruðrugis wrote: > I don't know that it's worth parametrizing the stride, as we'll always use > PAGE_SIZE? Done. Switched to always using the system page size. https://chromiumcodereview.appspot.com/9235053/diff/6006/base/file_util_win.c... base/file_util_win.cc:1318: // WinXP branch. Here, reading the DLL from disk doesn't do On 2012/01/26 18:59:20, Ruðrugis wrote: > copy/paste? At some point the comment still seemed relevant. Removed. https://chromiumcodereview.appspot.com/9235053/diff/6006/base/file_util_win.c... base/file_util_win.cc:1331: base::win::PEImage pe_image(dll_module); On 2012/01/26 18:59:20, Ruðrugis wrote: > May want to CHECK(pe_image.VerifyMagic())? Done. https://chromiumcodereview.appspot.com/9235053/diff/6006/base/file_util_win.c... base/file_util_win.cc:1338: size_t section_length = On 2012/01/26 18:59:20, Ruðrugis wrote: > Maybe a one-line comment on why we don't only use VirtualSize? Done. https://chromiumcodereview.appspot.com/9235053/diff/6006/base/file_util_win.c... base/file_util_win.cc:1354: touch = std::max(touch + step_size, max_touch); On 2012/01/26 18:59:20, Ruðrugis wrote: > I don't understand, this would always return max_touch? Oops, should have been std::min()... Nice catch! That said, I've tweaked the loop a bit to get rid of the compare/branches inside the loop. https://chromiumcodereview.appspot.com/9235053/diff/6006/base/file_util_win.c... base/file_util_win.cc:1366: bool PartialPreReadImage(const wchar_t* file_path, On 2012/01/26 18:23:09, chrisha wrote: > Confusing function names: PartialPreReadImage vs PartialPreReadPeImage. Renamed. Are the new names better? https://chromiumcodereview.appspot.com/9235053/diff/7007/base/file_util.h File base/file_util.h (right): https://chromiumcodereview.appspot.com/9235053/diff/7007/base/file_util.h#new... base/file_util.h:629: size_t percentage, On 2012/01/26 22:43:44, robertshield wrote: > does percentage need to be a size_t? No, it could be an uint8. Do you feel strongly that the use of size_t is inappropriate here? https://chromiumcodereview.appspot.com/9235053/diff/7007/base/file_util_win.cc File base/file_util_win.cc (right): https://chromiumcodereview.appspot.com/9235053/diff/7007/base/file_util_win.c... base/file_util_win.cc:1151: // the DLL and touching pages at a stride. On 2012/01/26 22:43:44, robertshield wrote: > Consider adding a note indicating that we intentionally disregard step_size in > this branch. Done. https://chromiumcodereview.appspot.com/9235053/diff/7007/base/file_util_win.c... base/file_util_win.cc:1228: percentage = std::min(percentage, kOneHundredPercent); On 2012/01/26 22:43:44, robertshield wrote: > This line is redundant with lines 1223-1224. Oops, was supposed to have been deleted. https://chromiumcodereview.appspot.com/9235053/diff/7007/base/file_util_win.c... base/file_util_win.cc:1327: const size_t kOneHundredPercent = 100; On 2012/01/26 22:43:44, robertshield wrote: > consider pulling this up out of the function since it is used in a couple of > places Done.
+jar (from OWNERS) @jar... We're looking to play with partial pre-read for chromium startup. Here's an initial CL to add some basic partial pre-read logic to experiment with. In particular, once Syzygy determines how much of the binary is actually read on startup, and globs it all together at the start of the PE sections, we should see an improvement from pre-reading in only as much as is needed. Our current data suggests that startup (to our event marker) only uses ~15% of the code and ~30% of the static data.
lgtm https://chromiumcodereview.appspot.com/9235053/diff/12007/base/file_util_unit... File base/file_util_unittest.cc (right): https://chromiumcodereview.appspot.com/9235053/diff/12007/base/file_util_unit... base/file_util_unittest.cc:675: // TODO(rogerm): Add checks to the *PreREadImage* tests to validate the nit: PreREadImage -> PreReadImage https://chromiumcodereview.appspot.com/9235053/diff/12007/base/file_util_unit... base/file_util_unittest.cc:683: ASSERT_TRUE(::GetModuleFileName(NULL, nit: this is available through PathService, see http://code.google.com/codesearch#OAMlx_jo-ck/src/base/base_paths.h&exact_pac...
Thanks! https://chromiumcodereview.appspot.com/9235053/diff/12007/base/file_util_unit... File base/file_util_unittest.cc (right): https://chromiumcodereview.appspot.com/9235053/diff/12007/base/file_util_unit... base/file_util_unittest.cc:675: // TODO(rogerm): Add checks to the *PreREadImage* tests to validate the On 2012/01/27 14:18:31, Ruðrugis wrote: > nit: PreREadImage -> PreReadImage Done. https://chromiumcodereview.appspot.com/9235053/diff/12007/base/file_util_unit... base/file_util_unittest.cc:683: ASSERT_TRUE(::GetModuleFileName(NULL, On 2012/01/27 14:18:31, Ruðrugis wrote: > nit: this is available through PathService, see > http://code.google.com/codesearch#OAMlx_jo-ck/src/base/base_paths.h&exact_pac... Done.
lgtm
lgtm
Thanks. I've added the code to enable the experiment. Please take a look at client_util.cc. All other files are, at present, unchanged from the set of LGTMs. Now I need to track down an owner, right?
lgtm
lgtm
Does this belong in base? Consider that base is code that is shared by all sorts of things in the Chrome family, from the Chrome window manager to the process that sticks around to run gmail in the background. Maybe all of this code belongs in its own file near where it's used -- the fact that it's so Windows-specific is a further hint towards that. What do you think? https://chromiumcodereview.appspot.com/9235053/diff/9008/base/file_util.h File base/file_util.h (right): https://chromiumcodereview.appspot.com/9235053/diff/9008/base/file_util.h#new... base/file_util.h:624: // the whole file is paged in via PreReadImage. The max_chunk_size, which This isn't clear to me -- does a percentage of 50 mean it only reads the first half of the file? Why is percentage a size_t when it is not a size? https://chromiumcodereview.appspot.com/9235053/diff/9008/base/file_util.h#new... base/file_util.h:628: bool BASE_EXPORT PartialPreReadImage(const wchar_t* file_path, Everywhere else I think we put BASE_EXPORT first. Can you also fix these the one function above, that you didn't change? https://chromiumcodereview.appspot.com/9235053/diff/9008/base/file_util_unitt... File base/file_util_unittest.cc (right): https://chromiumcodereview.appspot.com/9235053/diff/9008/base/file_util_unitt... base/file_util_unittest.cc:706: // The module_path is valid while current_exe is unchanged and in scope. Is this comment just pointing out that the wchar_t* remains valid for the body of the function? If so, I'd just omit it; we don't have such comments elsewhere. https://chromiumcodereview.appspot.com/9235053/diff/9008/base/file_util_win.cc File base/file_util_win.cc (right): https://chromiumcodereview.appspot.com/9235053/diff/9008/base/file_util_win.c... base/file_util_win.cc:1114: // we expect at least sizeof(IMAGE_DOS_HEADER) bytes, we're ok. This comment is about PE files, but it doesn't describe what the variable it's heading is for.
Thanks. Yeah, it would seem that base/file_util might not be the best place for this functionality to live in the long term. Right now it's, AFAICT, only called from chrome.exe on browser process on startup, to pave the way for itself and its children to load chrome.dll. We (the syzyty team) have some ideas we're actively exploring about where this functionality should ultimately end up, so I'd prefer to just leave it where it is for now with maybe a TODO to find it a better long term home. https://chromiumcodereview.appspot.com/9235053/diff/9008/base/file_util.h File base/file_util.h (right): https://chromiumcodereview.appspot.com/9235053/diff/9008/base/file_util.h#new... base/file_util.h:624: // the whole file is paged in via PreReadImage. The max_chunk_size, which On 2012/01/27 20:13:58, Evan Martin wrote: > This isn't clear to me -- does a percentage of 50 mean it only reads the first > half of the file? Why is percentage a size_t when it is not a size? Clarified the comments, and switched percentage to a uint8. https://chromiumcodereview.appspot.com/9235053/diff/9008/base/file_util.h#new... base/file_util.h:628: bool BASE_EXPORT PartialPreReadImage(const wchar_t* file_path, On 2012/01/27 20:13:58, Evan Martin wrote: > Everywhere else I think we put BASE_EXPORT first. Can you also fix these the > one function above, that you didn't change? Done. https://chromiumcodereview.appspot.com/9235053/diff/9008/base/file_util_unitt... File base/file_util_unittest.cc (right): https://chromiumcodereview.appspot.com/9235053/diff/9008/base/file_util_unitt... base/file_util_unittest.cc:706: // The module_path is valid while current_exe is unchanged and in scope. On 2012/01/27 20:13:58, Evan Martin wrote: > Is this comment just pointing out that the wchar_t* remains valid for the body > of the function? If so, I'd just omit it; we don't have such comments > elsewhere. Done. https://chromiumcodereview.appspot.com/9235053/diff/9008/base/file_util_win.cc File base/file_util_win.cc (right): https://chromiumcodereview.appspot.com/9235053/diff/9008/base/file_util_win.c... base/file_util_win.cc:1114: // we expect at least sizeof(IMAGE_DOS_HEADER) bytes, we're ok. On 2012/01/27 20:13:58, Evan Martin wrote: > This comment is about PE files, but it doesn't describe what the variable it's > heading is for. Fixed the comments and chose a better name for the constant.
On 2012/01/27 21:25:54, Roger McFarlane (Google) wrote: > We (the syzyty team) have some ideas we're actively exploring about where this > functionality should ultimately end up, so I'd prefer to just leave it where it > is for now with maybe a TODO to find it a better long term home. I also am not clear on where this belongs, but wouldn't you agree that that is a good argument for it to not to start in the global area? I am sorry to obstruct, I just know what happens to TODOs in this project. Even for a "small" library like base: $ grep TODO base/* | wc -l 132
maybe in chrome/app/client_util_win.* ? Siggi/Robert... your thoughts?
On 2012/01/27 21:46:53, Roger McFarlane (Google) wrote: > maybe in chrome/app/client_util_win.* ? > > Siggi/Robert... your thoughts? Windows-specific files in chrome/app/ sound good to me. Evan's right, I can't think of any compelling reason for keeping the pre-reading code in base. I'd bet it just got stuck there at first since base is sadly the defacto place for "code that might one day be shared".
On 2012/01/27 22:19:41, robertshield wrote: > On 2012/01/27 21:46:53, Roger McFarlane (Google) wrote: > > maybe in chrome/app/client_util_win.* ? > > > > Siggi/Robert... your thoughts? > > Windows-specific files in chrome/app/ sound good to me. > > Evan's right, I can't think of any compelling reason for keeping the pre-reading > code in base. I'd bet it just got stuck there at first since base is sadly the > defacto place for "code that might one day be shared". SGTM. That said, can I punt on moving the files 'til after the 18 branch point (which is tonight)? I'd prefer to land this reviewed CL then have a follow-on with the move.
I agree with Evan. At the very least, this doesn't belong in file_util.h. That is for basic file operations.
On 2012/01/27 22:19:41, robertshield wrote: > On 2012/01/27 21:46:53, Roger McFarlane (Google) wrote: > > maybe in chrome/app/client_util_win.* ? > > > > Siggi/Robert... your thoughts? > > Windows-specific files in chrome/app/ sound good to me. Do you only need this code in chrome.exe?
> SGTM. That said, can I punt on moving the files 'til after the 18 branch point > (which is tonight)? I'd prefer to land this reviewed CL then have a follow-on > with the move. I know this will make you unhappy, and I hate to cause you grief, but we already have a codebase that has grown up with many temporary hacks and TODOs. We need to stop the bleeding. Is this really a feature you were hoping to squeeze into M18? It seems like the time has past to try to push new features into M18.
On 2012/01/28 00:58:00, darin wrote: > > SGTM. That said, can I punt on moving the files 'til after the 18 branch point > > (which is tonight)? I'd prefer to land this reviewed CL then have a follow-on > > with the move. > > I know this will make you unhappy, and I hate to cause you grief, but we already > have a codebase that has grown up with many temporary hacks and TODOs. We need > to stop the bleeding. Indeed... an entirely reasonable response. :) > Is this really a feature you were hoping to squeeze into M18? It seems like the > time has past to try to push new features into M18. It's a modification of an existing optimization experiment. Instead of all vs none pre-read it's all vs partial pre-read of chrome.dll by the browser process on startup. Missing M18 wouldn't be the end of the world (although, it is 2012... just sayin'). We were just hoping to have this optimization in. > Do you only need this code in chrome.exe? This code is only referenced from chrome/app/client_util.cc and tests. http://code.google.com/p/chromium/source/search?q=PreReadImage
Some lab results and accompanying discussion can be found here. https://goto.google.com/syzygy-partial-pre-read-experiment
Renamed the issue... From: Add a PartialPreReadImage function to file_util (on Windows). To: Add partial pre-read functionality to browser startup (Windows). Moved the pre-read functionality out of base into chrome/app/image_pre_reader as files in a new projects (to facilitate unit and peft testing). Also added some additional perf tests. PTAL
http://codereview.chromium.org/9235053/diff/26020/chrome/app/client_util.cc File chrome/app/client_util.cc (right): http://codereview.chromium.org/9235053/diff/26020/chrome/app/client_util.cc#n... chrome/app/client_util.cc:216: key.ReadValueDW(L"PreReadPercentage", &pre_read_percentage); ubernit: Keep these in alphabetic order? http://codereview.chromium.org/9235053/diff/26020/chrome/app/client_util.cc#n... chrome/app/client_util.cc:255: std::min<DWORD>(pre_read ? 100 : pre_read_percentage, 100) & 0xFF); The & 0xFF is redundant at this point, given the clamping to <= 100. http://codereview.chromium.org/9235053/diff/26020/chrome/app/client_util.cc#n... chrome/app/client_util.cc:262: TRACE_EVENT_END_ETW("PreReadImage", 0, ""); Leave this surrounded by the "if (pre_read && percentage_to_read > 0)"? http://codereview.chromium.org/9235053/diff/26020/chrome/app/image_pre_reader... File chrome/app/image_pre_reader_win.cc (right): http://codereview.chromium.org/9235053/diff/26020/chrome/app/image_pre_reader... chrome/app/image_pre_reader_win.cc:8: No blank line here, as per the coding standard. However, convention trumps, if this is the way things are typically done in Chrome. http://codereview.chromium.org/9235053/diff/26020/chrome/app/image_pre_reader... chrome/app/image_pre_reader_win.cc:73: // @pre It is assumed that file has been used to sequentially populate that THE file http://codereview.chromium.org/9235053/diff/26020/chrome/app/image_pre_reader... chrome/app/image_pre_reader_win.cc:74: // @p current_buffer thus far and is already positioned at the Doxy-style comments are not used in Chrome, but rather parameter names are surrounded by pipes, ie |current_buffer|. http://codereview.chromium.org/9235053/diff/26020/chrome/app/image_pre_reader... File chrome/app/image_pre_reader_win.h (right): http://codereview.chromium.org/9235053/diff/26020/chrome/app/image_pre_reader... chrome/app/image_pre_reader_win.h:1: // Copyright (c) 2006-2011 The Chromium Authors. All rights reserved. New file: Copyright (c) 2012. http://codereview.chromium.org/9235053/diff/26020/chrome/app/image_pre_reader... chrome/app/image_pre_reader_win.h:49: // each step (for Vista and greater, where where this is the way the pages where where http://codereview.chromium.org/9235053/diff/26020/chrome/app/image_pre_reader... chrome/app/image_pre_reader_win.h:74: #endif // CHROME_APP_IMAGE_PRE_READER_WIN_H_ Great comments, btw! http://codereview.chromium.org/9235053/diff/26020/chrome_frame/chrome_frame.gyp File chrome_frame/chrome_frame.gyp (right): http://codereview.chromium.org/9235053/diff/26020/chrome_frame/chrome_frame.g... chrome_frame/chrome_frame.gyp:307: '../chrome/chrome.gyp:image_pre_reader', alphabetical order.
I suggest committing the cleanup of the files in base as a separate CL. We really want to get this change on the canaries ASAP, and that needn't block on the base cleanup. http://codereview.chromium.org/9235053/diff/26020/chrome/app/client_util.cc File chrome/app/client_util.cc (right): http://codereview.chromium.org/9235053/diff/26020/chrome/app/client_util.cc#n... chrome/app/client_util.cc:235: // specificaly set their pre-read options and is not participating in nit: specificaly->specifically http://codereview.chromium.org/9235053/diff/26020/chrome/app/image_pre_reader... File chrome/app/image_pre_reader_win.cc (right): http://codereview.chromium.org/9235053/diff/26020/chrome/app/image_pre_reader... chrome/app/image_pre_reader_win.cc:55: // given by @p file_handle into @p buffer. Chrome does not use doxy comments. http://codereview.chromium.org/9235053/diff/26020/chrome/app/image_pre_reader... chrome/app/image_pre_reader_win.cc:89: &((*current_buffer)[current_length]), nit: maybe a little less ugly as &(current_buffer->at(current_length))? http://codereview.chromium.org/9235053/diff/26020/chrome/app/image_pre_reader... chrome/app/image_pre_reader_win.cc:147: size_t max_chunk_size = temp_buffer_size; why the new variable? http://codereview.chromium.org/9235053/diff/26020/chrome/app/image_pre_reader... chrome/app/image_pre_reader_win.cc:153: maybe DCHECK(bytes_to_read <= temp_buffer_size)? http://codereview.chromium.org/9235053/diff/26020/chrome/app/image_pre_reader... chrome/app/image_pre_reader_win.cc:165: SYSTEM_INFO system_info; nit: initialize at declaration. http://codereview.chromium.org/9235053/diff/26020/chrome/app/image_pre_reader... chrome/app/image_pre_reader_win.cc:167: DCHECK(system_info.dwPageSize != 0) - which would lead to an infinite loop. For belt-and-suspenders you may even want to make exit on that condition - you never know what stupidity hookers and patchers can get up to in your process :(. http://codereview.chromium.org/9235053/diff/26020/chrome/app/image_pre_reader... chrome/app/image_pre_reader_win.cc:323: size_t size_to_read, nit: indent. http://codereview.chromium.org/9235053/diff/26020/chrome/app/image_pre_reader... File chrome/app/image_pre_reader_win.h (right): http://codereview.chromium.org/9235053/diff/26020/chrome/app/image_pre_reader... chrome/app/image_pre_reader_win.h:21: // subsequent hard page faults during LoadLibrary. The size to be pre read nit: pre read->pre-read http://codereview.chromium.org/9235053/diff/26020/chrome/app/image_pre_reader... chrome/app/image_pre_reader_win.h:39: // The percentage of the file to be read is an integral value between 0 and great comment! http://codereview.chromium.org/9235053/diff/24012/base/file_util_win.cc File base/file_util_win.cc (right): http://codereview.chromium.org/9235053/diff/24012/base/file_util_win.cc#newco... base/file_util_win.cc:1172: -} ?
Thanks. I've pulled out the base/file_util cleanup to another CL (see http://codereview.chromium.org/9309083/) and address Chris and Siggi's feedback. PTAL. http://codereview.chromium.org/9235053/diff/26020/chrome/app/client_util.cc File chrome/app/client_util.cc (right): http://codereview.chromium.org/9235053/diff/26020/chrome/app/client_util.cc#n... chrome/app/client_util.cc:216: key.ReadValueDW(L"PreReadPercentage", &pre_read_percentage); On 2012/02/03 16:31:08, chrisha wrote: > ubernit: Keep these in alphabetic order? Done. Also only read the sizes and percentage if there's a PreRead value. http://codereview.chromium.org/9235053/diff/26020/chrome/app/client_util.cc#n... chrome/app/client_util.cc:235: // specificaly set their pre-read options and is not participating in On 2012/02/03 16:36:39, Ruðrugis wrote: > nit: specificaly->specifically Done. http://codereview.chromium.org/9235053/diff/26020/chrome/app/client_util.cc#n... chrome/app/client_util.cc:255: std::min<DWORD>(pre_read ? 100 : pre_read_percentage, 100) & 0xFF); On 2012/02/03 16:31:08, chrisha wrote: > The & 0xFF is redundant at this point, given the clamping to <= 100. Done. http://codereview.chromium.org/9235053/diff/26020/chrome/app/client_util.cc#n... chrome/app/client_util.cc:262: TRACE_EVENT_END_ETW("PreReadImage", 0, ""); On 2012/02/03 16:31:08, chrisha wrote: > Leave this surrounded by the "if (pre_read && percentage_to_read > 0)"? So that's not quite the way the experiment works now. We're comparing pre-reading everthing (the status quo) with performing a partial pre-read. pre-read == 1: status_quo, the expression above selects 100 as the percentage pre-read == 0: partial, the expression selects the partial pre-read percentage. http://codereview.chromium.org/9235053/diff/26020/chrome/app/image_pre_reader... File chrome/app/image_pre_reader_win.cc (right): http://codereview.chromium.org/9235053/diff/26020/chrome/app/image_pre_reader... chrome/app/image_pre_reader_win.cc:8: On 2012/02/03 16:31:08, chrisha wrote: > No blank line here, as per the coding standard. However, convention trumps, if > this is the way things are typically done in Chrome. Done. http://codereview.chromium.org/9235053/diff/26020/chrome/app/image_pre_reader... chrome/app/image_pre_reader_win.cc:55: // given by @p file_handle into @p buffer. On 2012/02/03 16:36:39, Ruðrugis wrote: > Chrome does not use doxy comments. Done. http://codereview.chromium.org/9235053/diff/26020/chrome/app/image_pre_reader... chrome/app/image_pre_reader_win.cc:73: // @pre It is assumed that file has been used to sequentially populate On 2012/02/03 16:31:08, chrisha wrote: > that THE file Done. http://codereview.chromium.org/9235053/diff/26020/chrome/app/image_pre_reader... chrome/app/image_pre_reader_win.cc:74: // @p current_buffer thus far and is already positioned at the On 2012/02/03 16:31:08, chrisha wrote: > Doxy-style comments are not used in Chrome, but rather parameter names are > surrounded by pipes, ie |current_buffer|. Done. http://codereview.chromium.org/9235053/diff/26020/chrome/app/image_pre_reader... chrome/app/image_pre_reader_win.cc:89: &((*current_buffer)[current_length]), On 2012/02/03 16:36:39, Ruðrugis wrote: > nit: maybe a little less ugly as &(current_buffer->at(current_length))? Done. http://codereview.chromium.org/9235053/diff/26020/chrome/app/image_pre_reader... chrome/app/image_pre_reader_win.cc:147: size_t max_chunk_size = temp_buffer_size; On 2012/02/03 16:36:39, Ruðrugis wrote: > why the new variable? It seemed more readable at the time. Removed. http://codereview.chromium.org/9235053/diff/26020/chrome/app/image_pre_reader... chrome/app/image_pre_reader_win.cc:153: On 2012/02/03 16:36:39, Ruðrugis wrote: > maybe DCHECK(bytes_to_read <= temp_buffer_size)? Done. Also DCHECK that there are bytes left to read. http://codereview.chromium.org/9235053/diff/26020/chrome/app/image_pre_reader... chrome/app/image_pre_reader_win.cc:165: SYSTEM_INFO system_info; On 2012/02/03 16:36:39, Ruðrugis wrote: > nit: initialize at declaration. Done. http://codereview.chromium.org/9235053/diff/26020/chrome/app/image_pre_reader... chrome/app/image_pre_reader_win.cc:167: On 2012/02/03 16:36:39, Ruðrugis wrote: > DCHECK(system_info.dwPageSize != 0) - which would lead to an infinite loop. For > belt-and-suspenders you may even want to make exit on that condition - you never > know what stupidity hookers and patchers can get up to in your process :(. Handled as a defensive measure, with an explanatory comment. http://codereview.chromium.org/9235053/diff/26020/chrome/app/image_pre_reader... chrome/app/image_pre_reader_win.cc:323: size_t size_to_read, On 2012/02/03 16:36:39, Ruðrugis wrote: > nit: indent. Oops! Done... here and elsewhere. http://codereview.chromium.org/9235053/diff/26020/chrome/app/image_pre_reader... File chrome/app/image_pre_reader_win.h (right): http://codereview.chromium.org/9235053/diff/26020/chrome/app/image_pre_reader... chrome/app/image_pre_reader_win.h:1: // Copyright (c) 2006-2011 The Chromium Authors. All rights reserved. On 2012/02/03 16:31:08, chrisha wrote: > New file: Copyright (c) 2012. Done. http://codereview.chromium.org/9235053/diff/26020/chrome/app/image_pre_reader... chrome/app/image_pre_reader_win.h:21: // subsequent hard page faults during LoadLibrary. The size to be pre read On 2012/02/03 16:36:39, Ruðrugis wrote: > nit: pre read->pre-read Done. http://codereview.chromium.org/9235053/diff/26020/chrome/app/image_pre_reader... chrome/app/image_pre_reader_win.h:39: // The percentage of the file to be read is an integral value between 0 and On 2012/02/03 16:36:39, Ruðrugis wrote: > great comment! Thanks! http://codereview.chromium.org/9235053/diff/26020/chrome/app/image_pre_reader... chrome/app/image_pre_reader_win.h:49: // each step (for Vista and greater, where where this is the way the pages On 2012/02/03 16:31:08, chrisha wrote: > where where Done. http://codereview.chromium.org/9235053/diff/26020/chrome/app/image_pre_reader... chrome/app/image_pre_reader_win.h:74: #endif // CHROME_APP_IMAGE_PRE_READER_WIN_H_ On 2012/02/03 16:31:08, chrisha wrote: > Great comments, btw! Thanks! http://codereview.chromium.org/9235053/diff/26020/chrome_frame/chrome_frame.gyp File chrome_frame/chrome_frame.gyp (right): http://codereview.chromium.org/9235053/diff/26020/chrome_frame/chrome_frame.g... chrome_frame/chrome_frame.gyp:307: '../chrome/chrome.gyp:image_pre_reader', On 2012/02/03 16:31:08, chrisha wrote: > alphabetical order. Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rogerm@chromium.org/9235053/28018
Change committed as 120371
Message was sent while issue was closed.
thakis@chromium.org changed reviewers: + thakis@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/9235053/diff/28018/chrome/app/image_pre_reade... File chrome/app/image_pre_reader_win.cc (right): https://codereview.chromium.org/9235053/diff/28018/chrome/app/image_pre_reade... chrome/app/image_pre_reader_win.cc:33: min_header_buffer_size_at_least_as_big_as_the_dos_header); fyi, COMPILE_ASSERTs don't have to be in functions: https://codereview.chromium.org/1255073002/diff/1/chrome/app/image_pre_reader... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
