Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1001343002:
Prefetch the native library from native code by parsing /proc/pid/maps. (Closed)
|
Created:
5 years, 9 months ago by Benoit L Modified:
5 years, 7 months ago CC:
chromium-reviews, darin-cc_chromium.org, erikwright+watch_chromium.org, petrcermak Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPrefetch the native library from native code by parsing /proc/pid/maps.
Prefetching the native library by reading its pages speeds up startup on
Android (see the linked bug).
When loading the native library from the APK, there is no file to
associate with the native library. The most reliable way to prefetch the
native library pages is then to look at the pages mapped in the process
address space after the native library has been loaded.
To minimize disruption, the prefetch is done in a separate forked
process. This patch implements the parsing of the address space mappings
and the reading of pages from a forked process. Most of the work is done
before the fork() to avoid potential nasty issues associated with doing
work in a forked process in presence of threads.
BUG=460438
Committed: https://crrev.com/0a830d2548468ecbfb324644b7ecf9ddb46d4b42
Cr-Commit-Position: refs/heads/master@{#327473}
Patch Set 1 #Patch Set 2 : Don't rely on third party code that is not always there to parse /proc/pid/maps. #Patch Set 3 : Typo. #Patch Set 4 : . #Patch Set 5 : clang format. #Patch Set 6 : . #Patch Set 7 : . #
Total comments: 22
Patch Set 8 : Address comments. #
Total comments: 33
Patch Set 9 : Address comments. #Patch Set 10 : . #Patch Set 11 : Make the current prefetching a fallback in case forking goes wrong. #Patch Set 12 : Typo. #
Total comments: 6
Patch Set 13 : Address comments. #
Total comments: 41
Patch Set 14 : Address comments. #
Total comments: 2
Patch Set 15 : Address comments. #Patch Set 16 : . #
Total comments: 10
Patch Set 17 : Address comments, separate function, add tests to it. #Patch Set 18 : Re-use existing /proc/maps parsing code. #
Total comments: 10
Patch Set 19 : Address comments. #
Total comments: 4
Patch Set 20 : Move comment and disallow construction. #
Total comments: 15
Patch Set 21 : . #Patch Set 22 : Remove the fallback, add a TODO for a UMA count of failures. #Patch Set 23 : . #Patch Set 24 : Add GN files. #Patch Set 25 : . #Patch Set 26 : Missing BASE_EXPORT. #Messages
Total messages: 58 (10 generated)
rmcilroy@chromium.org changed reviewers: + rmcilroy@chromium.org
+petrcermak Just to let you know that Petr is working on a refactor of the libraryloader currently: https://docs.google.com/document/d/1kbSD83EttAhmCMFuUaIp0eVeEh-BoFbV6Bn7508je...
+petrcermak Just to let you know that Petr is working on a refactor of the libraryloader currently: https://docs.google.com/document/d/1kbSD83EttAhmCMFuUaIp0eVeEh-BoFbV6Bn7508je...
lizeb@chromium.org changed reviewers: + pasko@chromium.org
Hello, Please review this patch. Sorry about its size, but a lot of it is tests.
petrcermak@chromium.org changed reviewers: + petrcermak@chromium.org
A couple of (mostly style) comments. https://codereview.chromium.org/1001343002/diff/110001/base/android/java/src/... File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java (right): https://codereview.chromium.org/1001343002/diff/110001/base/android/java/src/... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:577: // Finds the ranges corresponding to the native library pages, and forks a nit: There shouldn't be a comma after "and". https://codereview.chromium.org/1001343002/diff/110001/base/android/java/src/... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:577: // Finds the ranges corresponding to the native library pages, and forks a I would say "... and then forks ..." to be a little clearer about the order. https://codereview.chromium.org/1001343002/diff/110001/base/android/java/src/... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:578: // new process to prefetch these pages. The process then terminates. Note I'd say "The new process..." to make it clear which process will terminate. https://codereview.chromium.org/1001343002/diff/110001/base/android/java/src/... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:580: // the calling thread. I would add "of the original process" after "calling thread". https://codereview.chromium.org/1001343002/diff/110001/base/android/library_l... File base/android/library_loader/library_prefetcher.cc (right): https://codereview.chromium.org/1001343002/diff/110001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:108: unsigned char* start_address = (unsigned char*)range.first; nit: There should be a space after the closing parenthesis before "range" (applies to the line below as well) https://codereview.chromium.org/1001343002/diff/110001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:112: addr += 4096) { Shouldn't this be a constant (e.g. kPageSize)? https://codereview.chromium.org/1001343002/diff/110001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:142: // - Isolating the main process from mistakes in the parsing. If the parsing This doesn't seem to be the case here. If I understand your code correctly, all the parsing is done inside FindRanges(), which is done *before* the fork. https://codereview.chromium.org/1001343002/diff/110001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:143: // returns an incorrect address, only the forked process will crash. nit: Incorrect indentation of this line. https://codereview.chromium.org/1001343002/diff/110001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:148: // The forked process has background priority, and since it is not declared to nit: The first comma should actually appear *after* the "and" here (to enclose "since ... runtime" in commas). https://codereview.chromium.org/1001343002/diff/110001/base/android/library_l... File base/android/library_loader/library_prefetcher.h (right): https://codereview.chromium.org/1001343002/diff/110001/base/android/library_l... base/android/library_loader/library_prefetcher.h:32: FileLineIterator() : file_(NULL){}; nullptr? https://codereview.chromium.org/1001343002/diff/110001/base/android/library_l... base/android/library_loader/library_prefetcher.h:66: // Finds the ranges matching the native library, and forks a low priority nit: There shouldn't be a comma before "and".
Thank you, all done. https://codereview.chromium.org/1001343002/diff/110001/base/android/java/src/... File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java (right): https://codereview.chromium.org/1001343002/diff/110001/base/android/java/src/... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:577: // Finds the ranges corresponding to the native library pages, and forks a On 2015/03/23 15:00:47, petrcermak wrote: > I would say "... and then forks ..." to be a little clearer about the order. Done. https://codereview.chromium.org/1001343002/diff/110001/base/android/java/src/... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:577: // Finds the ranges corresponding to the native library pages, and forks a On 2015/03/23 15:00:47, petrcermak wrote: > nit: There shouldn't be a comma after "and". Done. https://codereview.chromium.org/1001343002/diff/110001/base/android/java/src/... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:578: // new process to prefetch these pages. The process then terminates. Note On 2015/03/23 15:00:47, petrcermak wrote: > I'd say "The new process..." to make it clear which process will terminate. Done. https://codereview.chromium.org/1001343002/diff/110001/base/android/java/src/... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:580: // the calling thread. On 2015/03/23 15:00:47, petrcermak wrote: > I would add "of the original process" after "calling thread". Done. https://codereview.chromium.org/1001343002/diff/110001/base/android/library_l... File base/android/library_loader/library_prefetcher.cc (right): https://codereview.chromium.org/1001343002/diff/110001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:108: unsigned char* start_address = (unsigned char*)range.first; On 2015/03/23 15:00:48, petrcermak wrote: > nit: There should be a space after the closing parenthesis before "range" > (applies to the line below as well) Done. https://codereview.chromium.org/1001343002/diff/110001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:112: addr += 4096) { On 2015/03/23 15:00:47, petrcermak wrote: > Shouldn't this be a constant (e.g. kPageSize)? Done. https://codereview.chromium.org/1001343002/diff/110001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:142: // - Isolating the main process from mistakes in the parsing. If the parsing On 2015/03/23 15:00:47, petrcermak wrote: > This doesn't seem to be the case here. If I understand your code correctly, all > the parsing is done inside FindRanges(), which is done *before* the fork. I was talking about the fact that we might get incorrect addresses. Since actually accessing the addresses is done in a separate process, the parsing can be wrong without crashing the app. https://codereview.chromium.org/1001343002/diff/110001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:143: // returns an incorrect address, only the forked process will crash. On 2015/03/23 15:00:47, petrcermak wrote: > nit: Incorrect indentation of this line. Done. https://codereview.chromium.org/1001343002/diff/110001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:148: // The forked process has background priority, and since it is not declared to On 2015/03/23 15:00:47, petrcermak wrote: > nit: The first comma should actually appear *after* the "and" here (to enclose > "since ... runtime" in commas). Done. https://codereview.chromium.org/1001343002/diff/110001/base/android/library_l... File base/android/library_loader/library_prefetcher.h (right): https://codereview.chromium.org/1001343002/diff/110001/base/android/library_l... base/android/library_loader/library_prefetcher.h:32: FileLineIterator() : file_(NULL){}; On 2015/03/23 15:00:48, petrcermak wrote: > nullptr? Done. https://codereview.chromium.org/1001343002/diff/110001/base/android/library_l... base/android/library_loader/library_prefetcher.h:66: // Finds the ranges matching the native library, and forks a low priority On 2015/03/23 15:00:48, petrcermak wrote: > nit: There shouldn't be a comma before "and". Done.
Seems reasonable overall - a couple of comments. https://codereview.chromium.org/1001343002/diff/130001/base/android/library_l... File base/android/library_loader/library_prefetcher.cc (right): https://codereview.chromium.org/1001343002/diff/130001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:44: std::string* filename) { nit - this might be neater wrapping up the arguments as a struct. https://codereview.chromium.org/1001343002/diff/130001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:57: DCHECK((pieces_count == 5) || (pieces_count == 6)); Either do this as a CHECK or add an if to only to the code below if this is true (preferred I think), otherwise the code below could do an buffer-overflow on release builds if the format changes in the future. https://codereview.chromium.org/1001343002/diff/130001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:64: *end = strtoull(delimiter_ptr + 1, &dummy, 16); nit - I would prefer you just used base::SplitString here to split the start and end with the dash. https://codereview.chromium.org/1001343002/diff/130001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:89: return flags && flags[0] == 'r' && flags[3] == 'p'; nit - add a check here that flags is long enough to avoid buffer overflows. https://codereview.chromium.org/1001343002/diff/130001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:145: // returns an incorrect address, only the forked process will crash. The parsing is done on the main process though. https://codereview.chromium.org/1001343002/diff/130001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:149: // to evict, minimizing disruption caused by this prefetching. Does forking actually helps this - the pages should be clean even if they were Prefetched by the main process (unless the main process modifies them but in that case they wouldn't be clean and unreferenced even if they are prefetched by the forked process). https://codereview.chromium.org/1001343002/diff/130001/base/android/library_l... File base/android/library_loader/library_prefetcher.h (right): https://codereview.chromium.org/1001343002/diff/130001/base/android/library_l... base/android/library_loader/library_prefetcher.h:28: // empty lines. This seems slightly dangerous. Maybe just make NextLine return a bool for EOF signaling and fill in a std::string* argument. https://codereview.chromium.org/1001343002/diff/130001/base/android/library_l... base/android/library_loader/library_prefetcher.h:68: void ForkAndPrefetchNativeLibrary(); Could we wrap ForkAndPrefetchNativeLibrary as a static function in a class, then have ProcMapsIterator, FileLineIterator and FindRanges as private classes/functions and make ProcMapsIteratorTest a friend to minimize the API exposed for testing. https://codereview.chromium.org/1001343002/diff/130001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/init/NativeInitializationController.java (left): https://codereview.chromium.org/1001343002/diff/130001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/init/NativeInitializationController.java:94: libraryLoader.asyncPrefetchLibrariesToMemory(mContext); Can we delete asyncPrefetchLibrariesToMemory from the code now?
reasonable but a bit intense on iterators to my taste, see below https://codereview.chromium.org/1001343002/diff/130001/base/android/library_l... File base/android/library_loader/library_prefetcher.cc (right): https://codereview.chromium.org/1001343002/diff/130001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:30: char* line = fgets(line_buffer_, kMaxLineLength - 1, file_); why so low level and complex with FILE* and fgets etc? You can use ReadFileToString() and StringTokenizer. Then it is possible to use StringPiece. Amount of allocations would probably be the same in practice. https://codereview.chromium.org/1001343002/diff/130001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:62: "long long is not int64_t"); // This way we can use strtoll(). StringToInt64? https://codereview.chromium.org/1001343002/diff/130001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:86: const char* kSuffixesToMatch[] = {"libchrome.so", "base.apk"}; why base.apk? is the chrome apk always renamed to base.apk? Why not just .apk? https://codereview.chromium.org/1001343002/diff/130001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:113: for (unsigned char* addr = start_address; addr < end_address - sizeof(int); s/ - sizeof(int)// ? you can CHECK(!(start_address & 0xFFF) && !(end_address & 0xFFF)) to make it even more straightforward. https://codereview.chromium.org/1001343002/diff/130001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:136: ranges.push_back(std::make_pair(start, end)); We could potentially have both libchrome.so and .apk mapped, in this case we need to prefetch only libchrome.so (like on those Samsung devices where we cannot mmap from apk, it does not mean smb else could not mmap the apk in our process before us, right?) Having multiple ranges scares me, we are not deterministic at walking through the ranges, i.e. injecting a source of variance that'd be impossible to find. Can we just on encountering more than one range just record a histogram and walk away? https://codereview.chromium.org/1001343002/diff/130001/base/android/library_l... File base/android/library_loader/library_prefetcher.h (right): https://codereview.chromium.org/1001343002/diff/130001/base/android/library_l... base/android/library_loader/library_prefetcher.h:6: #define BASE_ANDROID_LIBRARY_PREFETCHER_H_ should be: BASE_ANDROID_LIBRARY_LOADER_LIBRARY_PREFETCHER_H_
Hello, Thank you for the review. I made changes to: - Avoid using C library functions to parse the mappings (using base:: functions instead) - Make the private functions more private - Return error codes in the parsing - Use the current prefetching mechanism as a fallback. After an offline discussion with pasko, it seems safer for now. We should probably add a histogram to figure out whether things are going well in the field, and eventually remove the fallback. This will be addressed in a forthcoming CL. https://codereview.chromium.org/1001343002/diff/130001/base/android/library_l... File base/android/library_loader/library_prefetcher.cc (right): https://codereview.chromium.org/1001343002/diff/130001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:30: char* line = fgets(line_buffer_, kMaxLineLength - 1, file_); On 2015/03/24 17:10:36, pasko wrote: > why so low level and complex with FILE* and fgets etc? You can use > ReadFileToString() and StringTokenizer. Then it is possible to use StringPiece. > > Amount of allocations would probably be the same in practice. I would prefer to avoid an unbounded memory allocation in the parent process, if possible. https://codereview.chromium.org/1001343002/diff/130001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:44: std::string* filename) { On 2015/03/24 15:18:26, rmcilroy wrote: > nit - this might be neater wrapping up the arguments as a struct. Done. https://codereview.chromium.org/1001343002/diff/130001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:57: DCHECK((pieces_count == 5) || (pieces_count == 6)); On 2015/03/24 15:18:26, rmcilroy wrote: > Either do this as a CHECK or add an if to only to the code below if this is true > (preferred I think), otherwise the code below could do an buffer-overflow on > release builds if the format changes in the future. Done. https://codereview.chromium.org/1001343002/diff/130001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:62: "long long is not int64_t"); // This way we can use strtoll(). On 2015/03/24 17:10:36, pasko wrote: > StringToInt64? Done. https://codereview.chromium.org/1001343002/diff/130001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:64: *end = strtoull(delimiter_ptr + 1, &dummy, 16); On 2015/03/24 15:18:26, rmcilroy wrote: > nit - I would prefer you just used base::SplitString here to split the start and > end with the dash. Done. https://codereview.chromium.org/1001343002/diff/130001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:86: const char* kSuffixesToMatch[] = {"libchrome.so", "base.apk"}; On 2015/03/24 17:10:36, pasko wrote: > why base.apk? is the chrome apk always renamed to base.apk? Why not just .apk? According to the android source code (see PackageInstallerSession#validateInstallLocked), yes, unless we are using split apks. https://codereview.chromium.org/1001343002/diff/130001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:89: return flags && flags[0] == 'r' && flags[3] == 'p'; On 2015/03/24 15:18:26, rmcilroy wrote: > nit - add a check here that flags is long enough to avoid buffer overflows. Done. https://codereview.chromium.org/1001343002/diff/130001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:113: for (unsigned char* addr = start_address; addr < end_address - sizeof(int); On 2015/03/24 17:10:36, pasko wrote: > s/ - sizeof(int)// ? > > you can CHECK(!(start_address & 0xFFF) && !(end_address & 0xFFF)) to make it > even more straightforward. Done. https://codereview.chromium.org/1001343002/diff/130001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:136: ranges.push_back(std::make_pair(start, end)); On 2015/03/24 17:10:36, pasko wrote: > We could potentially have both libchrome.so and .apk mapped, in this case we > need to prefetch only libchrome.so (like on those Samsung devices where we > cannot mmap from apk, it does not mean smb else could not mmap the apk in our > process before us, right?) > > Having multiple ranges scares me, we are not deterministic at walking through > the ranges, i.e. injecting a source of variance that'd be impossible to find. > Can we just on encountering more than one range just record a histogram and walk > away? Well, we have several ranges in pretty much all cases right now: - .text can be split, with a hole in the middle (related to shared relro IIRC) - .data is separate from .text (not the same permissions, obviously) So having several ranges is pretty much unavoidable. However code mappings are private, which is why we are checking this above. https://codereview.chromium.org/1001343002/diff/130001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:145: // returns an incorrect address, only the forked process will crash. On 2015/03/24 15:18:26, rmcilroy wrote: > The parsing is done on the main process though. Acknowledged. https://codereview.chromium.org/1001343002/diff/130001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:149: // to evict, minimizing disruption caused by this prefetching. On 2015/03/24 15:18:26, rmcilroy wrote: > Does forking actually helps this - the pages should be clean even if they were > Prefetched by the main process (unless the main process modifies them but in > that case they wouldn't be clean and unreferenced even if they are prefetched by > the forked process). You are right, removed this comment. Done. https://codereview.chromium.org/1001343002/diff/130001/base/android/library_l... File base/android/library_loader/library_prefetcher.h (right): https://codereview.chromium.org/1001343002/diff/130001/base/android/library_l... base/android/library_loader/library_prefetcher.h:6: #define BASE_ANDROID_LIBRARY_PREFETCHER_H_ On 2015/03/24 17:10:36, pasko wrote: > should be: BASE_ANDROID_LIBRARY_LOADER_LIBRARY_PREFETCHER_H_ Done. https://codereview.chromium.org/1001343002/diff/130001/base/android/library_l... base/android/library_loader/library_prefetcher.h:28: // empty lines. On 2015/03/24 15:18:26, rmcilroy wrote: > This seems slightly dangerous. Maybe just make NextLine return a bool for EOF > signaling and fill in a std::string* argument. Done. https://codereview.chromium.org/1001343002/diff/130001/base/android/library_l... base/android/library_loader/library_prefetcher.h:68: void ForkAndPrefetchNativeLibrary(); On 2015/03/24 15:18:26, rmcilroy wrote: > Could we wrap ForkAndPrefetchNativeLibrary as a static function in a class, then > have ProcMapsIterator, FileLineIterator and FindRanges as private > classes/functions and make ProcMapsIteratorTest a friend to minimize the API > exposed for testing. Yes, thank you for the suggestion. Only protected instead of private, because testing. Done. https://codereview.chromium.org/1001343002/diff/130001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/init/NativeInitializationController.java (left): https://codereview.chromium.org/1001343002/diff/130001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/init/NativeInitializationController.java:94: libraryLoader.asyncPrefetchLibrariesToMemory(mContext); On 2015/03/24 15:18:26, rmcilroy wrote: > Can we delete asyncPrefetchLibrariesToMemory from the code now? Can we keep it as a fallback for now, in case something goes wrong with the fork()-based solution?
just one comment, looking further https://codereview.chromium.org/1001343002/diff/130001/base/android/library_l... File base/android/library_loader/library_prefetcher.cc (right): https://codereview.chromium.org/1001343002/diff/130001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:136: ranges.push_back(std::make_pair(start, end)); On 2015/03/25 10:36:18, lizeb wrote: > On 2015/03/24 17:10:36, pasko wrote: > > We could potentially have both libchrome.so and .apk mapped, in this case we > > need to prefetch only libchrome.so (like on those Samsung devices where we > > cannot mmap from apk, it does not mean smb else could not mmap the apk in our > > process before us, right?) > > > > Having multiple ranges scares me, we are not deterministic at walking through > > the ranges, i.e. injecting a source of variance that'd be impossible to find. > > Can we just on encountering more than one range just record a histogram and > walk > > away? > > Well, we have several ranges in pretty much all cases right now: > - .text can be split, with a hole in the middle (related to shared relro IIRC) > - .data is separate from .text (not the same permissions, obviously) > > So having several ranges is pretty much unavoidable. However code mappings are > private, which is why we are checking this above. Got it. As per offline discussion, on later systems regions both libchrome.so and base.apk can be mapped (both private and shared), and we may introduce extra IO if prefetching the wrong one (it would not fail, just a slowdown and a source of non-determinism). So to deal with that let's: if there exists at least one range from libchrome.so: prefetch only libchrome.so ranges else: prefetch base.apk ranges
https://codereview.chromium.org/1001343002/diff/210001/base/android/library_l... File base/android/library_loader/library_prefetcher.cc (right): https://codereview.chromium.org/1001343002/diff/210001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:62: if ((pieces_count != 5) && (pieces_count != 6)) { a filename might contain a space, right? https://codereview.chromium.org/1001343002/diff/210001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:176: if (WIFSIGNALED(status)) { why checking this? we would have returned false anyway https://codereview.chromium.org/1001343002/diff/210001/base/android/library_l... File base/android/library_loader/library_prefetcher.h (right): https://codereview.chromium.org/1001343002/diff/210001/base/android/library_l... base/android/library_loader/library_prefetcher.h:29: class FileLineIterator { To my taste this iterator adds unnecessary clutter converting ~5 lines of implementation code into ~35 lines. Can you please put file opening and reading logic directly into ProcMapsIterator::FindRanges?
https://codereview.chromium.org/1001343002/diff/130001/base/android/library_l... File base/android/library_loader/library_prefetcher.cc (right): https://codereview.chromium.org/1001343002/diff/130001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:136: ranges.push_back(std::make_pair(start, end)); On 2015/03/25 10:36:18, lizeb wrote: > On 2015/03/24 17:10:36, pasko wrote: > > We could potentially have both libchrome.so and .apk mapped, in this case we > > need to prefetch only libchrome.so (like on those Samsung devices where we > > cannot mmap from apk, it does not mean smb else could not mmap the apk in our > > process before us, right?) > > > > Having multiple ranges scares me, we are not deterministic at walking through > > the ranges, i.e. injecting a source of variance that'd be impossible to find. > > Can we just on encountering more than one range just record a histogram and > walk > > away? > > Well, we have several ranges in pretty much all cases right now: > - .text can be split, with a hole in the middle (related to shared relro IIRC) It's due to packed relocations, but yes we will need to support it. > - .data is separate from .text (not the same permissions, obviously) > > So having several ranges is pretty much unavoidable. However code mappings are > private, which is why we are checking this above. https://codereview.chromium.org/1001343002/diff/130001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/init/NativeInitializationController.java (left): https://codereview.chromium.org/1001343002/diff/130001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/init/NativeInitializationController.java:94: libraryLoader.asyncPrefetchLibrariesToMemory(mContext); On 2015/03/25 10:36:19, lizeb wrote: > On 2015/03/24 15:18:26, rmcilroy wrote: > > Can we delete asyncPrefetchLibrariesToMemory from the code now? > > Can we keep it as a fallback for now, in case something goes wrong with the > fork()-based solution? Are you meaning fallback as in keep the code around incase we need it again or actually fallback at runtime to the previous approach if the fork fails? If the former - this is what revision history is for, if the latter, I prefer not to have runtime fallbacks, it is easy to accidentally end up using a fallback which you didn't think you were using.
Hello, - Simplified the code (removing the iterators) - Added logging when the child process crashes/is killed A forthcoming CL will add a histogram to check whether the fallback is used in practice. If not, I will remove the fallback. https://codereview.chromium.org/1001343002/diff/210001/base/android/library_l... File base/android/library_loader/library_prefetcher.cc (right): https://codereview.chromium.org/1001343002/diff/210001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:62: if ((pieces_count != 5) && (pieces_count != 6)) { On 2015/03/25 12:57:58, pasko wrote: > a filename might contain a space, right? Done. https://codereview.chromium.org/1001343002/diff/210001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:176: if (WIFSIGNALED(status)) { On 2015/03/25 12:57:58, pasko wrote: > why checking this? we would have returned false anyway To log whether the process was killed or crashed. Done. https://codereview.chromium.org/1001343002/diff/210001/base/android/library_l... File base/android/library_loader/library_prefetcher.h (right): https://codereview.chromium.org/1001343002/diff/210001/base/android/library_l... base/android/library_loader/library_prefetcher.h:29: class FileLineIterator { On 2015/03/25 12:57:58, pasko wrote: > To my taste this iterator adds unnecessary clutter converting ~5 lines of > implementation code into ~35 lines. Can you please put file opening and reading > logic directly into ProcMapsIterator::FindRanges? Done.
some more high level thoughts https://codereview.chromium.org/1001343002/diff/230001/base/android/java/src/... File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java (right): https://codereview.chromium.org/1001343002/diff/230001/base/android/java/src/... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:259: // Note: AsyncTasks are executed in a low priority background s/Note: // https://codereview.chromium.org/1001343002/diff/230001/base/android/java/src/... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:260: // thread, which is the desired behavior here since we don't Please avoid using 'we' in comments, writing in third person is always preferable. https://codereview.chromium.org/1001343002/diff/230001/base/android/library_l... File base/android/library_loader/library_prefetcher.cc (right): https://codereview.chromium.org/1001343002/diff/230001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:145: // - Isolating the main process from mistakes in the parsing. If the parsing Forking does not fully 'isolate' because the process was granted all the same capabilities. A buffer overrun may harm the main process by writing something to one of the FDs being open. We are hoping that doing a ROP with /proc/pid/maps is nontrivial, but it's certainly a new attack vector. https://codereview.chromium.org/1001343002/diff/230001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:158: pid_t pid = fork(); what if the child process deadlocks? It might happen to want to grab a lock in malloc(), and that might be acquired by the parent process just before the fork syscall. yey complexity, shall we spawn a timer to kill the child process upon timeout? https://codereview.chromium.org/1001343002/diff/230001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:162: exit(0); this would invoke destructors which we don't do normally for non-browser processes, I can expect all kinds of disasters caused by this up to database backends competing to write data to disk. Should just die here. https://codereview.chromium.org/1001343002/diff/230001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:181: << pid << ")"; break; https://codereview.chromium.org/1001343002/diff/230001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:185: LOG(WARNING) << "The native library prefetching process was killed " is there a reason to distinguish these cases? If you plan to add a histogram to cover this, please add a TODO here
looked through the rest of files, neat stuff https://codereview.chromium.org/1001343002/diff/230001/base/android/library_l... File base/android/library_loader/library_prefetcher.cc (right): https://codereview.chromium.org/1001343002/diff/230001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:23: // Android defines the background priority to this value since at least 2009 If THREAD_PRIORITY_BACKGROUND can change, let's just pass it from Java. Probably nothing would break in this century, but less things to think about when reading. https://codereview.chromium.org/1001343002/diff/230001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:30: const std::string kSuffixesToMatch[] = {"libchrome.so", "base.apk"}; <quote> ... we only allow static variables to contain POD data. This rule completely disallows vector (use C arrays instead), or string (use const char []). </quote> https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Static_and_G... https://codereview.chromium.org/1001343002/diff/230001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:64: bool NativeLibraryPrefetcher::ParseMapping(const std::string& line, // static (same for the 2 other functions) https://codereview.chromium.org/1001343002/diff/230001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:108: base::ScopedFILE file(base::OpenFile(base::FilePath("/proc/self/maps"), "r")); OpenFile is the implementation detail of base::File. Any reason not to use base::File? there are tons of uses of it across the codebase. https://codereview.chromium.org/1001343002/diff/230001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:125: const std::string& libchrome_suffix = kSuffixesToMatch[0]; nit: it's probably nice to separate this part with one empty line and putting a comment like: // Add ranges. If chrome DSO ranges are found avoid taking other ranges. https://codereview.chromium.org/1001343002/diff/230001/base/android/library_l... File base/android/library_loader/library_prefetcher.h (right): https://codereview.chromium.org/1001343002/diff/230001/base/android/library_l... base/android/library_loader/library_prefetcher.h:10: #include <stdio.h> please don't include what you don't use https://codereview.chromium.org/1001343002/diff/230001/base/android/library_l... base/android/library_loader/library_prefetcher.h:28: protected: // Protected and not private for testing. fyi: there is FRIEND_TEST_ALL_PREFIXES https://codereview.chromium.org/1001343002/diff/230001/base/android/library_l... base/android/library_loader/library_prefetcher.h:33: uint64_t offset; please remove offset, device, inode. They are easy to add, and having them adds small overhead: 1. parsing adds a tiny bit of deadcode 2. have to think 'why do we have this in the datastructure'? (one more wtf per day) https://codereview.chromium.org/1001343002/diff/230001/base/android/library_l... base/android/library_loader/library_prefetcher.h:41: // Returns true if we should keep the mapping. 'we should keep' is hard to understand, how about calling the function IsGoodToPrefetch? Maybe more specific: IsExecutableCode()? https://codereview.chromium.org/1001343002/diff/230001/base/android/library_l... File base/android/library_loader/library_prefetcher_unittest.cc (right): https://codereview.chromium.org/1001343002/diff/230001/base/android/library_l... base/android/library_loader/library_prefetcher_unittest.cc:14: FRIEND_TEST_ALL_PREFIXES(NativeLibraryPrefetcherTest, TestParseLine32Bits); FRIEND_TEST_ALL_PREFIXES is typically used in the implementation itself, it allows to keep the members private. I would prefer that at least for consistency. https://codereview.chromium.org/1001343002/diff/230001/base/android/library_l... base/android/library_loader/library_prefetcher_unittest.cc:34: EXPECT_EQ(mapping.start, 0xb6e62000ULL); it seems UINT64_C(0xb6e62000) would work and it is the (old) new fashionable way to do it https://codereview.chromium.org/1001343002/diff/230001/base/android/library_l... base/android/library_loader/library_prefetcher_unittest.cc:108: const std::string line("42-43 rw-p 00000000 b3:15 1204 base.apk"); should this also end with '\n'?
Thank you for the thorough review. https://codereview.chromium.org/1001343002/diff/230001/base/android/java/src/... File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java (right): https://codereview.chromium.org/1001343002/diff/230001/base/android/java/src/... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:259: // Note: AsyncTasks are executed in a low priority background On 2015/03/27 11:03:37, pasko wrote: > s/Note: // Done. https://codereview.chromium.org/1001343002/diff/230001/base/android/java/src/... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:260: // thread, which is the desired behavior here since we don't On 2015/03/27 11:03:37, pasko wrote: > Please avoid using 'we' in comments, writing in third person is always > preferable. Done. https://codereview.chromium.org/1001343002/diff/230001/base/android/library_l... File base/android/library_loader/library_prefetcher.cc (right): https://codereview.chromium.org/1001343002/diff/230001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:23: // Android defines the background priority to this value since at least 2009 On 2015/03/27 13:29:28, pasko wrote: > If THREAD_PRIORITY_BACKGROUND can change, let's just pass it from Java. Probably > nothing would break in this century, but less things to think about when > reading. Well, this adds clutter, and as you said, is highly likely not to change. And it is a lower priority than the default one anyway. Unless Android decides to have foreground applications with a lower priority than the default one, we are fine. https://codereview.chromium.org/1001343002/diff/230001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:30: const std::string kSuffixesToMatch[] = {"libchrome.so", "base.apk"}; On 2015/03/27 13:29:28, pasko wrote: > <quote> > ... we only allow static variables to contain POD data. This rule completely > disallows vector (use C arrays instead), or string (use const char []). > </quote> > > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Static_and_G... Done. https://codereview.chromium.org/1001343002/diff/230001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:64: bool NativeLibraryPrefetcher::ParseMapping(const std::string& line, On 2015/03/27 13:29:28, pasko wrote: > // static > > (same for the 2 other functions) Done. https://codereview.chromium.org/1001343002/diff/230001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:108: base::ScopedFILE file(base::OpenFile(base::FilePath("/proc/self/maps"), "r")); On 2015/03/27 13:29:27, pasko wrote: > OpenFile is the implementation detail of base::File. Any reason not to use > base::File? there are tons of uses of it across the codebase. We need a FILE*, and not a File. And File::PlatformFile is an int on Unix. https://codereview.chromium.org/1001343002/diff/230001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:125: const std::string& libchrome_suffix = kSuffixesToMatch[0]; On 2015/03/27 13:29:27, pasko wrote: > nit: it's probably nice to separate this part with one empty line and putting a > comment like: > // Add ranges. If chrome DSO ranges are found avoid taking other ranges. Done. https://codereview.chromium.org/1001343002/diff/230001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:158: pid_t pid = fork(); On 2015/03/27 11:03:37, pasko wrote: > what if the child process deadlocks? It might happen to want to grab a lock in > malloc(), and that might be acquired by the parent process just before the fork > syscall. > This process doesn't do any heap allocation, and doesn't call any syscall aside from _exit(). This is why the parsing is done in the parent process. > yey complexity, shall we spawn a timer to kill the child process upon timeout? In the worst case, this will leave an AsyncTask behind, sitting idle. I would like to avoid complexity if possible. https://codereview.chromium.org/1001343002/diff/230001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:162: exit(0); On 2015/03/27 11:03:37, pasko wrote: > this would invoke destructors which we don't do normally for non-browser > processes, I can expect all kinds of disasters caused by this up to database > backends competing to write data to disk. > > Should just die here. _exit(2) doesn't call the atexit() handlers. Done. https://codereview.chromium.org/1001343002/diff/230001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:181: << pid << ")"; On 2015/03/27 11:03:37, pasko wrote: > break; Done. https://codereview.chromium.org/1001343002/diff/230001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:185: LOG(WARNING) << "The native library prefetching process was killed " On 2015/03/27 11:03:37, pasko wrote: > is there a reason to distinguish these cases? If you plan to add a histogram to > cover this, please add a TODO here Done. https://codereview.chromium.org/1001343002/diff/230001/base/android/library_l... File base/android/library_loader/library_prefetcher.h (right): https://codereview.chromium.org/1001343002/diff/230001/base/android/library_l... base/android/library_loader/library_prefetcher.h:10: #include <stdio.h> On 2015/03/27 13:29:28, pasko wrote: > please don't include what you don't use > Done. https://codereview.chromium.org/1001343002/diff/230001/base/android/library_l... base/android/library_loader/library_prefetcher.h:28: protected: // Protected and not private for testing. On 2015/03/27 13:29:28, pasko wrote: > fyi: there is FRIEND_TEST_ALL_PREFIXES Thank you for educating me on the standard practice for the friendly test declaration, didn't know that. Done. https://codereview.chromium.org/1001343002/diff/230001/base/android/library_l... base/android/library_loader/library_prefetcher.h:33: uint64_t offset; On 2015/03/27 13:29:28, pasko wrote: > please remove offset, device, inode. > > They are easy to add, and having them adds small overhead: > 1. parsing adds a tiny bit of deadcode > 2. have to think 'why do we have this in the datastructure'? (one more wtf per > day) Done. https://codereview.chromium.org/1001343002/diff/230001/base/android/library_l... File base/android/library_loader/library_prefetcher_unittest.cc (right): https://codereview.chromium.org/1001343002/diff/230001/base/android/library_l... base/android/library_loader/library_prefetcher_unittest.cc:14: FRIEND_TEST_ALL_PREFIXES(NativeLibraryPrefetcherTest, TestParseLine32Bits); On 2015/03/27 13:29:28, pasko wrote: > FRIEND_TEST_ALL_PREFIXES is typically used in the implementation itself, it > allows to keep the members private. I would prefer that at least for > consistency. Done. https://codereview.chromium.org/1001343002/diff/230001/base/android/library_l... base/android/library_loader/library_prefetcher_unittest.cc:34: EXPECT_EQ(mapping.start, 0xb6e62000ULL); On 2015/03/27 13:29:28, pasko wrote: > it seems UINT64_C(0xb6e62000) would work and it is the (old) new fashionable way > to do it Done. https://codereview.chromium.org/1001343002/diff/230001/base/android/library_l... base/android/library_loader/library_prefetcher_unittest.cc:108: const std::string line("42-43 rw-p 00000000 b3:15 1204 base.apk"); On 2015/03/27 13:29:28, pasko wrote: > should this also end with '\n'? Done.
On 2015/03/27 13:29:28, pasko wrote: > https://codereview.chromium.org/1001343002/diff/230001/base/android/library_l... > base/android/library_loader/library_prefetcher.h:41: // Returns true if we > should keep the mapping. > 'we should keep' is hard to understand, how about calling the function > IsGoodToPrefetch? > > Maybe more specific: IsExecutableCode()? did you see this comment of mine? seems like another instance of rietveld flakiness. I.e. it appears in mail and responses, but not in side-by-side diff view. Annoying.
almost there https://codereview.chromium.org/1001343002/diff/230001/base/android/library_l... File base/android/library_loader/library_prefetcher.cc (right): https://codereview.chromium.org/1001343002/diff/230001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:45: // For each range, reads a byte per page to force it into the page cache. can you add a comment saying that heap allocations are not allowed in this function? https://codereview.chromium.org/1001343002/diff/230001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:108: base::ScopedFILE file(base::OpenFile(base::FilePath("/proc/self/maps"), "r")); On 2015/03/27 15:44:52, lizeb wrote: > On 2015/03/27 13:29:27, pasko wrote: > > OpenFile is the implementation detail of base::File. Any reason not to use > > base::File? there are tons of uses of it across the codebase. > > We need a FILE*, and not a File. And File::PlatformFile is an int on Unix. ah, you are right, fgets, and there is no line by line reading in base::File https://codereview.chromium.org/1001343002/diff/230001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:153: // memory allocations in the forked process, that can be problematic. s/ that can be problematic// does not add any useful information. You could explain, of course that memory allocator lock can be acquired in the child process. https://codereview.chromium.org/1001343002/diff/230001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:181: << pid << ")"; On 2015/03/27 15:44:52, lizeb wrote: > On 2015/03/27 11:03:37, pasko wrote: > > break; > > Done. I like how this got done :) https://codereview.chromium.org/1001343002/diff/230001/base/android/library_l... File base/android/library_loader/library_prefetcher_unittest.cc (right): https://codereview.chromium.org/1001343002/diff/230001/base/android/library_l... base/android/library_loader/library_prefetcher_unittest.cc:108: const std::string line("42-43 rw-p 00000000 b3:15 1204 base.apk"); On 2015/03/27 15:44:52, lizeb wrote: > On 2015/03/27 13:29:28, pasko wrote: > > should this also end with '\n'? > > Done. really done? https://codereview.chromium.org/1001343002/diff/250001/base/android/library_l... File base/android/library_loader/library_prefetcher.cc (right): https://codereview.chromium.org/1001343002/diff/250001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:95: // you're looking for. still not third person :) (one can leave it as is, Egor would not be picky this time)
All done, thank you. https://codereview.chromium.org/1001343002/diff/250001/base/android/library_l... File base/android/library_loader/library_prefetcher.cc (right): https://codereview.chromium.org/1001343002/diff/250001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:95: // you're looking for. On 2015/03/27 17:51:54, pasko wrote: > still not third person :) > > (one can leave it as is, Egor would not be picky this time) Acknowledged.
sorry to ask for a little more testing on the late stage of the review, hopefully it is straightforward. Also, a few stylistic issues I am not sure how I missed before. https://codereview.chromium.org/1001343002/diff/290001/base/android/library_l... File base/android/library_loader/library_prefetcher.cc (right): https://codereview.chromium.org/1001343002/diff/290001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:53: unsigned char* start_address = (unsigned char*)range.first; C-style casts are discouraged, please use the least aggressive suitable C++ cast ;) https://codereview.chromium.org/1001343002/diff/290001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:89: // Discards fields 3, 4, and 5 starting from 1 (ie offset, device and inode). We use this form 'Discards' and 'Truncates' when we describe a function or a class or a file. In the implementation itself it is more common to say 'Discard', 'Truncate'. Please do so. https://codereview.chromium.org/1001343002/diff/290001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:106: IsReadableAndPrivate(mapping.flags); // Code mappings are private. nit: s/Code/Code and data/ ? https://codereview.chromium.org/1001343002/diff/290001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:130: // If libchrome.so is mapped, keep only libchrome's ranges. please move it to another function and test separately TakeOnlyLibchromeRangesIfPossible() or something .. https://codereview.chromium.org/1001343002/diff/290001/base/android/library_l... File base/android/library_loader/library_prefetcher.h (right): https://codereview.chromium.org/1001343002/diff/290001/base/android/library_l... base/android/library_loader/library_prefetcher.h:47: FRIEND_TEST_ALL_PREFIXES( please to 'git cl format' for this change, it will reduce some formatting churn later for the new code. Luckily library_loader_hooks.cc is already properly formatted.
All done, thank you. https://codereview.chromium.org/1001343002/diff/290001/base/android/library_l... File base/android/library_loader/library_prefetcher.cc (right): https://codereview.chromium.org/1001343002/diff/290001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:53: unsigned char* start_address = (unsigned char*)range.first; On 2015/03/31 12:01:43, pasko wrote: > C-style casts are discouraged, please use the least aggressive suitable C++ cast > ;) Done. https://codereview.chromium.org/1001343002/diff/290001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:89: // Discards fields 3, 4, and 5 starting from 1 (ie offset, device and inode). On 2015/03/31 12:01:43, pasko wrote: > We use this form 'Discards' and 'Truncates' when we describe a function or a > class or a file. In the implementation itself it is more common to say > 'Discard', 'Truncate'. Please do so. Done. https://codereview.chromium.org/1001343002/diff/290001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:106: IsReadableAndPrivate(mapping.flags); // Code mappings are private. On 2015/03/31 12:01:43, pasko wrote: > nit: > > s/Code/Code and data/ > > ? Done. https://codereview.chromium.org/1001343002/diff/290001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:130: // If libchrome.so is mapped, keep only libchrome's ranges. On 2015/03/31 12:01:43, pasko wrote: > please move it to another function and test separately > > TakeOnlyLibchromeRangesIfPossible() or something .. Done. https://codereview.chromium.org/1001343002/diff/290001/base/android/library_l... File base/android/library_loader/library_prefetcher.h (right): https://codereview.chromium.org/1001343002/diff/290001/base/android/library_l... base/android/library_loader/library_prefetcher.h:47: FRIEND_TEST_ALL_PREFIXES( On 2015/03/31 12:01:43, pasko wrote: > please to 'git cl format' for this change, it will reduce some formatting churn > later for the new code. Luckily library_loader_hooks.cc is already properly > formatted. Done.
rsesek@chromium.org changed reviewers: + rsesek@chromium.org
Drive by: I was wondering if you had seen/considered using this code to parse proc maps? https://code.google.com/p/chromium/codesearch#chromium/src/base/debug/proc_ma... It's obviously not async-signal-safe, but you already parse the maps before the fork(). The comment in the header about the safety of reading the proc map is also relevant, I think, since this code will be executing in the context of multiple threads that Chrome does not control.
On 2015/03/31 15:42:49, Robert Sesek wrote: > Drive by: I was wondering if you had seen/considered using this code to parse > proc maps? > https://code.google.com/p/chromium/codesearch#chromium/src/base/debug/proc_ma... > > It's obviously not async-signal-safe, but you already parse the maps before the > fork(). The comment in the header about the safety of reading the proc map is > also relevant, I think, since this code will be executing in the context of > multiple threads that Chrome does not control. Robert, thanks for the great pointer! I did not know ParseProcMaps() exists. Occasional duplicate or missed maps entries should be fine for prefetch since the actual prefetch will be done in the forked process. We are also going to record a histogram to say how often the child process crashes in the field. I think it makes sense to reuse ParseProcMaps() in general, but here we wanted to avoid the unnecessary unbounded heap allocation for /proc/pid/maps. The allocation was easy to avoid, but if you insist that it is premature, I'll be OK with reusing ParseProcMaps() and the obvious bonus from making this CL simpler.
Hello, Thank you for the suggestion. As Egor said, I didn't reuse this code to avoid an unbounded allocation, especially since it takes place in the parent process. But I am on the fence as to whether it is better to reuse code or to avoid an unbounded allocation here. As for missing or duplicated entries, as Egor said, this is not an issue here. What do you think? On Wed, Apr 1, 2015 at 2:37 PM, <pasko@chromium.org> wrote: > On 2015/03/31 15:42:49, Robert Sesek wrote: > >> Drive by: I was wondering if you had seen/considered using this code to >> parse >> proc maps? >> > > https://code.google.com/p/chromium/codesearch#chromium/ > src/base/debug/proc_maps_linux.h > > It's obviously not async-signal-safe, but you already parse the maps >> before >> > the > >> fork(). The comment in the header about the safety of reading the proc >> map is >> also relevant, I think, since this code will be executing in the context >> of >> multiple threads that Chrome does not control. >> > > Robert, thanks for the great pointer! I did not know ParseProcMaps() > exists. > > Occasional duplicate or missed maps entries should be fine for prefetch > since > the actual prefetch will be done in the forked process. We are also going > to > record a histogram to say how often the child process crashes in the field. > > I think it makes sense to reuse ParseProcMaps() in general, but here we > wanted > to avoid the unnecessary unbounded heap allocation for /proc/pid/maps. The > allocation was easy to avoid, but if you insist that it is premature, I'll > be OK > with reusing ParseProcMaps() and the obvious bonus from making this CL > simpler. > > https://codereview.chromium.org/1001343002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
lgtm (i.e. looks good to me in the current sate; as usual, sticky only with trivial changes)
Ross, can you please take another look?
On 2015/04/01 12:37:16, pasko wrote: > On 2015/03/31 15:42:49, Robert Sesek wrote: > > Drive by: I was wondering if you had seen/considered using this code to parse > > proc maps? > > > https://code.google.com/p/chromium/codesearch#chromium/src/base/debug/proc_ma... > > > > It's obviously not async-signal-safe, but you already parse the maps before > the > > fork(). The comment in the header about the safety of reading the proc map is > > also relevant, I think, since this code will be executing in the context of > > multiple threads that Chrome does not control. > > Robert, thanks for the great pointer! I did not know ParseProcMaps() exists. > > Occasional duplicate or missed maps entries should be fine for prefetch since > the actual prefetch will be done in the forked process. We are also going to > record a histogram to say how often the child process crashes in the field. > > I think it makes sense to reuse ParseProcMaps() in general, but here we wanted > to avoid the unnecessary unbounded heap allocation for /proc/pid/maps. The > allocation was easy to avoid, but if you insist that it is premature, I'll be OK > with reusing ParseProcMaps() and the obvious bonus from making this CL simpler. Looking at the history of ParseProcMaps(), it looks like it was added initially for Android. I definitely appreciate the concern over unbounded allocation, but why is that a concern now but not before? I think you could potentially still use ParseProcMaps() without using ReadProcMaps(), and then handle the read internally to manage the size of the std::string buffer. I think it'd be good to avoid duplicating proc parsing code if at all possible.
On 2015/04/01 15:11:09, Robert Sesek wrote: > On 2015/04/01 12:37:16, pasko wrote: > > On 2015/03/31 15:42:49, Robert Sesek wrote: > > > Drive by: I was wondering if you had seen/considered using this code to > parse > > > proc maps? > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/base/debug/proc_ma... > > > > > > It's obviously not async-signal-safe, but you already parse the maps before > > the > > > fork(). The comment in the header about the safety of reading the proc map > is > > > also relevant, I think, since this code will be executing in the context of > > > multiple threads that Chrome does not control. > > > > Robert, thanks for the great pointer! I did not know ParseProcMaps() exists. > > > > Occasional duplicate or missed maps entries should be fine for prefetch since > > the actual prefetch will be done in the forked process. We are also going to > > record a histogram to say how often the child process crashes in the field. > > > > I think it makes sense to reuse ParseProcMaps() in general, but here we wanted > > to avoid the unnecessary unbounded heap allocation for /proc/pid/maps. The > > allocation was easy to avoid, but if you insist that it is premature, I'll be > OK > > with reusing ParseProcMaps() and the obvious bonus from making this CL > simpler. > > Looking at the history of ParseProcMaps(), it looks like it was added initially > for Android. I definitely appreciate the concern over unbounded allocation, but > why is that a concern now but not before? > > I think you could potentially still use ParseProcMaps() without using > ReadProcMaps(), and then handle the read internally to manage the size of the > std::string buffer. I think it'd be good to avoid duplicating proc parsing code > if at all possible. You are right, thank you for the input! I will remove the code duplication and then re-send the CL.
> Ross, can you please take another look? Let me know when you've uploaded the CL which switches to using ParseProcMaps and I'll take another look.
lgtm with a couple of small changes https://codereview.chromium.org/1001343002/diff/330001/base/android/library_l... File base/android/library_loader/library_prefetcher.h (right): https://codereview.chromium.org/1001343002/diff/330001/base/android/library_l... base/android/library_loader/library_prefetcher.h:11: #include <sys/types.h> unnecessary? https://codereview.chromium.org/1001343002/diff/330001/base/android/library_l... base/android/library_loader/library_prefetcher.h:28: static bool IsGoodToPrefetch(const base::debug::MappedMemoryRegion& region); this and following methods should be private
On 2015/04/02 10:03:53, rmcilroy wrote: > > Ross, can you please take another look? > > Let me know when you've uploaded the CL which switches to using ParseProcMaps > and I'll take another look. the change has switched to ParseProcMaps, PTAL
https://codereview.chromium.org/1001343002/diff/330001/base/android/library_l... File base/android/library_loader/library_prefetcher.cc (right): https://codereview.chromium.org/1001343002/diff/330001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:36: for (int i = 0; i < kSuffixesToMatchCount; i++) { You can remove this in favor of arraysize(kSuffixesToMatch). https://codereview.chromium.org/1001343002/diff/330001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:48: bool Prefetch(const std::vector<std::pair<uintptr_t, uintptr_t>>& ranges) { Consider making this pair a named type (in the .h private: section): using PointerRange = std::pair<uintptr_t, uintptr_t>; https://codereview.chromium.org/1001343002/diff/330001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:105: if (!ParseProcMaps(proc_maps, ®ions)) base::debug:: ?
All done, thank you! https://codereview.chromium.org/1001343002/diff/330001/base/android/library_l... File base/android/library_loader/library_prefetcher.cc (right): https://codereview.chromium.org/1001343002/diff/330001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:36: for (int i = 0; i < kSuffixesToMatchCount; i++) { On 2015/04/02 15:58:02, Robert Sesek wrote: > You can remove this in favor of arraysize(kSuffixesToMatch). Thank you for the suggestion! Done. https://codereview.chromium.org/1001343002/diff/330001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:48: bool Prefetch(const std::vector<std::pair<uintptr_t, uintptr_t>>& ranges) { On 2015/04/02 15:58:02, Robert Sesek wrote: > Consider making this pair a named type (in the .h private: section): > > using PointerRange = std::pair<uintptr_t, uintptr_t>; Thank you for the suggestion. I've updated the other locations to use AddressRange, but not this one. This is a function included in an anonymous namespace in this file, so I didn't want to make it a friend of NativeLibraryPrefetcher. But I am totally open to change this, wdyt? https://codereview.chromium.org/1001343002/diff/330001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:105: if (!ParseProcMaps(proc_maps, ®ions)) On 2015/04/02 15:58:02, Robert Sesek wrote: > base::debug:: ? Done. https://codereview.chromium.org/1001343002/diff/330001/base/android/library_l... File base/android/library_loader/library_prefetcher.h (right): https://codereview.chromium.org/1001343002/diff/330001/base/android/library_l... base/android/library_loader/library_prefetcher.h:11: #include <sys/types.h> On 2015/04/02 15:50:36, pasko wrote: > unnecessary? Done. https://codereview.chromium.org/1001343002/diff/330001/base/android/library_l... base/android/library_loader/library_prefetcher.h:28: static bool IsGoodToPrefetch(const base::debug::MappedMemoryRegion& region); On 2015/04/02 15:50:36, pasko wrote: > this and following methods should be private Done.
Nice, just two small things. https://codereview.chromium.org/1001343002/diff/350001/base/android/library_l... File base/android/library_loader/library_prefetcher.cc (right): https://codereview.chromium.org/1001343002/diff/350001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:118: // Forks and waits for a process prefetching the native library. This is done in I think this would be better as the class-level comment in the header. https://codereview.chromium.org/1001343002/diff/350001/base/android/library_l... File base/android/library_loader/library_prefetcher.h (right): https://codereview.chromium.org/1001343002/diff/350001/base/android/library_l... base/android/library_loader/library_prefetcher.h:52: }; DISALLOW_IMPLICIT_CONSTRUCTORS() to prevent instantiation.
Done, thank you! https://codereview.chromium.org/1001343002/diff/350001/base/android/library_l... File base/android/library_loader/library_prefetcher.cc (right): https://codereview.chromium.org/1001343002/diff/350001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:118: // Forks and waits for a process prefetching the native library. This is done in On 2015/04/02 19:46:48, Robert Sesek wrote: > I think this would be better as the class-level comment in the header. Done. https://codereview.chromium.org/1001343002/diff/350001/base/android/library_l... File base/android/library_loader/library_prefetcher.h (right): https://codereview.chromium.org/1001343002/diff/350001/base/android/library_l... base/android/library_loader/library_prefetcher.h:52: }; On 2015/04/02 19:46:48, Robert Sesek wrote: > DISALLOW_IMPLICIT_CONSTRUCTORS() to prevent instantiation. Done.
LGTM
Sorry for the delay (vacation and traveling now). Code looks reasonable, however I'm a bit concerned about the forking and the use of fallbacks. Details in comments. https://codereview.chromium.org/1001343002/diff/370001/base/android/java/src/... File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java (right): https://codereview.chromium.org/1001343002/diff/370001/base/android/java/src/... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:257: if (!ok) { Do you expect the fork to ever fail for any valid reason other than bugs? I would really prefer not to do a fallback - it just hides issues when the actual implementation fails and makes things more difficult to spot problems and debug them. Even if the fork fails it isn't the end of the world, startup will just be slightly slower. Let's just remove this and add some UMA metrics to track whether the fork actually fails in the wild. https://codereview.chromium.org/1001343002/diff/370001/base/android/library_l... File base/android/library_loader/library_prefetcher.cc (right): https://codereview.chromium.org/1001343002/diff/370001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:27: // We may load directly from the APK. This comment is not very clear, could you make it more clear that you are mentioning this due to the 'base.apk' being a suffix as well as libchrome.so https://codereview.chromium.org/1001343002/diff/370001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:78: void NativeLibraryPrefetcher::TakeOnlyLibchromeRangesIfPossible( nit - /s/Libchrome/LibChrome https://codereview.chromium.org/1001343002/diff/370001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:81: const std::string& libchrome_suffix = kSuffixesToMatch[0]; It would be better to have "libchrome.so" as a separate constant so that this code doesn't have to depend on the order of the kSuffixesToMatch array. https://codereview.chromium.org/1001343002/diff/370001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:114: TakeOnlyLibchromeRangesIfPossible(regions_to_prefetch, ranges); nit - FilterLibChromeRangesOnlyIfPossible https://codereview.chromium.org/1001343002/diff/370001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:124: // able to call malloc(). As you state here, forking a process with threads is full of gotchas and could potentially cause deadlocks. Are you really sure it is worth forking here? I'm not sure the reasons listed in library_prefetcher.h are very strong - I would actually prefer that the main process crashed if there were errors in the parsing code, at least that way we would know about it and could fix it (also the crazy linker already does a bunch of /proc/self/maps processing unconditionally and hasn't had problems doing so so far). I'm also not convinced that inflating memory usage is an issue since these pages will be clean and so the kernel would be more likely to simply unmap them rather than killing the Chrome process under memory pressure. If you are sure you need to do this in a separate process, did you consider doing a fork and execvp instead (perhaps transferring the /proc/<pid>/maps file as a file descriptor to the child process and doing the parsing on the child process)? This would be a bit more work but would be much safer IMO.
Hello, I have updated the CL to address your comments. I still think that forking is the better solution here, see the details in comments. https://codereview.chromium.org/1001343002/diff/370001/base/android/java/src/... File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java (right): https://codereview.chromium.org/1001343002/diff/370001/base/android/java/src/... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:257: if (!ok) { On 2015/04/07 12:17:27, rmcilroy wrote: > Do you expect the fork to ever fail for any valid reason other than bugs? I > would really prefer not to do a fallback - it just hides issues when the actual > implementation fails and makes things more difficult to spot problems and debug > them. Even if the fork fails it isn't the end of the world, startup will just be > slightly slower. Let's just remove this and add some UMA metrics to track > whether the fork actually fails in the wild. fork() can fail for a variety of reasons (weird SELinux policy for instance). I do agree that the fallback should not stay, but I suggest to first put a UMA metric, and then to remove the current prefetching code. Would that work for you? https://codereview.chromium.org/1001343002/diff/370001/base/android/library_l... File base/android/library_loader/library_prefetcher.cc (right): https://codereview.chromium.org/1001343002/diff/370001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:27: // We may load directly from the APK. On 2015/04/07 12:17:27, rmcilroy wrote: > This comment is not very clear, could you make it more clear that you are > mentioning this due to the 'base.apk' being a suffix as well as libchrome.so Done. https://codereview.chromium.org/1001343002/diff/370001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:78: void NativeLibraryPrefetcher::TakeOnlyLibchromeRangesIfPossible( On 2015/04/07 12:17:27, rmcilroy wrote: > nit - /s/Libchrome/LibChrome Acknowledged. https://codereview.chromium.org/1001343002/diff/370001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:81: const std::string& libchrome_suffix = kSuffixesToMatch[0]; On 2015/04/07 12:17:27, rmcilroy wrote: > It would be better to have "libchrome.so" as a separate constant so that this > code doesn't have to depend on the order of the kSuffixesToMatch array. Done. https://codereview.chromium.org/1001343002/diff/370001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:114: TakeOnlyLibchromeRangesIfPossible(regions_to_prefetch, ranges); On 2015/04/07 12:17:27, rmcilroy wrote: > nit - FilterLibChromeRangesOnlyIfPossible Done. https://codereview.chromium.org/1001343002/diff/370001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:124: // able to call malloc(). On 2015/04/07 12:17:27, rmcilroy wrote: > As you state here, forking a process with threads is full of gotchas and could > potentially cause deadlocks. Are you really sure it is worth forking here? Forking a new process was brought up as a nice solution after discussions with simonb@ and primiano@, partially traced in this (abandonned) CL comments : https://codereview.chromium.org/958473003/ I still believe that this is important. Also in other discussions, inflating chrome's PSS was considered as an important point, as well as having the page not be referenced, instead of clean but referenced. > I'm > not sure the reasons listed in library_prefetcher.h are very strong - I would > actually prefer that the main process crashed if there were errors in the > parsing code, at least that way we would know about it and could fix it (also > the crazy linker already does a bunch of /proc/self/maps processing > unconditionally and hasn't had problems doing so so far). I'm also not convinced > that inflating memory usage is an issue since these pages will be clean and so > the kernel would be more likely to simply unmap them rather than killing the > Chrome process under memory pressure. > > If you are sure you need to do this in a separate process, did you consider > doing a fork and execvp instead (perhaps transferring the /proc/<pid>/maps file > as a file descriptor to the child process and doing the parsing on the child > process)? This would be a bit more work but would be much safer IMO. Well, this would also require to a separate "one-trick pony" binary. I think that it is more trouble than necessary.
Sorry for the delay. Generally looks fine now, but let's remove the fallback. https://codereview.chromium.org/1001343002/diff/370001/base/android/java/src/... File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java (right): https://codereview.chromium.org/1001343002/diff/370001/base/android/java/src/... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:257: if (!ok) { On 2015/04/16 13:28:47, lizeb wrote: > On 2015/04/07 12:17:27, rmcilroy wrote: > > Do you expect the fork to ever fail for any valid reason other than bugs? I > > would really prefer not to do a fallback - it just hides issues when the > actual > > implementation fails and makes things more difficult to spot problems and > debug > > them. Even if the fork fails it isn't the end of the world, startup will just > be > > slightly slower. Let's just remove this and add some UMA metrics to track > > whether the fork actually fails in the wild. > > fork() can fail for a variety of reasons (weird SELinux policy for instance). I > do agree that the fallback should not stay, but I suggest to first put a UMA > metric, and then to remove the current prefetching code. > Would that work for you? I would prefer you removed the fallback now and added UMA either in this CL or in a followup soon after. As I say, the fallback would hide a multitude of sins and since we aren't breaking anything if the prefetching fails I would really prefer not to have any fallback code in here. https://codereview.chromium.org/1001343002/diff/370001/base/android/library_l... File base/android/library_loader/library_prefetcher.cc (right): https://codereview.chromium.org/1001343002/diff/370001/base/android/library_l... base/android/library_loader/library_prefetcher.cc:124: // able to call malloc(). On 2015/04/16 13:28:47, lizeb wrote: > On 2015/04/07 12:17:27, rmcilroy wrote: > > As you state here, forking a process with threads is full of gotchas and could > > potentially cause deadlocks. Are you really sure it is worth forking here? > > Forking a new process was brought up as a nice solution after discussions with > simonb@ and primiano@, partially traced in this (abandonned) CL comments : > https://codereview.chromium.org/958473003/ > > I still believe that this is important. Also in other discussions, inflating > chrome's PSS was considered as an important point, as well as having the page > not be referenced, instead of clean but referenced. Makes sense. Thanks for the explination. > > I'm > > not sure the reasons listed in library_prefetcher.h are very strong - I would > > actually prefer that the main process crashed if there were errors in the > > parsing code, at least that way we would know about it and could fix it (also > > the crazy linker already does a bunch of /proc/self/maps processing > > unconditionally and hasn't had problems doing so so far). I'm also not > convinced > > that inflating memory usage is an issue since these pages will be clean and so > > the kernel would be more likely to simply unmap them rather than killing the > > Chrome process under memory pressure. > > > > If you are sure you need to do this in a separate process, did you consider > > doing a fork and execvp instead (perhaps transferring the /proc/<pid>/maps > file > > as a file descriptor to the child process and doing the parsing on the child > > process)? This would be a bit more work but would be much safer IMO. > > Well, this would also require to a separate "one-trick pony" binary. I think > that it is more trouble than necessary. Yeah you are probably right. I was kindof thinking about the Linux world where we could launch the chrome executable with a different process-type argument, but this is a bit more tricky in the Android world. OK, I guess this is fine with the warnings.
Done, thank you. The histogram will be in a forthcoming CL. https://codereview.chromium.org/1001343002/diff/370001/base/android/java/src/... File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java (right): https://codereview.chromium.org/1001343002/diff/370001/base/android/java/src/... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:257: if (!ok) { On 2015/04/23 15:09:42, rmcilroy wrote: > On 2015/04/16 13:28:47, lizeb wrote: > > On 2015/04/07 12:17:27, rmcilroy wrote: > > > Do you expect the fork to ever fail for any valid reason other than bugs? I > > > would really prefer not to do a fallback - it just hides issues when the > > actual > > > implementation fails and makes things more difficult to spot problems and > > debug > > > them. Even if the fork fails it isn't the end of the world, startup will > just > > be > > > slightly slower. Let's just remove this and add some UMA metrics to track > > > whether the fork actually fails in the wild. > > > > fork() can fail for a variety of reasons (weird SELinux policy for instance). > I > > do agree that the fallback should not stay, but I suggest to first put a UMA > > metric, and then to remove the current prefetching code. > > Would that work for you? > > I would prefer you removed the fallback now and added UMA either in this CL or > in a followup soon after. As I say, the fallback would hide a multitude of sins > and since we aren't breaking anything if the prefetching fails I would really > prefer not to have any fallback code in here. Done.
still lgtm
Lgtm, thanks!
lizeb@chromium.org changed reviewers: + jam@chromium.org
Hello jam.
Please review (for OWNERS approval) the base/*.{gyp,gypi,gn} changes in this CL.
Thank you!
On 2015/04/27 16:37:44, lizeb wrote:
> Hello jam.
>
> Please review (for OWNERS approval) the base/*.{gyp,gypi,gn} changes in this
CL.
> Thank you!
lgtm
The CQ bit was checked by lizeb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org, pasko@chromium.org, rmcilroy@chromium.org Link to the patchset: https://codereview.chromium.org/1001343002/#ps470001 (title: ".")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1001343002/470001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
The CQ bit was checked by lizeb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pasko@chromium.org, rsesek@chromium.org, jam@chromium.org, rmcilroy@chromium.org Link to the patchset: https://codereview.chromium.org/1001343002/#ps490001 (title: "Missing BASE_EXPORT.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1001343002/490001
Message was sent while issue was closed.
Committed patchset #26 (id:490001)
Message was sent while issue was closed.
Patchset 26 (id:??) landed as https://crrev.com/0a830d2548468ecbfb324644b7ecf9ddb46d4b42 Cr-Commit-Position: refs/heads/master@{#327473} |
