Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2767903002:
[snapshot] only create snapshot files as last step in mksnapshot. (Closed)
Description[snapshot] only create snapshot files as last step in mksnapshot.
R=leszeks@chromium.org
BUG=chromium:633159
Review-Url: https://codereview.chromium.org/2767903002
Cr-Commit-Position: refs/heads/master@{#44015}
Committed: https://chromium.googlesource.com/v8/v8/+/0b90e985f736cae797b23969e6e53e303f018236
Patch Set 1 #
Total comments: 7
Patch Set 2 : address comments #Patch Set 3 : add comment #Messages
Total messages: 19 (12 generated)
Description was changed from ========== [snapshot] only create snapshot files as last step in mksnapshot. R=leszeks@chromium.org ========== to ========== [snapshot] only create snapshot files as last step in mksnapshot. R=leszeks@chromium.org BUG=chromium:633159 ==========
The CQ bit was checked by yangguo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2767903002/diff/1/src/snapshot/mksnapshot.cc File src/snapshot/mksnapshot.cc (left): https://codereview.chromium.org/2767903002/diff/1/src/snapshot/mksnapshot.cc#... src/snapshot/mksnapshot.cc:64: void WriteFilePrefix() const { why not const? Also, could these be static now? https://codereview.chromium.org/2767903002/diff/1/src/snapshot/mksnapshot.cc File src/snapshot/mksnapshot.cc (right): https://codereview.chromium.org/2767903002/diff/1/src/snapshot/mksnapshot.cc#... src/snapshot/mksnapshot.cc:40: void MaybeWriteStartupBlob(const i::Vector<const i::byte>& blob) { if this function fails, then we still have the snapshot cpp file -- is this ok?
wiktorg@google.com changed reviewers: + wiktorg@google.com
LGTM https://codereview.chromium.org/2767903002/diff/1/src/snapshot/mksnapshot.cc File src/snapshot/mksnapshot.cc (right): https://codereview.chromium.org/2767903002/diff/1/src/snapshot/mksnapshot.cc#... src/snapshot/mksnapshot.cc:50: fclose(fp); nit but shouldn't it also go before remove @ line 47
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2767903002/diff/1/src/snapshot/mksnapshot.cc File src/snapshot/mksnapshot.cc (left): https://codereview.chromium.org/2767903002/diff/1/src/snapshot/mksnapshot.cc#... src/snapshot/mksnapshot.cc:64: void WriteFilePrefix() const { On 2017/03/22 10:58:21, Leszek Swirski wrote: > why not const? Also, could these be static now? I just didn't see much use in const here. Made them static instead. https://codereview.chromium.org/2767903002/diff/1/src/snapshot/mksnapshot.cc File src/snapshot/mksnapshot.cc (right): https://codereview.chromium.org/2767903002/diff/1/src/snapshot/mksnapshot.cc#... src/snapshot/mksnapshot.cc:40: void MaybeWriteStartupBlob(const i::Vector<const i::byte>& blob) { On 2017/03/22 10:58:21, Leszek Swirski wrote: > if this function fails, then we still have the snapshot cpp file -- is this ok? That's correct. But I don't think this will happen a lot. The problem this CL is solving is when running V8 to create a snapshot fails, and we still end up creating these files. https://codereview.chromium.org/2767903002/diff/1/src/snapshot/mksnapshot.cc#... src/snapshot/mksnapshot.cc:50: fclose(fp); On 2017/03/22 11:01:15, wiktorg wrote: > nit but shouldn't it also go before remove @ line 47 Done.
The CQ bit was checked by yangguo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2767903002/diff/1/src/snapshot/mksnapshot.cc File src/snapshot/mksnapshot.cc (right): https://codereview.chromium.org/2767903002/diff/1/src/snapshot/mksnapshot.cc#... src/snapshot/mksnapshot.cc:40: void MaybeWriteStartupBlob(const i::Vector<const i::byte>& blob) { On 2017/03/22 11:41:42, Yang wrote: > On 2017/03/22 10:58:21, Leszek Swirski wrote: > > if this function fails, then we still have the snapshot cpp file -- is this > ok? > > That's correct. But I don't think this will happen a lot. The problem this CL is > solving is when running V8 to create a snapshot fails, and we still end up > creating these files. SGTM, maybe add a TODO for this rare error case since ideally we would make this entirely atomic.
The CQ bit was checked by yangguo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wiktorg@google.com, leszeks@chromium.org Link to the patchset: https://codereview.chromium.org/2767903002/#ps40001 (title: "add comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490183512663380,
"parent_rev": "86c2db5e33ccbff8acfa6c076a86b64c0b291675", "commit_rev":
"0b90e985f736cae797b23969e6e53e303f018236"}
Message was sent while issue was closed.
Description was changed from ========== [snapshot] only create snapshot files as last step in mksnapshot. R=leszeks@chromium.org BUG=chromium:633159 ========== to ========== [snapshot] only create snapshot files as last step in mksnapshot. R=leszeks@chromium.org BUG=chromium:633159 Review-Url: https://codereview.chromium.org/2767903002 Cr-Commit-Position: refs/heads/master@{#44015} Committed: https://chromium.googlesource.com/v8/v8/+/0b90e985f736cae797b23969e6e53e303f0... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/v8/v8/+/0b90e985f736cae797b23969e6e53e303f0... |
