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)
@@ -10,12 +10,17 @@
 """
 
 import os
+import re
 import shutil
 import subprocess
 import sys
 
 BASE_DIR = os.path.dirname(os.path.abspath(__file__))
 
+# A regex matching an argument corresponding to a PDB filename passed as an
+# argument to link.exe.
+_LINK_EXE_PDB_ARG = re.compile('/PDB\:(?P<pdb>[^ ]+\.exe\.pdb)',

[Sigurður Ásgeirsson, 2013/11/26 14:31:05]

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
'''

[Sébastien Marchand, 2013/11/27 19:26:57]

Thanks for the Regex 101 :), fixed.

+                               re.VERBOSE | re.IGNORECASE)
 
 def main(args):
   executor = WinTool()
@@ -28,6 +33,73 @@
   """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 an .exe target. This helps to reduce the memory pressure one
+    the shared instance of mspdbsrv.exe."""
+
+    def __init__(self, env, args):
+      self.env = env
+      self.args = args
+      self.mspdbsrv_process = None
+
+    def __enter__(self):
+      self._MaybeStartMsPdbSrv()

[Sigurður Ásgeirsson, 2013/11/26 14:31:05]

Stupid question: do you need to manually start the pdbsrv, or will the linker
start it if it's MIA?

[Sébastien Marchand, 2013/11/27 19:26:57]

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)

+
+    def __exit__(self, type, value, traceback):
+      if self.mspdbsrv_process is not None:
+        self._StopMsPdbSrv()
+
+    def _MaybeStartMsPdbSrv(self):
+      """Starts an unique instance of mspdbsrv.exe if the given arguments are
+      those of a linker linking a .exe target."""
+      if not os.environ.get('GYP_USE_SEPARATE_MSPDBSRV'):
+        return False

[scottmg, 2013/11/26 00:44:54]

just return, not return False

[Sébastien Marchand, 2013/11/27 19:26:57]

Oops, done.

+
+      if len(self.args) < 1:
+        raise Exception("Not enough arguments")
+
+      if self.args[0] != 'link.exe':
+        return False

[scottmg, 2013/11/26 00:44:54]

and here

[Sébastien Marchand, 2013/11/27 19:26:57]

Done.

+
+      # Checks if this linker produces a PDB for an .exe target. If so use the
+      # name of this PDB to generate an endpoint name for mspdbsrv.exe.
+      endpoint_name = None
+      for arg in self.args:
+        m = _LINK_EXE_PDB_ARG.match(arg)
+        if m:
+          endpoint_name = m.group('pdb') + '_' + str(os.getpid())

[Sigurður Ásgeirsson, 2013/11/26 14:31:05]

IMHO clearer so use string formatting for this, e.g. 

endpoint_name = "%s_%d" % (...)

[Sébastien Marchand, 2013/11/27 19:26:57]

Done.

+          break
+
+      if endpoint_name is None:
+        return False

[scottmg, 2013/11/26 00:44:54]

and here

[Sébastien Marchand, 2013/11/27 19:26:57]

Done.

+
+      # 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).
+      self.env['_MSPDBSRV_ENDPOINT_'] = endpoint_name
+
+      # 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')

[Sigurður Ásgeirsson, 2013/11/26 14:31:05]

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.

[Sébastien Marchand, 2013/11/27 19:26:57]

It looks like you're right.

+      self.mspdbsrv_process = subprocess.Popen(mspdbsrv_args,
+                                               shell=True,
+                                               env=self.env)
+      return

[scottmg, 2013/11/26 00:44:54]

no return

[Sébastien Marchand, 2013/11/27 19:26:57]

Done.

+
+    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.call(mspdbsrv_stop_args,
+                                              shell=True,
+                                              env=self.env)
+      self.mspdbsrv_process.wait()
+      return

[scottmg, 2013/11/26 00:44:54]

no return

[Sébastien Marchand, 2013/11/27 19:26:57]

Done.

+
   def Dispatch(self, args):
     """Dispatches a string command to a method."""
     if len(args) < 1:
@@ -71,12 +143,14 @@
     This happens when there are exports from the dll or exe.
     """
     env = self._GetEnv(arch)
-    popen = subprocess.Popen(args, shell=True, env=env,
-                              stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
-    out, _ = popen.communicate()
-    for line in out.splitlines():
-      if not line.startswith('   Creating library '):
-        print line
+    popen = None

[scottmg, 2013/11/26 00:44:54]

this isn't necessary. (i know it looks scary, but locals are function scope)

[Sigurður Ásgeirsson, 2013/11/26 14:31:05]

nit: perhaps a more descriptive name, while you're in there?
Perhaps link_wrapper?

[Sébastien Marchand, 2013/11/27 19:26:57]

This is scary.

[Sébastien Marchand, 2013/11/27 19:26:57]

Done.

+    with WinTool.MsPdbSrvHelper(env, args) as mspdbsrv_helper:

[scottmg, 2013/11/26 00:44:54]

you can remove ' as mspdbsrv_helper' since it's never referenced

[Sébastien Marchand, 2013/11/27 19:26:57]

Done.

+      popen = subprocess.Popen(args, shell=True, env=env,
+                               stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
+      out, _ = popen.communicate()
+      for line in out.splitlines():
+        if not line.startswith('   Creating library '):
+          print line
     return popen.returncode
 
   def ExecManifestWrapper(self, arch, *args):