Re: Adds an helper class to shard mspdbsrv. (issue 83803003)

[sco...@chromium.org]

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#newcode37
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#newcode39
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#newcode60
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#newcode79
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#newcode84
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#newcode97
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#newcode144
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.

https://codereview.chromium.org/83803003/

[sco...@chromium.org]

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#newcode144
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.

https://codereview.chromium.org/83803003/

[si...@chromium.org]

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#newcode33
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#newcode55
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#newcode59
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#newcode59
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#newcode68
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#newcode144
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/

[sco...@chromium.org]

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#newcode57
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#newcode63
pylib/gyp/win_tool.py:63: return False
and here

https://codereview.chromium.org/83803003/diff/110001/pylib/gyp/win_tool.py#newcode75
pylib/gyp/win_tool.py:75: return False
and here

https://codereview.chromium.org/83803003/diff/110001/pylib/gyp/win_tool.py#newcode91
pylib/gyp/win_tool.py:91: return
no return

https://codereview.chromium.org/83803003/diff/110001/pylib/gyp/win_tool.py#newcode101
pylib/gyp/win_tool.py:101: return
no return

https://codereview.chromium.org/83803003/diff/110001/pylib/gyp/win_tool.py#newcode146
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#newcode147
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

https://codereview.chromium.org/83803003/

[si...@chromium.org]

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#newcode22
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#newcode47
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#newcode71
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#newcode87
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#newcode146
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#newcode245
pylib/gyp/win_tool.py:245: popen = subprocess.Popen(args, shell=True,
env=env, cwd=dir)
isn't this === return subprocess.call(...)?

https://codereview.chromium.org/83803003/

[sco...@chromium.org]

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#newcode141
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".

https://codereview.chromium.org/83803003/

[sco...@chromium.org]

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#newcode118
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 :)

https://codereview.chromium.org/83803003/

[si...@chromium.org]

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#newcode22
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?

https://codereview.chromium.org/83803003/

[sco...@chromium.org]

LGTM

I'm not sure how useful a test for this would be, so I think it's fine to
land
this as-is.

https://codereview.chromium.org/83803003/

[sco...@chromium.org]

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#newcode22
> 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.


https://codereview.chromium.org/83803003/

[sco...@chromium.org]

Committed patchset #6 manually as r1801.

https://codereview.chromium.org/83803003/