Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
83803003:
Adds an helper class to shard mspdbsrv. (Closed)
|
Created:
7 years, 1 month ago by Sébastien Marchand Modified:
7 years ago CC:
gyp-developer_googlegroups.com Base URL:
http://gyp.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionAdds an helper class to shard mspdbsrv.
Currently there's a single instance of mspdbsrv shared between all the linkers. This can cause some OOMs if too many linkers are accessing it in parallel.
There's an undocumented environment variable which allows to specify the mspdbsrv endpoint to bound to a linker: _MSPDBSRV_ENDPOINT_ , by setting this variable to a unique value per target you ensure that one different instance of mspdbsrv will be used for every target.
Currently I only use this value for the .exe targets, I keep the shared one for the other ones.
This is hidden behind a 'GYP_USE_SEPARATE_MSPDBSRV' environment variable.
Patch by sebmarchand@chromium.org
R=scottmg@chromium.org, siggi@chromium.org
Committed: https://code.google.com/p/gyp/source/detail?r=1801
Patch Set 1 : #
Total comments: 27
Patch Set 2 : #
Total comments: 26
Patch Set 3 : #
Total comments: 2
Patch Set 4 : #
Total comments: 2
Patch Set 5 : #
Total comments: 2
Patch Set 6 : #Messages
Total messages: 19 (0 generated)
Please take a first look (I still need to unittest this, do some benchmark etc but I'd like a first opinion). Note that English and Python are not my primary languages so feel free to correct me on both :) Thanks !
Nice, thanks for trying this out, this could be very useful. I'm not sure how we can test this well. At least a "probabilistic test" where have a lot of pdbs generated by compile steps and then have them used by different mspdbsrvs. But since we don't know the actual requirements and constraints it might be tricky to come up with a solid test. siggi, any ideas? I'll try to think about it a bit more. https://codereview.chromium.org/83803003/diff/10002/pylib/gyp/win_tool.py File pylib/gyp/win_tool.py (right): https://codereview.chromium.org/83803003/diff/10002/pylib/gyp/win_tool.py#new... pylib/gyp/win_tool.py:37: self._is_started = self._MaybeStartMsPdbSrv(env, args) no _ prefix on member variables initialize the various things you intend to use as members of this class here to None: self.mspdbsrv_start_process = None self.mspdbsrv_stop_process = None self.env = None |is_started| seems redundant with |mspdbsrv_process|, just remove it? https://codereview.chromium.org/83803003/diff/10002/pylib/gyp/win_tool.py#new... pylib/gyp/win_tool.py:39: def __del__(self): __del__ isn't good as it's when it happens to get GCd, or possibly never. Instead use __enter__ and __exit__ and in the usage location, use with MsPdbSrvHelper(...) as mspdbsrv: blah and then __exit__ will be called on the end of that scope. https://codereview.chromium.org/83803003/diff/10002/pylib/gyp/win_tool.py#new... pylib/gyp/win_tool.py:60: endpoint_name = arg[arg.rfind(':') + 1:] + '_mspdbsrv_endpoint' is the ':' the drive separator? i know i suggested this to test, but instead, it'd be better to pass the target name through in the calls to gyp-win-tool link-wrapper and add another argument after $arch that's the endpoint name. (there's only 3 callsites and they're all windows-only) https://codereview.chromium.org/83803003/diff/10002/pylib/gyp/win_tool.py#new... pylib/gyp/win_tool.py:79: '-spawn') when i run "mspdbsrv -start -spawn -endpoint abcd" from the command line it doesn't actually seem to spawn, but rather blocks. does it do differently for you? (i didn't try running it from within python though) https://codereview.chromium.org/83803003/diff/10002/pylib/gyp/win_tool.py#new... pylib/gyp/win_tool.py:84: stderr=subprocess.STDOUT) i don't think there's any stdout/stderr, but just in case, don't redirect them because if there is they'll get swallowed (also, you're not using them anyway. https://codereview.chromium.org/83803003/diff/10002/pylib/gyp/win_tool.py#new... pylib/gyp/win_tool.py:97: _, _ = mspdbsrv_stop_process.communicate() and these can just be .wait() instead of .communicate then once you remove the stdout/stderr redirects. https://codereview.chromium.org/83803003/diff/10002/pylib/gyp/win_tool.py#new... pylib/gyp/win_tool.py:144: mspdbsrv_helper = MsPdbSrvHelper(args, env) i'm still concerned that there's a race between the compiler/default mspdbsrv completing its writes and the linker pdb starting up. it would be great if you can figure out how to test that, or otherwise, i think we should at least have the beginning of a link -stop the default mspdbsrv.
Thanks for the quick review, here's some comments/notes for myself for when I'll have access to my Windows machine (on my Chromebook now). https://codereview.chromium.org/83803003/diff/10002/pylib/gyp/win_tool.py File pylib/gyp/win_tool.py (right): https://codereview.chromium.org/83803003/diff/10002/pylib/gyp/win_tool.py#new... pylib/gyp/win_tool.py:37: self._is_started = self._MaybeStartMsPdbSrv(env, args) On 2013/11/23 00:07:26, scottmg wrote: > no _ prefix on member variables > > initialize the various things you intend to use as members of this class here to > None: > self.mspdbsrv_start_process = None > self.mspdbsrv_stop_process = None > self.env = None > > |is_started| seems redundant with |mspdbsrv_process|, just remove it? Yeah, if I initialize the members here then is_started could easily be replaced by mspdbsrv_process :) https://codereview.chromium.org/83803003/diff/10002/pylib/gyp/win_tool.py#new... pylib/gyp/win_tool.py:39: def __del__(self): On 2013/11/23 00:07:26, scottmg wrote: > __del__ isn't good as it's when it happens to get GCd, or possibly never. > Instead use __enter__ and __exit__ and in the usage location, use > > with MsPdbSrvHelper(...) as mspdbsrv: > blah > > and then __exit__ will be called on the end of that scope. Good to know, thanks. I'll do this. https://codereview.chromium.org/83803003/diff/10002/pylib/gyp/win_tool.py#new... pylib/gyp/win_tool.py:60: endpoint_name = arg[arg.rfind(':') + 1:] + '_mspdbsrv_endpoint' On 2013/11/23 00:07:26, scottmg wrote: > is the ':' the drive separator? i know i suggested this to test, but instead, > it'd be better to pass the target name through in the calls to gyp-win-tool > link-wrapper and add another argument after $arch that's the endpoint name. > (there's only 3 callsites and they're all windows-only) No, it's the beginning of the PDB filename. There's an arg /PDB:foo.exe.pdb passed to link to link.exe when linking foo.exe. I'll try to find a cleaner way to do this. https://codereview.chromium.org/83803003/diff/10002/pylib/gyp/win_tool.py#new... pylib/gyp/win_tool.py:79: '-spawn') On 2013/11/23 00:07:26, scottmg wrote: > when i run "mspdbsrv -start -spawn -endpoint abcd" from the command line it > doesn't actually seem to spawn, but rather blocks. does it do differently for > you? (i didn't try running it from within python though) Yeah it's blocking, but Popen spawn a child process but doesn't block. I just start the server, call link.exe and stop it afterwards. https://codereview.chromium.org/83803003/diff/10002/pylib/gyp/win_tool.py#new... pylib/gyp/win_tool.py:84: stderr=subprocess.STDOUT) On 2013/11/23 00:07:26, scottmg wrote: > i don't think there's any stdout/stderr, but just in case, don't redirect them > because if there is they'll get swallowed (also, you're not using them anyway. Good point. Thanks. https://codereview.chromium.org/83803003/diff/10002/pylib/gyp/win_tool.py#new... pylib/gyp/win_tool.py:97: _, _ = mspdbsrv_stop_process.communicate() On 2013/11/23 00:07:26, scottmg wrote: > and these can just be .wait() instead of .communicate then once you remove the > stdout/stderr redirects. I'll also use call instead of Popen/communicate for this one. https://codereview.chromium.org/83803003/diff/10002/pylib/gyp/win_tool.py#new... pylib/gyp/win_tool.py:144: mspdbsrv_helper = MsPdbSrvHelper(args, env) On 2013/11/23 00:07:26, scottmg wrote: > i'm still concerned that there's a race between the compiler/default mspdbsrv > completing its writes and the linker pdb starting up. it would be great if you > can figure out how to test that, or otherwise, i think we should at least have > the beginning of a link -stop the default mspdbsrv. I've tried this but it was really crashy, as we're building several target in parallel there might be some other process currently using the default mspdbsrv. I've been able to compile chromium_builder_tests several times with this approach. I think that it works fine because I use another server only for the .exe files, and at this point I think that all the .lib and their PDBs have been written to the disk. Siggi, what do you think?
https://codereview.chromium.org/83803003/diff/10002/pylib/gyp/win_tool.py File pylib/gyp/win_tool.py (right): https://codereview.chromium.org/83803003/diff/10002/pylib/gyp/win_tool.py#new... pylib/gyp/win_tool.py:144: mspdbsrv_helper = MsPdbSrvHelper(args, env) On 2013/11/23 04:30:14, Sébastien Marchand wrote: > On 2013/11/23 00:07:26, scottmg wrote: > > i'm still concerned that there's a race between the compiler/default mspdbsrv > > completing its writes and the linker pdb starting up. it would be great if you > > can figure out how to test that, or otherwise, i think we should at least have > > the beginning of a link -stop the default mspdbsrv. > > I've tried this but it was really crashy, as we're building several target in > parallel there might be some other process currently using the default mspdbsrv. > > > I've been able to compile chromium_builder_tests several times with this > approach. I think that it works fine because I use another server only for the > .exe files, and at this point I think that all the .lib and their PDBs have been > written to the disk. Siggi, what do you think? Hmm, OK. :( See what Siggi thinks, but I'm worried that's a deal breaker unless we can be confident that the pdbs associated with the compile steps are completely flushed somehow.
nice! https://codereview.chromium.org/83803003/diff/10002/pylib/gyp/win_tool.py File pylib/gyp/win_tool.py (right): https://codereview.chromium.org/83803003/diff/10002/pylib/gyp/win_tool.py#new... pylib/gyp/win_tool.py:33: linkers linking a .exe target. This helps to reduce the memory pressure one nit: a->an .exe Is this factual, though? You also do the same thing for .dll targets? https://codereview.chromium.org/83803003/diff/10002/pylib/gyp/win_tool.py#new... pylib/gyp/win_tool.py:55: # Checks if this linker produce a PDB for an .exe target. If so use the nit: produces https://codereview.chromium.org/83803003/diff/10002/pylib/gyp/win_tool.py#new... pylib/gyp/win_tool.py:59: if arg.endswith('exe.pdb'): interesting, why only .exes? I'd think DLLs are just as prone to bloat, especially chrome.dll? https://codereview.chromium.org/83803003/diff/10002/pylib/gyp/win_tool.py#new... pylib/gyp/win_tool.py:59: if arg.endswith('exe.pdb'): Maybe use a regexp to match the argument and the parameter? https://codereview.chromium.org/83803003/diff/10002/pylib/gyp/win_tool.py#new... pylib/gyp/win_tool.py:68: # not set then the default endpoint is used). nit: you might as well go properly unique here and mix in your pid. This will allow multiple builds on the same builder without conflict. Probably not a practical issue, but debugging it if it occurs would be maddening :). https://codereview.chromium.org/83803003/diff/10002/pylib/gyp/win_tool.py#new... pylib/gyp/win_tool.py:144: mspdbsrv_helper = MsPdbSrvHelper(args, env) On 2013/11/23 04:30:14, Sébastien Marchand wrote: > On 2013/11/23 00:07:26, scottmg wrote: > > i'm still concerned that there's a race between the compiler/default mspdbsrv > > completing its writes and the linker pdb starting up. it would be great if you > > can figure out how to test that, or otherwise, i think we should at least have > > the beginning of a link -stop the default mspdbsrv. > > I've tried this but it was really crashy, as we're building several target in > parallel there might be some other process currently using the default mspdbsrv. > > > I've been able to compile chromium_builder_tests several times with this > approach. I think that it works fine because I use another server only for the > .exe files, and at this point I think that all the .lib and their PDBs have been > written to the disk. Siggi, what do you think? Well, it's either safe or it isn't. It's safe iff: - mspdbsrv.exe uses immediate, blocking, cached writes to store new data on each call. - mdpdbsrv.exe does not open the files exclusively. Under these conditions (assuming build dependencies are correct, so that the type pdbs aren't concurrently modified), you'd be able to open a second handle to a PDB open in another pdbsrv instance, and you'd be able to read consistent data from it. I can't think of less stringent conditions under which you can be sure this is safe. Alternatively (again assuming the build deps are correct), you can probably put the cachetime argument in the default pdbsrv's environment, and then you can simply wait for cachetime before allowing the link to start.
https://codereview.chromium.org/83803003/diff/10002/pylib/gyp/win_tool.py File pylib/gyp/win_tool.py (right): https://codereview.chromium.org/83803003/diff/10002/pylib/gyp/win_tool.py#new... pylib/gyp/win_tool.py:33: linkers linking a .exe target. This helps to reduce the memory pressure one On 2013/11/25 16:29:57, Sigurður Ásgeirsson wrote: > nit: a->an .exe > > Is this factual, though? You also do the same thing for .dll targets? my first goal was to only use the mspdbsrv sharding for the .exe (as I plan to test this on a unittest builder), and if everything work fine I was going to add support for the dlls. https://codereview.chromium.org/83803003/diff/10002/pylib/gyp/win_tool.py#new... pylib/gyp/win_tool.py:59: if arg.endswith('exe.pdb'): On 2013/11/25 16:29:57, Sigurður Ásgeirsson wrote: > Maybe use a regexp to match the argument and the parameter? For now I'm only doing this for the .exe because I want to give it a first try first on the Win ASan builder. I'll use a regex (to easily support more extensions. https://codereview.chromium.org/83803003/diff/10002/pylib/gyp/win_tool.py#new... pylib/gyp/win_tool.py:68: # not set then the default endpoint is used). On 2013/11/25 16:29:57, Sigurður Ásgeirsson wrote: > nit: you might as well go properly unique here and mix in your pid. This will > allow multiple builds on the same builder without conflict. > Probably not a practical issue, but debugging it if it occurs would be maddening > :). Good point, will do. https://codereview.chromium.org/83803003/diff/10002/pylib/gyp/win_tool.py#new... pylib/gyp/win_tool.py:144: mspdbsrv_helper = MsPdbSrvHelper(args, env) On 2013/11/25 16:29:57, Sigurður Ásgeirsson wrote: > On 2013/11/23 04:30:14, Sébastien Marchand wrote: > > On 2013/11/23 00:07:26, scottmg wrote: > > > i'm still concerned that there's a race between the compiler/default > mspdbsrv > > > completing its writes and the linker pdb starting up. it would be great if > you > > > can figure out how to test that, or otherwise, i think we should at least > have > > > the beginning of a link -stop the default mspdbsrv. > > > > I've tried this but it was really crashy, as we're building several target in > > parallel there might be some other process currently using the default > mspdbsrv. > > > > > > I've been able to compile chromium_builder_tests several times with this > > approach. I think that it works fine because I use another server only for the > > .exe files, and at this point I think that all the .lib and their PDBs have > been > > written to the disk. Siggi, what do you think? > > Well, it's either safe or it isn't. It's safe iff: > - mspdbsrv.exe uses immediate, blocking, cached writes to store new data on each > call. > - mdpdbsrv.exe does not open the files exclusively. > > Under these conditions (assuming build dependencies are correct, so that the > type pdbs aren't concurrently modified), you'd be able to open a second handle > to a PDB open in another pdbsrv instance, and you'd be able to read consistent > data from it. > I can't think of less stringent conditions under which you can be sure this is > safe. > > Alternatively (again assuming the build deps are correct), you can probably put > the cachetime argument in the default pdbsrv's environment, and then you can > simply wait for cachetime before allowing the link to start. Ok, so I've used procmon to look at what happen in the pipeline (with Siggi looking over my shoulder), and it seems that this approach is safe: - The writes are consistent (sync and cached writes). - The pdb are always opened with - ShareMode: Read, Write, Delete - Options: Synchronous IO Non-Alert, Non-Directory File So according to Siggi it's enough to think that it's safe (that and the fact that I've been able to compile chromium_builder_tests several time). In any case my plan is to let this running on the Win ASan builder for a while before considering using it somewhere else.
Here's a second patchset, PTAnL. Still no unittest, I'll try to find a way to test this tomorrow. https://codereview.chromium.org/83803003/diff/10002/pylib/gyp/win_tool.py File pylib/gyp/win_tool.py (right): https://codereview.chromium.org/83803003/diff/10002/pylib/gyp/win_tool.py#new... pylib/gyp/win_tool.py:55: # Checks if this linker produce a PDB for an .exe target. If so use the On 2013/11/25 16:29:57, Sigurður Ásgeirsson wrote: > nit: produces Done. https://codereview.chromium.org/83803003/diff/10002/pylib/gyp/win_tool.py#new... pylib/gyp/win_tool.py:59: if arg.endswith('exe.pdb'): On 2013/11/25 16:29:57, Sigurður Ásgeirsson wrote: > Maybe use a regexp to match the argument and the parameter? Done.
lgtm with changes below, but looks better to me if you can come up with a sensible test. https://codereview.chromium.org/83803003/diff/110001/pylib/gyp/win_tool.py File pylib/gyp/win_tool.py (right): https://codereview.chromium.org/83803003/diff/110001/pylib/gyp/win_tool.py#ne... pylib/gyp/win_tool.py:57: return False just return, not return False https://codereview.chromium.org/83803003/diff/110001/pylib/gyp/win_tool.py#ne... pylib/gyp/win_tool.py:63: return False and here https://codereview.chromium.org/83803003/diff/110001/pylib/gyp/win_tool.py#ne... pylib/gyp/win_tool.py:75: return False and here https://codereview.chromium.org/83803003/diff/110001/pylib/gyp/win_tool.py#ne... pylib/gyp/win_tool.py:91: return no return https://codereview.chromium.org/83803003/diff/110001/pylib/gyp/win_tool.py#ne... pylib/gyp/win_tool.py:101: return no return https://codereview.chromium.org/83803003/diff/110001/pylib/gyp/win_tool.py#ne... pylib/gyp/win_tool.py:146: popen = None this isn't necessary. (i know it looks scary, but locals are function scope) https://codereview.chromium.org/83803003/diff/110001/pylib/gyp/win_tool.py#ne... pylib/gyp/win_tool.py:147: with WinTool.MsPdbSrvHelper(env, args) as mspdbsrv_helper: you can remove ' as mspdbsrv_helper' since it's never referenced
nice! https://codereview.chromium.org/83803003/diff/110001/pylib/gyp/win_tool.py File pylib/gyp/win_tool.py (right): https://codereview.chromium.org/83803003/diff/110001/pylib/gyp/win_tool.py#ne... pylib/gyp/win_tool.py:22: _LINK_EXE_PDB_ARG = re.compile('/PDB\:(?P<pdb>[^ ]+\.exe\.pdb)', Do you need to escape the ":"? Note that this re will not match PDB paths with embedded spaces, and it'll terminate at the first .exe.pdb. This is probably not what you intend, perhaps you want something like: ".+\.exe\.pdb$" - or if there can be trailing whitespace, you can trim it with ".+\.exe\.pdb$)\s+" (IIRC). You don't actually need the verbose flag here, AFAICT. You'd need it if you have embedded white space or comments in your RE, like e.g. so: re.compile(''' /PDB: # Match the flag. (?P<pdb>[^ ]+\.exe\.pdb$) # Match the file name ''' https://codereview.chromium.org/83803003/diff/110001/pylib/gyp/win_tool.py#ne... pylib/gyp/win_tool.py:47: self._MaybeStartMsPdbSrv() Stupid question: do you need to manually start the pdbsrv, or will the linker start it if it's MIA? https://codereview.chromium.org/83803003/diff/110001/pylib/gyp/win_tool.py#ne... pylib/gyp/win_tool.py:71: endpoint_name = m.group('pdb') + '_' + str(os.getpid()) IMHO clearer so use string formatting for this, e.g. endpoint_name = "%s_%d" % (...) https://codereview.chromium.org/83803003/diff/110001/pylib/gyp/win_tool.py#ne... pylib/gyp/win_tool.py:87: mspdbsrv_args = ('mspdbsrv.exe', '-start', '-spawn') I'd be surprised if the stop command isn't synchronous to shutting down the instance. E.g. if the linker auto-spawns it, you can probably get away with just stopping it when you're done. https://codereview.chromium.org/83803003/diff/110001/pylib/gyp/win_tool.py#ne... pylib/gyp/win_tool.py:146: popen = None nit: perhaps a more descriptive name, while you're in there? Perhaps link_wrapper? https://codereview.chromium.org/83803003/diff/110001/pylib/gyp/win_tool.py#ne... pylib/gyp/win_tool.py:245: popen = subprocess.Popen(args, shell=True, env=env, cwd=dir) isn't this === return subprocess.call(...)?
Thanks, Siggi PTAnL. Any idea for a test ? I was planning to test the MsPdbSrvHelper class directly: - Create an instance (put /PDB:foo.exe.pdb in the arg list) - call __enter__ (indirectly, through a "with ... as ...") - Check that I can't start another instance (call "mspdbsrv.exe -start -spawn -endpoint foo.exe.pdb_%pid%" and check that the return code is != 0) - Do several variants of this (different pdb name etc). https://codereview.chromium.org/83803003/diff/110001/pylib/gyp/win_tool.py File pylib/gyp/win_tool.py (right): https://codereview.chromium.org/83803003/diff/110001/pylib/gyp/win_tool.py#ne... pylib/gyp/win_tool.py:22: _LINK_EXE_PDB_ARG = re.compile('/PDB\:(?P<pdb>[^ ]+\.exe\.pdb)', On 2013/11/26 14:31:05, Sigurður Ásgeirsson wrote: > Do you need to escape the ":"? > Note that this re will not match PDB paths with embedded spaces, and it'll > terminate at the first .exe.pdb. This is probably not what you intend, perhaps > you want something like: > ".+\.exe\.pdb$" - or if there can be trailing whitespace, you can trim it with > ".+\.exe\.pdb$)\s+" (IIRC). > > You don't actually need the verbose flag here, AFAICT. > You'd need it if you have embedded white space or comments in your RE, like e.g. > so: > re.compile(''' > /PDB: # Match the flag. > (?P<pdb>[^ ]+\.exe\.pdb$) # Match the file name > ''' Thanks for the Regex 101 :), fixed. https://codereview.chromium.org/83803003/diff/110001/pylib/gyp/win_tool.py#ne... pylib/gyp/win_tool.py:47: self._MaybeStartMsPdbSrv() On 2013/11/26 14:31:05, Sigurður Ásgeirsson wrote: > Stupid question: do you need to manually start the pdbsrv, or will the linker > start it if it's MIA? Yeah, the linker will automatically spawn it but I'm seeing some weird things when I don't take ownership of it. The main reason why I start it here is to be able to wait for its termination in the stop function. (otherwise it looks like some mspdbsrv processes are not correctly stopped) https://codereview.chromium.org/83803003/diff/110001/pylib/gyp/win_tool.py#ne... pylib/gyp/win_tool.py:57: return False On 2013/11/26 00:44:54, scottmg wrote: > just return, not return False Oops, done. https://codereview.chromium.org/83803003/diff/110001/pylib/gyp/win_tool.py#ne... pylib/gyp/win_tool.py:63: return False On 2013/11/26 00:44:54, scottmg wrote: > and here Done. https://codereview.chromium.org/83803003/diff/110001/pylib/gyp/win_tool.py#ne... pylib/gyp/win_tool.py:71: endpoint_name = m.group('pdb') + '_' + str(os.getpid()) On 2013/11/26 14:31:05, Sigurður Ásgeirsson wrote: > IMHO clearer so use string formatting for this, e.g. > > endpoint_name = "%s_%d" % (...) Done. https://codereview.chromium.org/83803003/diff/110001/pylib/gyp/win_tool.py#ne... pylib/gyp/win_tool.py:75: return False On 2013/11/26 00:44:54, scottmg wrote: > and here Done. https://codereview.chromium.org/83803003/diff/110001/pylib/gyp/win_tool.py#ne... pylib/gyp/win_tool.py:87: mspdbsrv_args = ('mspdbsrv.exe', '-start', '-spawn') On 2013/11/26 14:31:05, Sigurður Ásgeirsson wrote: > I'd be surprised if the stop command isn't synchronous to shutting down the > instance. E.g. if the linker auto-spawns it, you can probably get away with just > stopping it when you're done. It looks like you're right. https://codereview.chromium.org/83803003/diff/110001/pylib/gyp/win_tool.py#ne... pylib/gyp/win_tool.py:91: return On 2013/11/26 00:44:54, scottmg wrote: > no return Done. https://codereview.chromium.org/83803003/diff/110001/pylib/gyp/win_tool.py#ne... pylib/gyp/win_tool.py:101: return On 2013/11/26 00:44:54, scottmg wrote: > no return Done. https://codereview.chromium.org/83803003/diff/110001/pylib/gyp/win_tool.py#ne... pylib/gyp/win_tool.py:146: popen = None On 2013/11/26 00:44:54, scottmg wrote: > this isn't necessary. (i know it looks scary, but locals are function scope) This is scary. https://codereview.chromium.org/83803003/diff/110001/pylib/gyp/win_tool.py#ne... pylib/gyp/win_tool.py:146: popen = None On 2013/11/26 14:31:05, Sigurður Ásgeirsson wrote: > nit: perhaps a more descriptive name, while you're in there? > Perhaps link_wrapper? Done. https://codereview.chromium.org/83803003/diff/110001/pylib/gyp/win_tool.py#ne... pylib/gyp/win_tool.py:147: with WinTool.MsPdbSrvHelper(env, args) as mspdbsrv_helper: On 2013/11/26 00:44:54, scottmg wrote: > you can remove ' as mspdbsrv_helper' since it's never referenced Done. https://codereview.chromium.org/83803003/diff/110001/pylib/gyp/win_tool.py#ne... pylib/gyp/win_tool.py:245: popen = subprocess.Popen(args, shell=True, env=env, cwd=dir) On 2013/11/26 14:31:05, Sigurður Ásgeirsson wrote: > isn't this === return subprocess.call(...)? It looks like it is. Done.
Sorry for going around in circles on this, but this morning it occurred to me that we don't really need any of the process start/stop stuff, right? That's how mspdbsrv works now? i.e. all we need to do is set _MSPDBSRV_ENDPOINT_ in the environment, then run the link as usual and link will take care of starting the mspdbsrv instance, and it'll eventually time out when it's not in use any more? I just did a quick test and this seems to work as desired. https://codereview.chromium.org/83803003/diff/150001/pylib/gyp/win_tool.py File pylib/gyp/win_tool.py (right): https://codereview.chromium.org/83803003/diff/150001/pylib/gyp/win_tool.py#ne... pylib/gyp/win_tool.py:141: link_wrapper = subprocess.Popen(args, if you want to change this, it shouldn't be "link_wrapper", it's just "link".
Thanks. Yeah we don't have to start mspdbsrv manually, it's just that the default timeout is 600sec, that makes a lot of mspdbsrv zombies when you build chromium_builder_tests but it's not a big deal (their working set is pretty small so it shouldn't be noticeable on a builder with a decent quantity of RAM). https://codereview.chromium.org/83803003/diff/150001/pylib/gyp/win_tool.py File pylib/gyp/win_tool.py (right): https://codereview.chromium.org/83803003/diff/150001/pylib/gyp/win_tool.py#ne... pylib/gyp/win_tool.py:141: link_wrapper = subprocess.Popen(args, On 2013/11/27 20:00:44, scottmg wrote: > if you want to change this, it shouldn't be "link_wrapper", it's just "link". Done.
https://codereview.chromium.org/83803003/diff/190001/pylib/gyp/win_tool.py File pylib/gyp/win_tool.py (right): https://codereview.chromium.org/83803003/diff/190001/pylib/gyp/win_tool.py#ne... pylib/gyp/win_tool.py:118: mspdbsrv_helper = WinTool.MsPdbSrvHelper(env, args) this can just be a function now (and it doesn't actually "Start" anything :)
Thanks ! I guess the unittest for this would just be to check that _MSPDBSRV_ENDPOINT_ is added to the env block if GYP_USE_SEPARATE_MSPDBSRV is set ? A more accurate test will be to ensure that there's really a new instance of mspdbsrv.exe but I'm not sure the this'll integrate really well with the current test framework.. https://codereview.chromium.org/83803003/diff/190001/pylib/gyp/win_tool.py File pylib/gyp/win_tool.py (right): https://codereview.chromium.org/83803003/diff/190001/pylib/gyp/win_tool.py#ne... pylib/gyp/win_tool.py:118: mspdbsrv_helper = WinTool.MsPdbSrvHelper(env, args) On 2013/11/27 20:33:41, scottmg wrote: > this can just be a function now (and it doesn't actually "Start" anything :) Of course ! It's much simpler now that we know how this work :)
so clean, so nice! lgtm https://codereview.chromium.org/83803003/diff/220001/pylib/gyp/win_tool.py File pylib/gyp/win_tool.py (right): https://codereview.chromium.org/83803003/diff/220001/pylib/gyp/win_tool.py#ne... pylib/gyp/win_tool.py:22: _LINK_EXE_PDB_ARG = re.compile('/PDB:(?P<pdb>.+\.exe\.pdb)', re.IGNORECASE) nit: this RE will match any string that contains '.exe.pdb'. You probably want ... .exe\.pdb)$' to force it to only match strings that end in .exe.pdb?
LGTM I'm not sure how useful a test for this would be, so I think it's fine to land this as-is.
Thanks for the quick reviews ! I'm not sure that a test would be really useful too... (Except if you want to guarantee a minimal coverage with you unittests...). Landing this, then I'll roll deps on Chrome and I'll set the GYP_USE_SEPARATE_MSPDBSRV var on the Win ASan Builder ! Can't wait to see if it fix my issues :) (it's the case locally, so I expect that it'd be the case on the builder) https://codereview.chromium.org/83803003/diff/220001/pylib/gyp/win_tool.py File pylib/gyp/win_tool.py (right): https://codereview.chromium.org/83803003/diff/220001/pylib/gyp/win_tool.py#ne... pylib/gyp/win_tool.py:22: _LINK_EXE_PDB_ARG = re.compile('/PDB:(?P<pdb>.+\.exe\.pdb)', re.IGNORECASE) On 2013/11/27 20:55:37, Sigurður Ásgeirsson wrote: > nit: this RE will match any string that contains '.exe.pdb'. You probably want > ... .exe\.pdb)$' to force it to only match strings that end in .exe.pdb? Done.
On 2013/11/27 21:07:16, Sébastien Marchand wrote: > Thanks for the quick reviews ! I'm not sure that a test would be really useful > too... (Except if you want to guarantee a minimal coverage with you > unittests...). Landing this, then I'll roll deps on Chrome and I'll set the > GYP_USE_SEPARATE_MSPDBSRV var on the Win ASan Builder ! Can't wait to see if it > fix my issues :) (it's the case locally, so I expect that it'd be the case on > the builder) Unfortunately we can't roll gyp DEPS in chrome until the next release of goma due to https://code.google.com/p/gyp/source/detail?r=1791 . > > https://codereview.chromium.org/83803003/diff/220001/pylib/gyp/win_tool.py > File pylib/gyp/win_tool.py (right): > > https://codereview.chromium.org/83803003/diff/220001/pylib/gyp/win_tool.py#ne... > pylib/gyp/win_tool.py:22: _LINK_EXE_PDB_ARG = > re.compile('/PDB:(?P<pdb>.+\.exe\.pdb)', re.IGNORECASE) > On 2013/11/27 20:55:37, Sigurður Ásgeirsson wrote: > > nit: this RE will match any string that contains '.exe.pdb'. You probably want > > ... .exe\.pdb)$' to force it to only match strings that end in .exe.pdb? > > Done.
Message was sent while issue was closed.
Committed patchset #6 manually as r1801. |
