|
|
Created:
9 years, 7 months ago by Nico Modified:
9 years, 7 months ago CC:
gyp-developer_googlegroups.com Base URL:
http://gyp.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionTeach gyp about configurable filter commands, add "pymod_do_main" filter
The syntax for gyp commands was "<(command)", which will execute "command" in a shell. Now it's "<filter(command)", and if filter is omitted command is run in a shell. Else, it depends on "filter" what happens.
Add the filter "pymod_do_main", which loads the python module that's the first part of "command", passes the rest of "command" to that module's DoMain() function, and then uses the result of this function. That's roughly equivalent to `<(python module.py parameters)`, but can be a lot faster for modules that have nontrivial startup time (because they are now imported only once by gyp).
This speeds up `build/gyp_chromium` by 8s on my system (with the change to gyp_chromium in http://codereview.chromium.org/7035004).
BUG=http://crbug.com/82230
Comitted: http://code.google.com/p/gyp/source/detail?r=924
Patch Set 1 #Patch Set 2 : '' #
Total comments: 9
Patch Set 3 : '' #Patch Set 4 : '' #Patch Set 5 : '' #Patch Set 6 : '' #Patch Set 7 : '' #
Total comments: 8
Patch Set 8 : '' #
Total comments: 2
Patch Set 9 : '' #
Total comments: 5
Messages
Total messages: 21 (0 generated)
Not very general / pretty, but very effective.
I defer the actual review to Mark. I'm not sure if we want a grit/chromium specific hack in gyp. http://codereview.chromium.org/7034007/diff/2001/pylib/gyp/input.py File pylib/gyp/input.py (right): http://codereview.chromium.org/7034007/diff/2001/pylib/gyp/input.py#newcode29 pylib/gyp/input.py:29: grit_available = False The python idiom for this is on Exception to just set grit_info to None and check for grit_info below. http://codereview.chromium.org/7034007/diff/2001/pylib/gyp/input.py#newcode649 pylib/gyp/input.py:649: parsed_contents[1].endswith('grit_info.py')): Will parsed_contents always have at least 2 valeus? http://codereview.chromium.org/7034007/diff/2001/pylib/gyp/input.py#newcode654 pylib/gyp/input.py:654: replacement = str(grit_info.DoMain(contents.split()[2:]).rstrip()) The extra str() seems unnecessary (doesn't rstrip() always returns a string?).
Hmmn, given how common these are, I'd expect a big win. How much speedup does it give? LGTM http://codereview.chromium.org/7034007/diff/2001/pylib/gyp/input.py File pylib/gyp/input.py (right): http://codereview.chromium.org/7034007/diff/2001/pylib/gyp/input.py#newcode649 pylib/gyp/input.py:649: parsed_contents[1].endswith('grit_info.py')): Maybe raise an exception with a more specific error.
On 2011/05/16 16:35:50, bradn wrote: > Hmmn, given how common these are, I'd expect a big win. > How much speedup does it give? Read the CL description? :-) > > LGTM > > http://codereview.chromium.org/7034007/diff/2001/pylib/gyp/input.py > File pylib/gyp/input.py (right): > > http://codereview.chromium.org/7034007/diff/2001/pylib/gyp/input.py#newcode649 > pylib/gyp/input.py:649: parsed_contents[1].endswith('grit_info.py')): > Maybe raise an exception with a more specific error.
http://codereview.chromium.org/7034007/diff/2001/pylib/gyp/input.py File pylib/gyp/input.py (right): http://codereview.chromium.org/7034007/diff/2001/pylib/gyp/input.py#newcode26 pylib/gyp/input.py:26: import grit_info # TODO: make optional What does “TODO: make optional” mean when this is already inside a try? http://codereview.chromium.org/7034007/diff/2001/pylib/gyp/input.py#newcode646 pylib/gyp/input.py:646: parsed_contents = contents.split() Can you refactor this whole block into its own function that handles subprocess.Popen, p.communicate, and p.wait, or their equivalents when doing things internally? http://codereview.chromium.org/7034007/diff/2001/pylib/gyp/input.py#newcode646 pylib/gyp/input.py:646: parsed_contents = contents.split() shlex instead? http://codereview.chromium.org/7034007/diff/2001/pylib/gyp/input.py#newcode649 pylib/gyp/input.py:649: parsed_contents[1].endswith('grit_info.py')): It certainly seems like you can tighten up the safety of this check. http://codereview.chromium.org/7034007/diff/2001/pylib/gyp/input.py#newcode654 pylib/gyp/input.py:654: replacement = str(grit_info.DoMain(contents.split()[2:]).rstrip()) You already have parsed_contents, why are you going back to contents again?
Oh, yeah, once it’s refactored I’m probably going to say something like “can you make this not the default behavior, but have gyp_chromium set an environment variable or pass a flag or do something else that enables this Chromium-specific hacklet?”
Hacky hack is hacky. How about an alternative implementation: extend gyp syntax that currently lets you inline a command (is it <@! I think?) with an additional flag that has the semantics of "the stuff inside here is a call to a Python module; import it and call module.Main() with argv"). That allows us to at least not hardcode grit within gyp.
On 2011/05/16 19:37:06, Evan Martin wrote: > Hacky hack is hacky. > > How about an alternative implementation: extend gyp syntax that currently lets > you inline a command (is it <@! I think?) with an additional flag that has the > semantics of "the stuff inside here is a call to a Python module; import it and > call module.Main() with argv"). Would something like (pseudocode): if cmd[0] == 'python': module = __import__(cmd[1].rstrip('.py')) if 'DoMain' in module: module.DoMain(cmd[2:]) qualifiy as less hacky? That way it's a transparent optimization that doesn't require more syntax. If you prefer to make the function name explicit in the gyp, then I'll do that instead. > > That allows us to at least not hardcode grit within gyp.
On 2011/05/16 23:05:27, Nico wrote: > if cmd[0] == 'python': > module = __import__(cmd[1].rstrip('.py')) > if 'DoMain' in module: > module.DoMain(cmd[2:]) > > qualifiy as less hacky? That way it's a transparent optimization that doesn't > require more syntax. If you prefer to make the function name explicit in the > gyp, then I'll do that instead. That doesn't seem so bad to me. (Needs try:except: bits, of course.)
On 2011/05/16 23:07:24, Evan Martin wrote: > On 2011/05/16 23:05:27, Nico wrote: > > if cmd[0] == 'python': > > module = __import__(cmd[1].rstrip('.py')) > > if 'DoMain' in module: > > module.DoMain(cmd[2:]) > > > > qualifiy as less hacky? That way it's a transparent optimization that doesn't > > require more syntax. If you prefer to make the function name explicit in the > > gyp, then I'll do that instead. > > That doesn't seem so bad to me. (Needs try:except: bits, of course.) The downside to this is all the other python scripts (other that grit_info.py) will now be imported into gyp. That might also be slow since import requires lots of directory searching or might cause random code to execute (e.g., if someone forgot the __name__ == '__main__' check in their .py file).
Less hacky, more complicated patch is up. I updated the CL description to describe what this new version does.
http://codereview.chromium.org/7034007/diff/12001/pylib/gyp/input.py File pylib/gyp/input.py (right): http://codereview.chromium.org/7034007/diff/12001/pylib/gyp/input.py#newcode539 pylib/gyp/input.py:539: # command string. Currently, only 'python' is supported. Except you’ve called it pymod_do_main in the code below. http://codereview.chromium.org/7034007/diff/12001/pylib/gyp/input.py#newcode662 pylib/gyp/input.py:662: replacement = str(py_module.DoMain(parsed_contents[1:])).rstrip() This is hokey. Any reason you don’t just pass parsed_contents[1:] to DoMain? Is there some existing API that you’re trying to maintain conformity with? If you need to keep a string: Since you split the string up with shlex.split, you should really join it back together with something that makes strings following the same rules shlex uses. http://codereview.chromium.org/7034007/diff/12001/pylib/gyp/input.py#newcode666 pylib/gyp/input.py:666: else: assert on unknown command_strings?
http://codereview.chromium.org/7034007/diff/12001/pylib/gyp/input.py File pylib/gyp/input.py (right): http://codereview.chromium.org/7034007/diff/12001/pylib/gyp/input.py#newcode539 pylib/gyp/input.py:539: # command string. Currently, only 'python' is supported. On 2011/05/24 21:54:57, Mark Mentovai wrote: > Except you’ve called it pymod_do_main in the code below. Done. http://codereview.chromium.org/7034007/diff/12001/pylib/gyp/input.py#newcode662 pylib/gyp/input.py:662: replacement = str(py_module.DoMain(parsed_contents[1:])).rstrip() On 2011/05/24 21:54:57, Mark Mentovai wrote: > This is hokey. Any reason you don’t just pass parsed_contents[1:] to DoMain? I think that's what I'm doing. > Is there some existing API that you’re trying to maintain conformity with? DoMain returns a unicode, but gyp asserts that replacement is a str somewhere below. And the rstrip() just gets rid of eventual trailing newlines. http://codereview.chromium.org/7034007/diff/12001/pylib/gyp/input.py#newcode666 pylib/gyp/input.py:666: else: On 2011/05/24 21:54:57, Mark Mentovai wrote: > assert on unknown command_strings? Done.
http://codereview.chromium.org/7034007/diff/12001/pylib/gyp/input.py File pylib/gyp/input.py (right): http://codereview.chromium.org/7034007/diff/12001/pylib/gyp/input.py#newcode654 pylib/gyp/input.py:654: # passing "param eters" as a single string argument. For modules Sorry, I read this comment saying |passing "param eters" as a single string argument| and my mind put an implicit |' '.join()| around your list. Update the comment, then!
http://codereview.chromium.org/7034007/diff/14001/pylib/gyp/input.py File pylib/gyp/input.py (right): http://codereview.chromium.org/7034007/diff/14001/pylib/gyp/input.py#newcode667 pylib/gyp/input.py:667: raise Exception("Unknown command string '%s' in action '%s'." % Let’s not overload the word “action” any further. This would be fine if you just took that one word out.
http://codereview.chromium.org/7034007/diff/12001/pylib/gyp/input.py File pylib/gyp/input.py (right): http://codereview.chromium.org/7034007/diff/12001/pylib/gyp/input.py#newcode654 pylib/gyp/input.py:654: # passing "param eters" as a single string argument. For modules On 2011/05/24 22:04:16, Mark Mentovai wrote: > Sorry, I read this comment saying |passing "param eters" as a single string > argument| and my mind put an implicit |' '.join()| around your list. > > Update the comment, then! Done. http://codereview.chromium.org/7034007/diff/14001/pylib/gyp/input.py#newcode667 pylib/gyp/input.py:667: # Fix up command with platform specific workarounds. On 2011/05/24 22:06:26, Mark Mentovai wrote: > Let’s not overload the word “action” any further. This would be fine if you just > took that one word out. Acted.
LGTM. You may want to wait for the others to chime in too.
LGTM2 http://codereview.chromium.org/7034007/diff/15002/pylib/gyp/input.py File pylib/gyp/input.py (right): http://codereview.chromium.org/7034007/diff/15002/pylib/gyp/input.py#newcode22 pylib/gyp/input.py:22: import shlex Nit: alphabetize http://codereview.chromium.org/7034007/diff/15002/pylib/gyp/input.py#newcode678 pylib/gyp/input.py:678: (p_stdout, p_stderr) = p.communicate('') Nit: no () on the left side
http://codereview.chromium.org/7034007/diff/15002/pylib/gyp/input.py File pylib/gyp/input.py (right): http://codereview.chromium.org/7034007/diff/15002/pylib/gyp/input.py#newcode651 pylib/gyp/input.py:651: if command_string == 'pymod_do_main': drive by - should we also support pymod_main for the things that just call it Main()?
Thanks! Mark, can you land this? http://codereview.chromium.org/7034007/diff/15002/pylib/gyp/input.py File pylib/gyp/input.py (right): http://codereview.chromium.org/7034007/diff/15002/pylib/gyp/input.py#newcode651 pylib/gyp/input.py:651: if command_string == 'pymod_do_main': On 2011/05/24 23:44:03, TVL wrote: > drive by - should we also support pymod_main for the things that just call it > Main()? We can add this once we need it I think.
http://codereview.chromium.org/7034007/diff/15002/pylib/gyp/input.py File pylib/gyp/input.py (right): http://codereview.chromium.org/7034007/diff/15002/pylib/gyp/input.py#newcode20 pylib/gyp/input.py:20: import shlex shlex was already in here. LGTM for a quick follow-up to remove the duplicate. |