Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10826067:
Implement a tool called link_limiter to impose a global restriction on how many (Closed)
|
Created:
8 years, 4 months ago by iannucci Modified:
8 years, 4 months ago CC:
chromium-reviews, pam+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImplement a tool called link_limiter to impose a global restriction on how many
occurances of link.exe and lib.exe can run concurrently.
Python builder emits link_limiter.exe and lib_limiter.exe.
Skeleton copied from supalink.
Seems to behave as expected. Tested with max-concurrency=1 on my
local machine.
Run python script with the argument 'clean' to... well... clean up.
R=cmp,nsylvain
BUG=
NOTRY=true
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=149911
Patch Set 1 #Patch Set 2 : refactor concurrency metrics #
Total comments: 1
Patch Set 3 : Build in a subdirectory. #Patch Set 4 : Refactor to take shimmed exe and pipe name from argv[0] (sneaky, hunh?) #
Total comments: 20
Patch Set 5 : Implement feedback from cl #
Total comments: 32
Patch Set 6 : Fix some formatting issues #Patch Set 7 : Fix quotes :) #Patch Set 8 : Fix an issue with printing errors and cmdline rendering #
Total comments: 15
Patch Set 9 : Incorporate feedback from thestig #
Total comments: 8
Patch Set 10 : Fix a few more bits #Patch Set 11 : Fix _wtoi, make fallback wait 5 sec, event wait 60 sec #
Messages
Total messages: 30 (0 generated)
Please keep an eye on implementation robustness. Some issues on my mind: * CPU count is a bad heuristic. I think we'll need to hand-set on the bots after profiling, anyway (via env variable). * Right now it builds link_limiter.exe and lib_limiter.exe, which share the same named pipe/event. Running it on my machine seemed to be OK... We may want to play with limiting lib independently (each exe must be specified separately on the msbuild commandline anyhow). It's not essential to have this run on developer machines (though it might help). I'm thinking it would probably be good anyway to have a way to opt-in to using it. I'm assuming for the bots it will just be additional logic in compile.py, but since devs don't use this (I think?), I'm not sure how to nicely let people opt in. R
should this be in gyp instead? or is this really specific to chrome? (i.e. will you specific the path to those files in the chrome gyp files or do you need to teach gyp itself to use them?) https://chromiumcodereview.appspot.com/10826067/diff/2001/.gitignore File .gitignore (right): https://chromiumcodereview.appspot.com/10826067/diff/2001/.gitignore#newcode231 .gitignore:231: /tools/win/link_limiter/ why? Maybe you want to ignore only {link/lib}_limiter.txt ? Otherwise it will make it hard to make changes to the .py and the .cc
On 2012/07/30 22:16:25, nsylvain wrote: > should this be in gyp instead? or is this really specific to chrome? (i.e. will > you specific the path to those files in the chrome gyp files or do you need to > teach gyp itself to use them?) > > https://chromiumcodereview.appspot.com/10826067/diff/2001/.gitignore > File .gitignore (right): > > https://chromiumcodereview.appspot.com/10826067/diff/2001/.gitignore#newcode231 > .gitignore:231: /tools/win/link_limiter/ > why? Maybe you want to ignore only {link/lib}_limiter.txt ? Otherwise it will > make it hard to make changes to the .py and the .cc The tool needs to have the property that it's executable by msbuild. I don't think gyp could do this (since it's not the one actually running link/lib). If we move to ninja, I will be inclined to integrate this behavior (i.e. limited concurrency for a particular rule) into ninja itself in a cross-platform way, at which point these shims are not needed. I added the py and cc, so they're tracked by git now. The other files that get generated (i.e. exe/obj/ilk/pdb) are being ignored. I suppose I should fix this to have a build directory which is ignored instead of the main one.
On 2012/07/30 22:22:15, iannucci1 wrote: > On 2012/07/30 22:16:25, nsylvain wrote: > > should this be in gyp instead? or is this really specific to chrome? (i.e. > will > > you specific the path to those files in the chrome gyp files or do you need to > > teach gyp itself to use them?) > > > > https://chromiumcodereview.appspot.com/10826067/diff/2001/.gitignore > > File .gitignore (right): > > > > > https://chromiumcodereview.appspot.com/10826067/diff/2001/.gitignore#newcode231 > > .gitignore:231: /tools/win/link_limiter/ > > why? Maybe you want to ignore only {link/lib}_limiter.txt ? Otherwise it will > > make it hard to make changes to the .py and the .cc > > The tool needs to have the property that it's executable by msbuild. I don't > think gyp could do this (since it's not the one actually running link/lib). If > we move to ninja, I will be inclined to integrate this behavior (i.e. limited > concurrency for a particular rule) into ninja itself in a cross-platform way, at > which point these shims are not needed. > > I added the py and cc, so they're tracked by git now. The other files that get > generated (i.e. exe/obj/ilk/pdb) are being ignored. I suppose I should fix this > to have a build directory which is ignored instead of the main one. yeah, that'd be better. I thought "git status" would not run at all in those ignored directories, even if you have tracked files in them.
On 2012/07/30 22:23:36, nsylvain wrote: > On 2012/07/30 22:22:15, iannucci1 wrote: > > On 2012/07/30 22:16:25, nsylvain wrote: > > > should this be in gyp instead? or is this really specific to chrome? (i.e. > > will > > > you specific the path to those files in the chrome gyp files or do you need > to > > > teach gyp itself to use them?) > > > > > > https://chromiumcodereview.appspot.com/10826067/diff/2001/.gitignore > > > File .gitignore (right): > > > > > > > > > https://chromiumcodereview.appspot.com/10826067/diff/2001/.gitignore#newcode231 > > > .gitignore:231: /tools/win/link_limiter/ > > > why? Maybe you want to ignore only {link/lib}_limiter.txt ? Otherwise it > will > > > make it hard to make changes to the .py and the .cc > > > > The tool needs to have the property that it's executable by msbuild. I don't > > think gyp could do this (since it's not the one actually running link/lib). If > > we move to ninja, I will be inclined to integrate this behavior (i.e. limited > > concurrency for a particular rule) into ninja itself in a cross-platform way, > at > > which point these shims are not needed. > > > > I added the py and cc, so they're tracked by git now. The other files that get > > generated (i.e. exe/obj/ilk/pdb) are being ignored. I suppose I should fix > this > > to have a build directory which is ignored instead of the main one. > > yeah, that'd be better. I thought "git status" would not run at all in those > ignored directories, even if you have tracked files in them. Ok, I fixed the directories/ignore issue. Was there any other feedback? I also need to talk to one of you about actually integrating this/getting a trybot on msbuild+goma/etc.
Ok. Please take another look when you have a minute. I got rid of some additional hackiness, fixed some bugs, etc. I'd like to get this on trunk so I can start messing with it on the FYI bot. R
I'm forgetting most of the c++ style guide, so there might be parts of the code that is not really style guide friendly. Also this code uses things that we usually don't use in chrome, so I'm a little afraid that it will cause some presubmit tests to fail or something like that.. but we'll see. https://chromiumcodereview.appspot.com/10826067/diff/8001/tools/win/link_limi... File tools/win/link_limiter/build_link_limiter.py (right): https://chromiumcodereview.appspot.com/10826067/diff/8001/tools/win/link_limi... tools/win/link_limiter/build_link_limiter.py:47: if not os.path.exists(outpath) or cpptime > os.path.getmtime(outpath): do you really need the optimization for the time? This is a manual process right? And this is a small file. We should probably just build it all the time? https://chromiumcodereview.appspot.com/10826067/diff/8001/tools/win/link_limi... tools/win/link_limiter/build_link_limiter.py:64: if os.path.exists(BUILD_DIR): Slightly afraid someone would do : cd src\ && python tools\win\link_limiter\build_link_limiter.py and get their src\build directory destroyed. Can we make it an absolute path instead? https://chromiumcodereview.appspot.com/10826067/diff/8001/tools/win/link_limi... tools/win/link_limiter/build_link_limiter.py:80: os.chdir(os.path.dirname(os.path.abspath(__file__))) or maybe you can just run this before checking for the BUILD_DIR https://chromiumcodereview.appspot.com/10826067/diff/8001/tools/win/link_limi... tools/win/link_limiter/build_link_limiter.py:89: for shim_exe in "lib.exe link.exe".split(): why the .split? why not for shim_exe in('lib.exe', 'link.exe'): Also, we should use single quotes everywhere. https://chromiumcodereview.appspot.com/10826067/diff/8001/tools/win/link_limi... tools/win/link_limiter/build_link_limiter.py:90: newpath = "%s__LIMITER.exe" % shim_exe single quotes https://chromiumcodereview.appspot.com/10826067/diff/8001/tools/win/link_limi... File tools/win/link_limiter/limiter.cpp (right): https://chromiumcodereview.appspot.com/10826067/diff/8001/tools/win/link_limi... tools/win/link_limiter/limiter.cpp:24: DWORD rc = FormatMessageA(FORMAT_MESSAGE_ALLOCATE_BUFFER | FormatMessageA is sad a little bit. We can't use the default? https://chromiumcodereview.appspot.com/10826067/diff/8001/tools/win/link_limi... tools/win/link_limiter/limiter.cpp:26: NULL, spacing is off by one https://chromiumcodereview.appspot.com/10826067/diff/8001/tools/win/link_limi... tools/win/link_limiter/limiter.cpp:64: std::string error = ErrorMessageToString(GetLastError()); Little overkill maybe. I'm sure people can just look up the error number. https://chromiumcodereview.appspot.com/10826067/diff/8001/tools/win/link_limi... tools/win/link_limiter/limiter.cpp:100: << " to (num_cores-1): " << ErrorMessageToString(last_error) << "\n"; same here I guess.
https://chromiumcodereview.appspot.com/10826067/diff/8001/tools/win/link_limi... File tools/win/link_limiter/build_link_limiter.py (right): https://chromiumcodereview.appspot.com/10826067/diff/8001/tools/win/link_limi... tools/win/link_limiter/build_link_limiter.py:47: if not os.path.exists(outpath) or cpptime > os.path.getmtime(outpath): On 2012/08/01 18:53:46, nsylvain wrote: > do you really need the optimization for the time? This is a manual process > right? And this is a small file. We should probably just build it all the time? This was a holdover from the supalink code. I can remove it. I figured that since it was there I'd keep it :) https://chromiumcodereview.appspot.com/10826067/diff/8001/tools/win/link_limi... tools/win/link_limiter/build_link_limiter.py:80: os.chdir(os.path.dirname(os.path.abspath(__file__))) On 2012/08/01 18:53:46, nsylvain wrote: > or maybe you can just run this before checking for the BUILD_DIR yeah good call. I'll make the script normalize itself relative to __file__ earlier https://chromiumcodereview.appspot.com/10826067/diff/8001/tools/win/link_limi... tools/win/link_limiter/build_link_limiter.py:89: for shim_exe in "lib.exe link.exe".split(): On 2012/08/01 18:53:46, nsylvain wrote: > why the .split? why not for shim_exe in('lib.exe', 'link.exe'): > > Also, we should use single quotes everywhere. I tend to lose split b/c less punctuation. I'll tuple-ify it. https://chromiumcodereview.appspot.com/10826067/diff/8001/tools/win/link_limi... tools/win/link_limiter/build_link_limiter.py:90: newpath = "%s__LIMITER.exe" % shim_exe On 2012/08/01 18:53:46, nsylvain wrote: > single quotes Why single quotes (curious)? Python makes no distinction between the two (except w.r.t. escaping the quotes of the same gender), and I don't see anything in the style guide. Bash would definitely need single quotes if possible :D https://chromiumcodereview.appspot.com/10826067/diff/8001/tools/win/link_limi... File tools/win/link_limiter/limiter.cpp (right): https://chromiumcodereview.appspot.com/10826067/diff/8001/tools/win/link_limi... tools/win/link_limiter/limiter.cpp:24: DWORD rc = FormatMessageA(FORMAT_MESSAGE_ALLOCATE_BUFFER | On 2012/08/01 18:53:46, nsylvain wrote: > FormatMessageA is sad a little bit. We can't use the default? Because I use it for formatting exception messages (and because the std:: exception classes only take narrow strings (and because I didn't want to make a custom exception class)), I opted to use the ansi variant here. Now that I think about it, if someone runs this on a non-ansi-friendly box, this would potentially do the wrong thing. I'll suck it up and make a custom wide exception so I can do unicode everywhere :) https://chromiumcodereview.appspot.com/10826067/diff/8001/tools/win/link_limi... tools/win/link_limiter/limiter.cpp:26: NULL, On 2012/08/01 18:53:46, nsylvain wrote: > spacing is off by one So it is... ugly :( https://chromiumcodereview.appspot.com/10826067/diff/8001/tools/win/link_limi... tools/win/link_limiter/limiter.cpp:64: std::string error = ErrorMessageToString(GetLastError()); On 2012/08/01 18:53:46, nsylvain wrote: > Little overkill maybe. I'm sure people can just look up the error number. Yeah probably. Again, this was a holdover from supalink, and it's one less "google msdn error ######" search I have to do :P
https://chromiumcodereview.appspot.com/10826067/diff/8001/tools/win/link_limi... File tools/win/link_limiter/build_link_limiter.py (right): https://chromiumcodereview.appspot.com/10826067/diff/8001/tools/win/link_limi... tools/win/link_limiter/build_link_limiter.py:89: for shim_exe in "lib.exe link.exe".split(): On 2012/08/01 19:09:53, iannucci wrote: > On 2012/08/01 18:53:46, nsylvain wrote: > > why the .split? why not for shim_exe in('lib.exe', 'link.exe'): > > > > Also, we should use single quotes everywhere. > > I tend to lose split b/c less punctuation. I'll tuple-ify it. Er... 'use' split.
looks like there are outstanding review comments from nsylvain in the .py and .cpp files i believe limiter.cpp should be renamed to limiter.cc. nicolas, did you see that already? https://chromiumcodereview.appspot.com/10826067/diff/8001/tools/win/link_limi... File tools/win/link_limiter/build_link_limiter.py (right): https://chromiumcodereview.appspot.com/10826067/diff/8001/tools/win/link_limi... tools/win/link_limiter/build_link_limiter.py:13: BUILD_DIR = "build" nit: single quotes https://chromiumcodereview.appspot.com/10826067/diff/8001/tools/win/link_limi... tools/win/link_limiter/build_link_limiter.py:18: with os.fdopen(fd, "w") as f: nit: single quotes https://chromiumcodereview.appspot.com/10826067/diff/8001/tools/win/link_limi... tools/win/link_limiter/build_link_limiter.py:90: newpath = "%s__LIMITER.exe" % shim_exe On 2012/08/01 19:09:53, iannucci wrote: > On 2012/08/01 18:53:46, nsylvain wrote: > > single quotes > > Why single quotes (curious)? Python makes no distinction between the two (except > w.r.t. escaping the quotes of the same gender), and I don't see anything in the > style guide. Nothing in the style guide about single versus double quotes, but the style guide does say 'be consistent'. So within the same file, consistency means picking one and sticking with it unless the specific need is there for something else. Zoom out and you'll find we support a bunch of Python code and work on a variety of areas, and we still want to be consistent across all of those. So when it comes to choosing which of the two to pick for the whole file, we pick single quotes in order to stay consistent across all of the Python files we maintain in tools/build and src/. I would not be surprised if there were files in other parts of src/ that aren't consistent with this approach (or even aren't consistent within the same file). We fix those if we run into those cases and have to, assuming there's no objection. > Bash would definitely need single quotes if possible :D Don't get us started on bash. :)
Ok, I implemented the changes discussed. I also noticed that exceptions are strongly forbidden by the style guide, so I ripped out the exception handling stuff and implemented it in good old 70's style return code handling. It's a good thing we're building clean, modern software using the cutting edge techniques from 40 years ago :D
we should get someone with c++ readability to review, too maybe thestig could take a look? https://chromiumcodereview.appspot.com/10826067/diff/13001/tools/win/link_lim... File tools/win/link_limiter/build_link_limiter.py (right): https://chromiumcodereview.appspot.com/10826067/diff/13001/tools/win/link_lim... tools/win/link_limiter/build_link_limiter.py:77: print "Couldn't get VCINSTALLDIR. Run vsvars32.bat?" nit: single quotes https://chromiumcodereview.appspot.com/10826067/diff/13001/tools/win/link_lim... File tools/win/link_limiter/limiter.cc (right): https://chromiumcodereview.appspot.com/10826067/diff/13001/tools/win/link_lim... tools/win/link_limiter/limiter.cc:15: #include <vector> alphanum sort lines 11-15 https://chromiumcodereview.appspot.com/10826067/diff/13001/tools/win/link_lim... tools/win/link_limiter/limiter.cc:20: const int g_wait_time = 30 * 1000; // 30 seconds probably should remove one of the extra spaces after 'int' https://chromiumcodereview.appspot.com/10826067/diff/13001/tools/win/link_lim... tools/win/link_limiter/limiter.cc:31: static void Warn(const tstring& msg, ...) { insert an empty line before line 31 https://chromiumcodereview.appspot.com/10826067/diff/13001/tools/win/link_lim... tools/win/link_limiter/limiter.cc:32: if(!g_is_debug) one space after if https://chromiumcodereview.appspot.com/10826067/diff/13001/tools/win/link_lim... tools/win/link_limiter/limiter.cc:107: bufsize > buffer.size()) this indent should line the first b in bufsize under the ! in the line above (aka "4 space indent when spanning lines in a conditional") https://chromiumcodereview.appspot.com/10826067/diff/13001/tools/win/link_lim... tools/win/link_limiter/limiter.cc:111: } while(!ok && last_error == ERROR_INSUFFICIENT_BUFFER); insert a space after while https://chromiumcodereview.appspot.com/10826067/diff/13001/tools/win/link_lim... tools/win/link_limiter/limiter.cc:121: reinterpret_cast<PSYSTEM_LOGICAL_PROCESSOR_INFORMATION>(&buffer[0]); increase indent on line 121 by 2 spaces https://chromiumcodereview.appspot.com/10826067/diff/13001/tools/win/link_lim... tools/win/link_limiter/limiter.cc:123: sizeof(SYSTEM_LOGICAL_PROCESSOR_INFORMATION); increase indent on line 123 by 2 spaces https://chromiumcodereview.appspot.com/10826067/diff/13001/tools/win/link_lim... tools/win/link_limiter/limiter.cc:125: for(int i = 0; i < num_entries; ++i) { one space after for https://chromiumcodereview.appspot.com/10826067/diff/13001/tools/win/link_lim... tools/win/link_limiter/limiter.cc:127: if(info.Relationship == RelationProcessorCore) { one space after if https://chromiumcodereview.appspot.com/10826067/diff/13001/tools/win/link_lim... tools/win/link_limiter/limiter.cc:152: if (max_concurrent == -1) { could max_concurrent ever not equal -1? i'm thinking this if condition could go away and the code in it could be de-indented 2 spaces https://chromiumcodereview.appspot.com/10826067/diff/13001/tools/win/link_lim... tools/win/link_limiter/limiter.cc:159: switch(metric) { one space after switch https://chromiumcodereview.appspot.com/10826067/diff/13001/tools/win/link_lim... tools/win/link_limiter/limiter.cc:160: case CONCURRENCY_METRIC_CPU: each case in a switch should be indented by 2 spaces https://chromiumcodereview.appspot.com/10826067/diff/13001/tools/win/link_lim... tools/win/link_limiter/limiter.cc:162: if(max_concurrent) one space after if https://chromiumcodereview.appspot.com/10826067/diff/13001/tools/win/link_lim... tools/win/link_limiter/limiter.cc:178: let's be consistent about 2 empty lines between functions or 1 empty line. i couldn't find anything in the style guide about this and example files i found in base/ only use 1. so i guess stick with 1? https://chromiumcodereview.appspot.com/10826067/diff/13001/tools/win/link_lim... tools/win/link_limiter/limiter.cc:186: HANDLE hPipe = INVALID_HANDLE_VALUE; i believe the 'no hungarian notation' rule applies here https://chromiumcodereview.appspot.com/10826067/diff/13001/tools/win/link_lim... tools/win/link_limiter/limiter.cc:187: for(;;) { one space after for https://chromiumcodereview.appspot.com/10826067/diff/13001/tools/win/link_lim... tools/win/link_limiter/limiter.cc:189: pipename.c_str(), these should be indented by an extra 2 spaces per line https://chromiumcodereview.appspot.com/10826067/diff/13001/tools/win/link_lim... tools/win/link_limiter/limiter.cc:197: ); i think this line should be de-indented by 2 spaces https://chromiumcodereview.appspot.com/10826067/diff/13001/tools/win/link_lim... tools/win/link_limiter/limiter.cc:236: HANDLE hPipe = hungarian notation should not be used (see C++ style guide section on 'windows code') https://chromiumcodereview.appspot.com/10826067/diff/13001/tools/win/link_lim... tools/win/link_limiter/limiter.cc:329: size_t dot = base_pipename.find(L'.'); for consistency, use " here https://chromiumcodereview.appspot.com/10826067/diff/13001/tools/win/link_lim... tools/win/link_limiter/limiter.cc:331: usage(L"Hunh? No '.' in argv[0]? Are we on windows?"); can you make this more clear? (eg. 'ERROR: Required . in argv[0], but none found.') https://chromiumcodereview.appspot.com/10826067/diff/13001/tools/win/link_lim... tools/win/link_limiter/limiter.cc:337: nit: remove empty line
https://chromiumcodereview.appspot.com/10826067/diff/13001/tools/win/link_lim... File tools/win/link_limiter/build_link_limiter.py (right): https://chromiumcodereview.appspot.com/10826067/diff/13001/tools/win/link_lim... tools/win/link_limiter/build_link_limiter.py:77: print "Couldn't get VCINSTALLDIR. Run vsvars32.bat?" On 2012/08/01 23:36:42, cmp wrote: > nit: single quotes even for the contained "'"? https://chromiumcodereview.appspot.com/10826067/diff/13001/tools/win/link_lim... File tools/win/link_limiter/limiter.cc (right): https://chromiumcodereview.appspot.com/10826067/diff/13001/tools/win/link_lim... tools/win/link_limiter/limiter.cc:152: if (max_concurrent == -1) { On 2012/08/01 23:36:42, cmp wrote: > could max_concurrent ever not equal -1? i'm thinking this if condition could go > away and the code in it could be de-indented 2 spaces It's static. Multiple calls will get the cached value. (and since this application is single threaded, static is safe here). This used to be more important in a previous flavors of the code (this function was called multiple times). I could take it or leave it. https://chromiumcodereview.appspot.com/10826067/diff/13001/tools/win/link_lim... tools/win/link_limiter/limiter.cc:329: size_t dot = base_pipename.find(L'.'); On 2012/08/01 23:36:42, cmp wrote: > for consistency, use " here The implementation of find is (marginally) more efficient for chars than for c-strings. Make it a string for real? https://chromiumcodereview.appspot.com/10826067/diff/13001/tools/win/link_lim... tools/win/link_limiter/limiter.cc:331: usage(L"Hunh? No '.' in argv[0]? Are we on windows?"); On 2012/08/01 23:36:42, cmp wrote: > can you make this more clear? (eg. 'ERROR: Required . in argv[0], but none > found.') Well... Since argv[0] must be the executable name, and this program can only run on windows, this is (completely?) impossible, since windows can't execute binaries which don't have an extension (AFAIK). Maybe, "Expected an executable extension in argv[0]. No '.' found." ?
https://chromiumcodereview.appspot.com/10826067/diff/13001/tools/win/link_lim... File tools/win/link_limiter/build_link_limiter.py (right): https://chromiumcodereview.appspot.com/10826067/diff/13001/tools/win/link_lim... tools/win/link_limiter/build_link_limiter.py:77: print "Couldn't get VCINSTALLDIR. Run vsvars32.bat?" On 2012/08/01 23:52:39, iannucci1 wrote: > even for the contained "'"? Thanks for pointing that out, I overlooked it. I'll say a little more to give you more background. I tend to expand contractions to avoid contained single quotes. If that's easy for you to change, please do that. I've also seen strict adherence to single quotes and then contained single quotes being escaped. You're probably wondering where the line is on this, or if there is one. There is, and it's when contained single quotes really get in the way and require a bunch of escaping that makes the code hard to read. A good example of this is when defining regular expressions that match on single quotes.
On 2012/08/02 00:00:04, cmp wrote: > https://chromiumcodereview.appspot.com/10826067/diff/13001/tools/win/link_lim... > File tools/win/link_limiter/build_link_limiter.py (right): > > https://chromiumcodereview.appspot.com/10826067/diff/13001/tools/win/link_lim... > tools/win/link_limiter/build_link_limiter.py:77: print "Couldn't get > VCINSTALLDIR. Run vsvars32.bat?" > On 2012/08/01 23:52:39, iannucci1 wrote: > > even for the contained "'"? > > Thanks for pointing that out, I overlooked it. I'll say a little more to give > you more background. > > I tend to expand contractions to avoid contained single quotes. If that's easy > for you to change, please do that. > > I've also seen strict adherence to single quotes and then contained single > quotes being escaped. > > You're probably wondering where the line is on this, or if there is one. There > is, and it's when contained single quotes really get in the way and require a > bunch of escaping that makes the code hard to read. A good example of this is > when defining regular expressions that match on single quotes. Makes sense. Fixed :)
https://chromiumcodereview.appspot.com/10826067/diff/13001/tools/win/link_lim... File tools/win/link_limiter/limiter.cc (right): https://chromiumcodereview.appspot.com/10826067/diff/13001/tools/win/link_lim... tools/win/link_limiter/limiter.cc:152: if (max_concurrent == -1) { Never mind, then. https://chromiumcodereview.appspot.com/10826067/diff/13001/tools/win/link_lim... tools/win/link_limiter/limiter.cc:329: size_t dot = base_pipename.find(L'.'); On 2012/08/01 23:52:39, iannucci1 wrote: > The implementation of find is (marginally) more efficient for chars than for > c-strings. Make it a string for real? Nah, character here, then. Can you document that before line 329? https://chromiumcodereview.appspot.com/10826067/diff/13001/tools/win/link_lim... tools/win/link_limiter/limiter.cc:331: usage(L"Hunh? No '.' in argv[0]? Are we on windows?"); SGTM
This is mostly a style review. I wasn't paying full attention to the logic. https://chromiumcodereview.appspot.com/10826067/diff/13003/tools/win/link_lim... File tools/win/link_limiter/limiter.cc (right): https://chromiumcodereview.appspot.com/10826067/diff/13003/tools/win/link_lim... tools/win/link_limiter/limiter.cc:19: const bool g_is_debug = (_wgetenv(L"LIMITER_DEBUG") != NULL); I think this will create a static initializer, but since this is a separate tool, meh. https://chromiumcodereview.appspot.com/10826067/diff/13003/tools/win/link_lim... tools/win/link_limiter/limiter.cc:19: const bool g_is_debug = (_wgetenv(L"LIMITER_DEBUG") != NULL); Either these should be static too, or just put all of these guys in an anonymous namespace. namespace { blah blah ... } // namespace https://chromiumcodereview.appspot.com/10826067/diff/13003/tools/win/link_lim... tools/win/link_limiter/limiter.cc:20: const int g_wait_time = 30 * 1000; // 30 seconds |g_wait_time| -> |g_wait_time_in_ms| ? https://chromiumcodereview.appspot.com/10826067/diff/13003/tools/win/link_lim... tools/win/link_limiter/limiter.cc:25: tstring new_msg = tstring(L"limiter fatal error: ")+msg+L"\n"; foo + bar, instead of foo+bar, here and below. https://chromiumcodereview.appspot.com/10826067/diff/13003/tools/win/link_lim... tools/win/link_limiter/limiter.cc:44: DWORD rc = FormatMessage( If you can label the arguments here and in CreateProcess() below, like you did for CreateNamedPipe(), that would be great. https://chromiumcodereview.appspot.com/10826067/diff/13003/tools/win/link_lim... tools/win/link_limiter/limiter.cc:45: FORMAT_MESSAGE_ALLOCATE_BUFFER | FORMAT_MESSAGE_FROM_SYSTEM, 4 space indent, not 2. Same elsewhere below. https://chromiumcodereview.appspot.com/10826067/diff/13003/tools/win/link_lim... tools/win/link_limiter/limiter.cc:85: cmdline.c_str(), ErrorMessageToString(GetLastError()).c_str()); align with L"Error... https://chromiumcodereview.appspot.com/10826067/diff/13003/tools/win/link_lim... tools/win/link_limiter/limiter.cc:109: { brace on previous line https://chromiumcodereview.appspot.com/10826067/diff/13003/tools/win/link_lim... tools/win/link_limiter/limiter.cc:127: SYSTEM_LOGICAL_PROCESSOR_INFORMATION &info = pproc_info[i]; Foo& bar, not Foo &bar https://chromiumcodereview.appspot.com/10826067/diff/13003/tools/win/link_lim... tools/win/link_limiter/limiter.cc:129: max_concurrent += 1; max_concurrent++ ? https://chromiumcodereview.appspot.com/10826067/diff/13003/tools/win/link_lim... tools/win/link_limiter/limiter.cc:148: ConcurrencyMetricEnum metric = CONCURRENCY_METRIC_DEFAULT) no default arguments please https://chromiumcodereview.appspot.com/10826067/diff/13003/tools/win/link_lim... tools/win/link_limiter/limiter.cc:164: // else fallthrough fallthrough -> fall through https://chromiumcodereview.appspot.com/10826067/diff/13003/tools/win/link_lim... tools/win/link_limiter/limiter.cc:217: static int wait_and_run(const tstring& shimmed_exe, Functions should be NamedLikeThis. Be consistent with function names through out the file. https://chromiumcodereview.appspot.com/10826067/diff/13003/tools/win/link_lim... tools/win/link_limiter/limiter.cc:246: if (pipe != INVALID_HANDLE_VALUE) { CloseHandle(pipe); } if (foo) bar; https://chromiumcodereview.appspot.com/10826067/diff/13003/tools/win/link_lim... tools/win/link_limiter/limiter.cc:254: strm << msg << L"\n"; Streams are also generally disallowed. I think you can do: tstring foo = L"blah 1\n" L"blah 2\n" ... Error("%s\n%s", msg.c_str(), foo.c_str());
@thestig PTAL :) R
Doing good, a few more nits. FWIW, there's a google.vim somewhere that you can put in your .vimrc, and it will help you do some of this automatically. https://chromiumcodereview.appspot.com/10826067/diff/15001/tools/win/link_lim... File tools/win/link_limiter/limiter.cc (right): https://chromiumcodereview.appspot.com/10826067/diff/15001/tools/win/link_lim... tools/win/link_limiter/limiter.cc:106: reinterpret_cast<PSYSTEM_LOGICAL_PROCESSOR_INFORMATION>(&buffer[0]), 4 spaces here. https://chromiumcodereview.appspot.com/10826067/diff/15001/tools/win/link_lim... tools/win/link_limiter/limiter.cc:117: L" environment variable '%s' to (num_cores-1): %s", foo - bar instead of foo-bar here and below. https://chromiumcodereview.appspot.com/10826067/diff/15001/tools/win/link_lim... tools/win/link_limiter/limiter.cc:155: max_concurrent = _wtoi(max_concurrent_str ? max_concurrent_str : L"0"); = max_concurrent_str ? _wtoi(max_concurrent_str) : 0 https://chromiumcodereview.appspot.com/10826067/diff/15001/tools/win/link_lim... tools/win/link_limiter/limiter.cc:186: pipename.c_str(), 4 spaces https://chromiumcodereview.appspot.com/10826067/diff/15001/tools/win/link_lim... tools/win/link_limiter/limiter.cc:223: NULL, // Default security attributes put this on the previous line or 4 space indent. https://chromiumcodereview.appspot.com/10826067/diff/15001/tools/win/link_lim... tools/win/link_limiter/limiter.cc:238: (end_time - start_time) / 1000.0); indent
Fixed those (and one I missed on the previous round, e.g. commenting the function calls). Comment below. https://chromiumcodereview.appspot.com/10826067/diff/15001/tools/win/link_lim... File tools/win/link_limiter/limiter.cc (right): https://chromiumcodereview.appspot.com/10826067/diff/15001/tools/win/link_lim... tools/win/link_limiter/limiter.cc:155: max_concurrent = _wtoi(max_concurrent_str ? max_concurrent_str : L"0"); On 2012/08/03 05:03:23, Lei Zhang wrote: > = max_concurrent_str ? _wtoi(max_concurrent_str) : 0 Not sure about this one. I was attempting to prevent running _wtoi on NULL (in case it's not defined in the environment). I think calling it on NULL would create an exception in the debugger (http://msdn.microsoft.com/en-us/library/yd5xkb5c(v=vs.80).aspx)
lgtm style lgtm https://chromiumcodereview.appspot.com/10826067/diff/15001/tools/win/link_lim... File tools/win/link_limiter/limiter.cc (right): https://chromiumcodereview.appspot.com/10826067/diff/15001/tools/win/link_lim... tools/win/link_limiter/limiter.cc:155: max_concurrent = _wtoi(max_concurrent_str ? max_concurrent_str : L"0"); On 2012/08/03 05:50:34, iannucci wrote: > On 2012/08/03 05:03:23, Lei Zhang wrote: > > = max_concurrent_str ? _wtoi(max_concurrent_str) : 0 > > Not sure about this one. I was attempting to prevent running _wtoi on NULL (in > case it's not defined in the environment). I think calling it on NULL would > create an exception in the debugger > (http://msdn.microsoft.com/en-us/library/yd5xkb5c%28v=vs.80%29.aspx) Yep, I know, but my suggestion also avoids calling _wtoi() with NULL. The difference is it doesn't bother passing "0" into _wtoi(). We already know what it should return in that case.
LGTM on the logic as well, thanks (and thanks Lei for the style review) A few concerns to look for while doing our initial deployment - 30 seconds of sleep time seems a little high. I wonder if we should not use something smaller, like 5 seconds. - Using the number of CPUs should probably not be our only factor. RAM accounts for quite a bit too. On one of those new shiny 32core system, I doubt we'll have enough ram (or even disk IO) to run 31 links in parallel. We should probably limit it to max(cpu-1, ram/4) or something like that.
On 2012/08/03 06:09:54, Lei Zhang wrote: > lgtm > > style lgtm > > https://chromiumcodereview.appspot.com/10826067/diff/15001/tools/win/link_lim... > File tools/win/link_limiter/limiter.cc (right): > > https://chromiumcodereview.appspot.com/10826067/diff/15001/tools/win/link_lim... > tools/win/link_limiter/limiter.cc:155: max_concurrent = _wtoi(max_concurrent_str > ? max_concurrent_str : L"0"); > On 2012/08/03 05:50:34, iannucci wrote: > > On 2012/08/03 05:03:23, Lei Zhang wrote: > > > = max_concurrent_str ? _wtoi(max_concurrent_str) : 0 > > > > Not sure about this one. I was attempting to prevent running _wtoi on NULL (in > > case it's not defined in the environment). I think calling it on NULL would > > create an exception in the debugger > > (http://msdn.microsoft.com/en-us/library/yd5xkb5c%2528v=vs.80%2529.aspx) > > Yep, I know, but my suggestion also avoids calling _wtoi() with NULL. The > difference is it doesn't bother passing "0" into _wtoi(). We already know what > it should return in that case. Drrr... I totally misread that. I'll fix it. Thanks for the review :)
On 2012/08/03 15:48:25, nsylvain wrote: > LGTM on the logic as well, thanks (and thanks Lei for the style review) > > A few concerns to look for while doing our initial deployment > - 30 seconds of sleep time seems a little high. I wonder if we should not use > something smaller, like 5 seconds. > - Using the number of CPUs should probably not be our only factor. RAM accounts > for quite a bit too. On one of those new shiny 32core system, I doubt we'll have > enough ram (or even disk IO) to run 31 links in parallel. We should probably > limit it to max(cpu-1, ram/4) or something like that. It will only sleep for 30 seconds if: * An instance of the link.exe__LIMIT.exe executable is holding the last available semaphore * The process segfaults/calls std::terminate before triggering the Event. If that happens, there's probably a (much) deeper issue which should be fixed. The 30 seconds is more of a concession to allow the build to finish rather than hanging forever. I haven't observed this happening yet. In practice, I've observed other contended instances of the limiter immediately pick up (i.e. the signal is working). I think it might be a good idea to completely remove the fallback sleep code though, and have the limiter error out instead. Yeah I think I mentioned this somewhere back in the email discussion. I think we will need to hand-tune the environment variable chrome. Since we're in between the builder and link, we can actually be even smarter and make the threshold proportional to the size of the inputs (# of objects linked, size of objects/libs). If the linker's output already exists, we can make the threshold proportional to the existing output (won't help for clobbers though). Another factor to take into account is the amount of available IOPS, which might be impossible to discover (especially on shared devices like a SAN). I still maintain that having 1 linker / core running on a (reasonable) SSD should be no problem. After hand tuning in a couple scenarios, I'll try to derive a better heuristic, and if I can't, then I'll remove it and set the default concurrency to 2 or something.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/iannucci@chromium.org/10826067/16004
Try job failure for 10826067-16004 (retry) on mac for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...
Does this get built with the regular build? If not, I'd just put NOTRY=true in the commit msg.
On 2012/08/03 19:54:23, Lei Zhang wrote: > Does this get built with the regular build? If not, I'd just put NOTRY=true in > the commit msg. Yeah no it doesn't. I'll do that. Thanks :)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/iannucci@chromium.org/10826067/16004
Change committed as 149911 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
