|
|
Created:
5 years, 10 months ago by Michael Achenbach Modified:
5 years, 10 months ago Reviewers:
Vyacheslav Egorov (Google), Sven Panne, Michael Starzinger, tandrii(chromium) CC:
v8-dev Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionMake gcmole execute in parallel.
TBR=tandrii@chromium.org
NOTRY=true
Committed: https://crrev.com/94e683b526099fbcb4e28baf1de3e280d2289960
Cr-Commit-Position: refs/heads/master@{#26724}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : Docu #
Total comments: 11
Patch Set 5 : Review #Patch Set 6 : More review #Patch Set 7 : Rebase #
Total comments: 2
Patch Set 8 : Python review #
Created: 5 years, 10 months ago
Messages
Total messages: 17 (5 generated)
machenbach@chromium.org changed reviewers: + mstarzinger@chromium.org, svenpanne@chromium.org, tandrii@chromium.org
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)
vegorov@google.com changed reviewers: + vegorov@google.com
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())
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
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?
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.
New patchsets have been uploaded after l-g-t-m from vegorov@google.com
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.
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?
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
New patchsets have been uploaded after l-g-t-m from tandrii@chromium.org
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
The CQ bit was checked by machenbach@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/931233002/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/94e683b526099fbcb4e28baf1de3e280d2289960 Cr-Commit-Position: refs/heads/master@{#26724} |