Make gcmole execute in parallel.

Make gcmole execute in parallel.

TBR=tandrii@chromium.org
NOTRY=true

Committed: https://crrev.com/94e683b526099fbcb4e28baf1de3e280d2289960
Cr-Commit-Position: refs/heads/master@{#26724}

[Michael Achenbach, 2015-02-18 10:00:44]

machenbach@chromium.org changed reviewers:
+ mstarzinger@chromium.org, svenpanne@chromium.org, tandrii@chromium.org

[Michael Achenbach, 2015-02-18 10:04:54]

PTAL. Anybody comfortable reviewing lua code (I wasn't comfortable writing it)?
Writing parallel code in lua is a pain, therefore I shell out to python.

Mid-term todo: transform the whole output processing into python instead of
returning the output back to lua using this ugly protocol.

Long-term todo: transform the whole script into python.

https://codereview.chromium.org/931233002/diff/60001/tools/gcmole/gcmole.lua
File tools/gcmole/gcmole.lua (right):

https://codereview.chromium.org/931233002/diff/60001/tools/gcmole/gcmole.lua#...
tools/gcmole/gcmole.lua:162: log("** Sequential execution.")
This is the original piece of code c/p 1:1 (codereview not smart)

[Vyacheslav Egorov (Google), 2015-02-18 10:56:49]

vegorov@google.com changed reviewers:
+ vegorov@google.com

[Vyacheslav Egorov (Google), 2015-02-18 10:56:49]

lua code lgtm

https://codereview.chromium.org/931233002/diff/60001/tools/gcmole/gcmole.lua
File tools/gcmole/gcmole.lua (right):

https://codereview.chromium.org/931233002/diff/60001/tools/gcmole/gcmole.lua#...
tools/gcmole/gcmole.lua:117: function IterTable(t)
I'd write it 

local function IterTable(t)
  return coroutine.wrap(function ()
    for i, v in ipairs(t) do
      coroutine.yield(v)
    end
  end)
end

or (which arguably is uglier)

local function IterTable(t)
  local f, n = ipairs(t), 0
  return function ()
    local v
    n, v = ipairs(t, n)
    return n
  end
end

but see the suggestion below which can eliminate the need for it entirely.

https://codereview.chromium.org/931233002/diff/60001/tools/gcmole/gcmole.lua#...
tools/gcmole/gcmole.lua:182: action = table.concat(parallel_cmd_line, " ")
arguably you can just do

action = table.concat(parallel_cmd_line, " ") .. " " .. table.concat(filenames,
" ")

and kill the loop above

https://codereview.chromium.org/931233002/diff/60001/tools/gcmole/gcmole.lua#...
tools/gcmole/gcmole.lua:185: local success = SplitResults(pipe:lines(), func)
and pipe:close()
pipe will not be closed if SplitResults returns false.

https://codereview.chromium.org/931233002/diff/60001/tools/gcmole/gcmole.lua#...
tools/gcmole/gcmole.lua:409: local function SearchForErrors(filename, lines)
if you change signature of this function and `parse` to (filename, iterator,
lines) and do

for l in iterator(lines) do ... end

then you can write

func(..., ipairs, current) instead of IterTable.

and 

func(..., pipe.lines, pipe) instead of func(..., pipe:lines())

[Vyacheslav Egorov (Google), 2015-02-18 10:59:51]

https://codereview.chromium.org/931233002/diff/60001/tools/gcmole/gcmole.lua
File tools/gcmole/gcmole.lua (right):

https://codereview.chromium.org/931233002/diff/60001/tools/gcmole/gcmole.lua#...
tools/gcmole/gcmole.lua:117: function IterTable(t)
my comment should obviously read:

>     n, v = ipairs(t, n)
>     return v

at the end

[Michael Achenbach, 2015-02-18 11:42:03]

Thanks for the suggestions! I run in some error after the changes. See below.

https://codereview.chromium.org/931233002/diff/60001/tools/gcmole/gcmole.lua
File tools/gcmole/gcmole.lua (right):

https://codereview.chromium.org/931233002/diff/60001/tools/gcmole/gcmole.lua#...
tools/gcmole/gcmole.lua:409: local function SearchForErrors(filename, lines)
On 2015/02/18 10:56:49, Vyacheslav Egorov (Google) wrote:
> if you change signature of this function and `parse` to (filename, iterator,
> lines) and do
> 
> for l in iterator(lines) do ... end
> 
> then you can write
> 
> func(..., ipairs, current) instead of IterTable.
> 
> and 
> 
> func(..., pipe.lines, pipe) instead of func(..., pipe:lines())

hmm but that would lead to
for l in ipairs(current) do ... end
and then l is a number not the value. I think I still need to wrap the iterator
somehow, either in the sequential case or the other - or is there another way?

[Vyacheslav Egorov (Google), 2015-02-18 11:58:12]

https://codereview.chromium.org/931233002/diff/60001/tools/gcmole/gcmole.lua
File tools/gcmole/gcmole.lua (right):

https://codereview.chromium.org/931233002/diff/60001/tools/gcmole/gcmole.lua#...
tools/gcmole/gcmole.lua:409: local function SearchForErrors(filename, lines)
On 2015/02/18 11:42:03, Michael Achenbach wrote:
> On 2015/02/18 10:56:49, Vyacheslav Egorov (Google) wrote:
> > if you change signature of this function and `parse` to (filename, iterator,
> > lines) and do
> > 
> > for l in iterator(lines) do ... end
> > 
> > then you can write
> > 
> > func(..., ipairs, current) instead of IterTable.
> > 
> > and 
> > 
> > func(..., pipe.lines, pipe) instead of func(..., pipe:lines())
> 
> hmm but that would lead to
> for l in ipairs(current) do ... end
> and then l is a number not the value. I think I still need to wrap the
iterator
> somehow, either in the sequential case or the other - or is there another way?

Yeah, true. Guh. 

Yeah, this does not work results are different, was stupid of me to suggest.
Sorry.

Then just a simpler IterTable is the only answer I have.

[Michael Achenbach, 2015-02-18 12:17:51]

New patchsets have been uploaded after l-g-t-m from vegorov@google.com

[Michael Achenbach, 2015-02-18 14:48:59]

Done. Andrii do you have time to review the python part?

https://codereview.chromium.org/931233002/diff/60001/tools/gcmole/gcmole.lua
File tools/gcmole/gcmole.lua (right):

https://codereview.chromium.org/931233002/diff/60001/tools/gcmole/gcmole.lua#...
tools/gcmole/gcmole.lua:117: function IterTable(t)
On 2015/02/18 10:56:49, Vyacheslav Egorov (Google) wrote:
> I'd write it 
> 
> local function IterTable(t)
>   return coroutine.wrap(function ()
>     for i, v in ipairs(t) do
>       coroutine.yield(v)
>     end
>   end)
> end
> 
> or (which arguably is uglier)
> 
> local function IterTable(t)
>   local f, n = ipairs(t), 0
>   return function ()
>     local v
>     n, v = ipairs(t, n)
>     return n
>   end
> end
> 
> but see the suggestion below which can eliminate the need for it entirely.

Done.

https://codereview.chromium.org/931233002/diff/60001/tools/gcmole/gcmole.lua#...
tools/gcmole/gcmole.lua:182: action = table.concat(parallel_cmd_line, " ")
On 2015/02/18 10:56:49, Vyacheslav Egorov (Google) wrote:
> arguably you can just do
> 
> action = table.concat(parallel_cmd_line, " ") .. " " ..
table.concat(filenames,
> " ")
> 
> and kill the loop above

Made it even shorter.

https://codereview.chromium.org/931233002/diff/60001/tools/gcmole/gcmole.lua#...
tools/gcmole/gcmole.lua:185: local success = SplitResults(pipe:lines(), func)
and pipe:close()
On 2015/02/18 10:56:49, Vyacheslav Egorov (Google) wrote:
> pipe will not be closed if SplitResults returns false. 

Done.

[tandrii(chromium), 2015-02-18 15:19:08]

LGTM for Python.

https://codereview.chromium.org/931233002/diff/120001/tools/gcmole/parallel.py
File tools/gcmole/parallel.py (right):

https://codereview.chromium.org/931233002/diff/120001/tools/gcmole/parallel.p...
tools/gcmole/parallel.py:42: print "______________ %s" % filename
I assume this output is for humans, right?

[tandrii(chromium), 2015-02-18 15:20:11]

https://codereview.chromium.org/931233002/diff/120001/tools/gcmole/parallel.py
File tools/gcmole/parallel.py (right):

https://codereview.chromium.org/931233002/diff/120001/tools/gcmole/parallel.p...
tools/gcmole/parallel.py:28: def Invoke(cmdline):
nit: pep8 recommends lowercase

[Michael Achenbach, 2015-02-18 15:28:14]

New patchsets have been uploaded after l-g-t-m from tandrii@chromium.org

[Michael Achenbach, 2015-02-18 15:34:03]

On 2015/02/18 15:20:11, tandrii(chromium) wrote:
> https://codereview.chromium.org/931233002/diff/120001/tools/gcmole/parallel.py
> File tools/gcmole/parallel.py (right):
> 
>
https://codereview.chromium.org/931233002/diff/120001/tools/gcmole/parallel.p...
> tools/gcmole/parallel.py:28: def Invoke(cmdline):
> nit: pep8 recommends lowercase

Done

[Michael Achenbach, 2015-02-18 15:34:53]

The CQ bit was checked by machenbach@chromium.org

[commit-bot: I haz the power, 2015-02-18 15:35:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/931233002/140001

[commit-bot: I haz the power, 2015-02-18 15:35:28]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-02-18 15:35:41]

Patchset 8 (id:??) landed as
https://crrev.com/94e683b526099fbcb4e28baf1de3e280d2289960
Cr-Commit-Position: refs/heads/master@{#26724}