Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1616223002:
rewrite_to_chrome_style: Add a lock file when writing replacements. (Closed)
Descriptionrewrite_to_chrome_style: Add a lock file when writing replacements.
The tool is run in parallel many times, so they may clobber each
other writing to the rewrite-sym.txt file. So add a rewrite-sym.lock
file that each instance grabs when writing.
Also attempts a windows implementation which is untested.
R=dcheng
BUG=578344
Committed: https://crrev.com/bf6983ea366435a931b8f8af4408855b0b580de1
Cr-Commit-Position: refs/heads/master@{#371650}
Patch Set 1 #
Total comments: 4
Patch Set 2 : rewrite-lock: nomemset #
Total comments: 2
Patch Set 3 : rewrite-lock: semicolonsforall #
Total comments: 4
Patch Set 4 : rewrite-lock: windoze #
Total comments: 3
Patch Set 5 : rewrite-lock: reuse-struct #Messages
Total messages: 26 (7 generated)
The CQ bit was checked by danakj@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1616223002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1616223002/1
https://codereview.chromium.org/1616223002/diff/1/tools/clang/rewrite_to_chro... File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right): https://codereview.chromium.org/1616223002/diff/1/tools/clang/rewrite_to_chro... tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:526: memset(&lock_out, 0, sizeof(lock_out)); OVERLAPPED lock_out = {}; then you can skip the memset. https://codereview.chromium.org/1616223002/diff/1/tools/clang/rewrite_to_chro... tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:530: while (flock(lockfd, LOCK_EX)) { // :D Do we need to worry about contention here? Is there any fairness guarantee around flock? I'm curious if how much this affects running time. We could use named semaphores instead (sem_open/sem_wait/sem_post/sem_close), right? Presumably, that has some guarantee of fairness and doesn't have a spinloop like this. If there's no real effect on running time, then I guess this is Good Enough.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1616223002/diff/1/tools/clang/rewrite_to_chro... File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right): https://codereview.chromium.org/1616223002/diff/1/tools/clang/rewrite_to_chro... tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:526: memset(&lock_out, 0, sizeof(lock_out)); On 2016/01/22 00:31:30, dcheng wrote: > OVERLAPPED lock_out = {}; > > then you can skip the memset. Fancy~ https://codereview.chromium.org/1616223002/diff/1/tools/clang/rewrite_to_chro... tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:530: while (flock(lockfd, LOCK_EX)) { // :D On 2016/01/22 00:31:30, dcheng wrote: > Do we need to worry about contention here? Is there any fairness guarantee > around flock? I'm curious if how much this affects running time. > > We could use named semaphores instead (sem_open/sem_wait/sem_post/sem_close), > right? Presumably, that has some guarantee of fairness and doesn't have a > spinloop like this. > > If there's no real effect on running time, then I guess this is Good Enough. I think it's okay, it takes like ~20min to run the tool over everything, but each file's processing time is multiple seconds, the writing output is very tiny compared to it. I didn't take measurements.. but it feels similar to before the lock.
PTAL no memset.
https://codereview.chromium.org/1616223002/diff/20001/tools/clang/rewrite_to_... File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right): https://codereview.chromium.org/1616223002/diff/20001/tools/clang/rewrite_to_... tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:524: OVERLAPPED lock_out = {} LockFileEx(lockfd, LOCKFILE_EXCLUSIVE_LOCK, 0, 1, 0, Missing a ; here I think.
https://codereview.chromium.org/1616223002/diff/20001/tools/clang/rewrite_to_... File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right): https://codereview.chromium.org/1616223002/diff/20001/tools/clang/rewrite_to_... tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:524: OVERLAPPED lock_out = {} LockFileEx(lockfd, LOCKFILE_EXCLUSIVE_LOCK, 0, 1, 0, On 2016/01/23 00:10:07, dcheng wrote: > Missing a ; here I think. oops :) ya
PTAL
The CQ bit was checked by danakj@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1616223002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1616223002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
(Sorry I forgot to send my drafts) https://codereview.chromium.org/1616223002/diff/40001/tools/clang/rewrite_to_... File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right): https://codereview.chromium.org/1616223002/diff/40001/tools/clang/rewrite_to_... tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:523: HFILE lockfd = OpenFile("rewrite-sym.lock", &lockfd_out, OF_CREATE); It might be slightly more robust to use CreateFile(). From MSDN: This function has limited capabilities and is not recommended. For new application development, use the CreateFile function. (specifically, there's a 128 character path limit... which we probably won't hit, but it'd be nice not to have to worry about hitting it by accident while we're trying to get the tree reopened) https://codereview.chromium.org/1616223002/diff/40001/tools/clang/rewrite_to_... tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:545: UnlockFile(lockfd, 0, 0, 1, 0); Nit: UnlockFileEx for symmetry? The parameters are already pretty mysterious looking.
https://codereview.chromium.org/1616223002/diff/40001/tools/clang/rewrite_to_... File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right): https://codereview.chromium.org/1616223002/diff/40001/tools/clang/rewrite_to_... tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:523: HFILE lockfd = OpenFile("rewrite-sym.lock", &lockfd_out, OF_CREATE); On 2016/01/26 07:04:15, dcheng wrote: > It might be slightly more robust to use CreateFile(). From MSDN: > This function has limited capabilities and is not recommended. For new > application development, use the CreateFile function. > > (specifically, there's a 128 character path limit... which we probably won't > hit, but it'd be nice not to have to worry about hitting it by accident while > we're trying to get the tree reopened) Done. I think. https://codereview.chromium.org/1616223002/diff/40001/tools/clang/rewrite_to_... tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:545: UnlockFile(lockfd, 0, 0, 1, 0); On 2016/01/26 07:04:15, dcheng wrote: > Nit: UnlockFileEx for symmetry? The parameters are already pretty mysterious > looking. Done.
lgtm with a nit https://codereview.chromium.org/1616223002/diff/60001/tools/clang/rewrite_to_... File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right): https://codereview.chromium.org/1616223002/diff/60001/tools/clang/rewrite_to_... tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:546: UnlockFileEx(lockfd, 0, 1, 0, &unlock_out); I think (?) you should pass in the OVERLAPPED structure from earlier. We can give it a clear name like "overlapped".
https://codereview.chromium.org/1616223002/diff/60001/tools/clang/rewrite_to_... File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right): https://codereview.chromium.org/1616223002/diff/60001/tools/clang/rewrite_to_... tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:546: UnlockFileEx(lockfd, 0, 1, 0, &unlock_out); On 2016/01/26 23:41:45, dcheng wrote: > I think (?) you should pass in the OVERLAPPED structure from earlier. We can > give it a clear name like "overlapped". I was worried that it is an inout so I would have to reset it before re-using it. Or that's actually good? Ok
https://codereview.chromium.org/1616223002/diff/60001/tools/clang/rewrite_to_... File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right): https://codereview.chromium.org/1616223002/diff/60001/tools/clang/rewrite_to_... tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:546: UnlockFileEx(lockfd, 0, 1, 0, &unlock_out); On 2016/01/26 23:43:51, danakj wrote: > On 2016/01/26 23:41:45, dcheng wrote: > > I think (?) you should pass in the OVERLAPPED structure from earlier. We can > > give it a clear name like "overlapped". > > I was worried that it is an inout so I would have to reset it before re-using > it. Or that's actually good? Ok Done.
The CQ bit was checked by danakj@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/1616223002/#ps80001 (title: "rewrite-lock: reuse-struct")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1616223002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1616223002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== rewrite_to_chrome_style: Add a lock file when writing replacements. The tool is run in parallel many times, so they may clobber each other writing to the rewrite-sym.txt file. So add a rewrite-sym.lock file that each instance grabs when writing. Also attempts a windows implementation which is untested. R=dcheng BUG=578344 ========== to ========== rewrite_to_chrome_style: Add a lock file when writing replacements. The tool is run in parallel many times, so they may clobber each other writing to the rewrite-sym.txt file. So add a rewrite-sym.lock file that each instance grabs when writing. Also attempts a windows implementation which is untested. R=dcheng BUG=578344 Committed: https://crrev.com/bf6983ea366435a931b8f8af4408855b0b580de1 Cr-Commit-Position: refs/heads/master@{#371650} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/bf6983ea366435a931b8f8af4408855b0b580de1 Cr-Commit-Position: refs/heads/master@{#371650} |
