Adds an helper class to shard mspdbsrv.

pylib/gyp/win_tool.py

Index: pylib/gyp/win_tool.py
===================================================================
--- pylib/gyp/win_tool.py	(revision 1796)
+++ pylib/gyp/win_tool.py	(working copy)
@@ -28,6 +28,76 @@
   """This class performs all the Windows tooling steps. The methods can either
   be executed directly, or dispatched from an argument list."""
 
+  class MsPdbSrvHelper(object):
+    """This is an helper class to use a unique instance of mspdbsrv.exe for the
+    linkers linking a .exe target. This helps to reduce the memory pressure one

[Sigurður Ásgeirsson, 2013/11/25 16:29:57]

nit: a->an .exe

Is this factual, though? You also do the same thing for .dll targets?

[Sébastien Marchand, 2013/11/25 19:24:40]

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.

+    the shared instance of mspdbsrv.exe."""
+
+    def __init__(self, env, args):
+      self._is_started = self._MaybeStartMsPdbSrv(env, args)

[scottmg, 2013/11/23 00:07:26]

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?

[Sébastien Marchand, 2013/11/23 04:30:14]

Yeah, if I initialize the members here then is_started could easily be replaced
by mspdbsrv_process :)

+
+    def __del__(self):

[scottmg, 2013/11/23 00:07:26]

__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.

[Sébastien Marchand, 2013/11/23 04:30:14]

Good to know, thanks. I'll do this.

+      if self._is_started:
+        self._StopMsPdbSrv()
+
+    def _MaybeStartMsPdbSrv(self, env, args):
+      """Starts an unique instance of mspdbsrv.exe if the given arguments are
+      those of a linker linking a .exe target."""
+      if os.environ.get('GYP_USE_SEPARATE_MSPDBSRV'):
+        return False
+
+      if len(args) < 1:
+        raise Exception("Not enough arguments")
+
+      if args[0] != 'link.exe':
+        return False
+
+      # Checks if this linker produce a PDB for an .exe target. If so use the

[Sigurður Ásgeirsson, 2013/11/25 16:29:57]

nit: produces

[Sébastien Marchand, 2013/11/26 00:37:56]

Done.

+      # name of this PDB to generate an endpoint name for mspdbsrv.exe.
+      endpoint_name = None
+      for arg in args:
+        if arg.endswith('exe.pdb'):

[Sigurður Ásgeirsson, 2013/11/25 16:29:57]

interesting, why only .exes? I'd think DLLs are just as prone to bloat,
especially chrome.dll?

[Sigurður Ásgeirsson, 2013/11/25 16:29:57]

Maybe use a regexp to match the argument and the parameter?

[Sébastien Marchand, 2013/11/25 19:24:40]

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.

[Sébastien Marchand, 2013/11/26 00:37:56]

Done.

+          endpoint_name = arg[arg.rfind(':') + 1:] + '_mspdbsrv_endpoint'

[scottmg, 2013/11/23 00:07:26]

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)

[Sébastien Marchand, 2013/11/23 04:30:14]

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.

+          break
+
+      if endpoint_name is None:
+        return False
+
+      # Adds the appropriate environment variable. This will be read by link.exe
+      # to know which instance of mspdbsrv.exe it should connect to (if it's
+      # not set then the default endpoint is used).

[Sigurður Ásgeirsson, 2013/11/25 16:29:57]

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
:).

[Sébastien Marchand, 2013/11/25 19:24:40]

Good point, will do.

+      env['_MSPDBSRV_ENDPOINT_'] = endpoint_name
+      self._env = env
+
+      # Starts the mspdbsrv.exe instance manually. We could specify the endpoint
+      # in the command line but it's redundant with setting it in the
+      # environment block. Starting this process manually allows us to wait for
+      # its completion once the link step is finished. This is required to
+      # respect the dependencies between the different targets.
+      mspdbsrv_args = ('mspdbsrv.exe',
+                       '-start',
+                       '-spawn')

[scottmg, 2013/11/23 00:07:26]

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)

[Sébastien Marchand, 2013/11/23 04:30:14]

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.

+      self._mspdbsrv_process = subprocess.Popen(mspdbsrv_args,
+                                                shell=True,
+                                                env=env,
+                                                stdout=subprocess.PIPE,
+                                                stderr=subprocess.STDOUT)

[scottmg, 2013/11/23 00:07:26]

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.

[Sébastien Marchand, 2013/11/23 04:30:14]

Good point. Thanks.

+      return True
+
+    def _StopMsPdbSrv(self):
+      """Stop the instance of mspdbsrv.exe that has been started by this
+      helper."""
+      mspdbsrv_stop_args = ('mspdbsrv.exe',
+                            '-stop')
+      mspdbsrv_stop_process = subprocess.Popen(mspdbsrv_stop_args,
+                                               shell=True,
+                                               env=self._env,
+                                               stdout=subprocess.PIPE,
+                                               stderr=subprocess.STDOUT)
+      _, _ = mspdbsrv_stop_process.communicate()

[scottmg, 2013/11/23 00:07:26]

and these can just be .wait() instead of .communicate then once you remove the
stdout/stderr redirects.

[Sébastien Marchand, 2013/11/23 04:30:14]

I'll also use call instead of Popen/communicate for this one.

+      _, _ = self._mspdbsrv_process.communicate()
+      return
+
   def Dispatch(self, args):
     """Dispatches a string command to a method."""
     if len(args) < 1:
@@ -71,6 +141,7 @@
     This happens when there are exports from the dll or exe.
     """
     env = self._GetEnv(arch)
+    mspdbsrv_helper = MsPdbSrvHelper(args, env)

[scottmg, 2013/11/23 00:07:26]

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.

[Sébastien Marchand, 2013/11/23 04:30:14]

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?

[scottmg, 2013/11/23 04:44:12]

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.

[Sigurður Ásgeirsson, 2013/11/25 16:29:57]

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.

[Sébastien Marchand, 2013/11/25 19:24:40]

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.

     popen = subprocess.Popen(args, shell=True, env=env,
                               stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
     out, _ = popen.communicate()