Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1833643002:
[Interpreter] Remove separate Ignition snapshot. (Closed)
Description[Interpreter] Remove separate Ignition snapshot.
Removes the seperate Ignition snapshot and build the Ignition bytecode
handlers in the default snapshot.
BUG=v8:4280
LOG=N
Committed: https://crrev.com/1798f3fe84faff32ba44e09f6aed79245dd98d80
Cr-Commit-Position: refs/heads/master@{#35058}
Committed: https://crrev.com/1b9066fdf1b40e77cd5edd4d09ed456ece5e25d5
Cr-Commit-Position: refs/heads/master@{#35359}
Patch Set 1 #
Total comments: 4
Patch Set 2 : unconditionally initialize interpreter #Patch Set 3 : Rebase and only build handlers when building the snapshot or if --ignition is passed #Patch Set 4 : Add back interpreter initialization to tests #
Depends on Patchset: Messages
Total messages: 44 (20 generated)
The CQ bit was checked by rmcilroy@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/1833643002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1833643002/1
rmcilroy@chromium.org changed reviewers: + machenbach@google.com
Michael, please take a look, thanks. This is essentially a revert of your previous two CLs to create the extra snapshot, and a modification of run_mksnapshot to pass --ignition so that the default snapshot includes it.
machenbach@chromium.org changed reviewers: + machenbach@chromium.org, mstarzinger@chromium.org, yangguo@chromium.org
lgtm on the infra parts - added yangguo and mstarzinger to judge if passing --ignition to the default snapshot will do the right thing. https://codereview.chromium.org/1833643002/diff/1/tools/gyp/v8.gyp File tools/gyp/v8.gyp (right): https://codereview.chromium.org/1833643002/diff/1/tools/gyp/v8.gyp#newcode203 tools/gyp/v8.gyp:203: '--ignition', Does that mean we'll ship this in our current production snapshot?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_arm64_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel/builds/1...)
https://codereview.chromium.org/1833643002/diff/1/tools/gyp/v8.gyp File tools/gyp/v8.gyp (right): https://codereview.chromium.org/1833643002/diff/1/tools/gyp/v8.gyp#newcode203 tools/gyp/v8.gyp:203: '--ignition', On 2016/03/24 11:29:32, Michael Achenbach wrote: > Does that mean we'll ship this in our current production snapshot? Why not just call Interpreter::Initialize() unconditionally in Isolate::Init?
https://codereview.chromium.org/1833643002/diff/1/tools/gyp/v8.gyp File tools/gyp/v8.gyp (right): https://codereview.chromium.org/1833643002/diff/1/tools/gyp/v8.gyp#newcode203 tools/gyp/v8.gyp:203: '--ignition', On 2016/03/24 11:36:25, Yang wrote: > On 2016/03/24 11:29:32, Michael Achenbach wrote: > > Does that mean we'll ship this in our current production snapshot? > > Why not just call Interpreter::Initialize() unconditionally in Isolate::Init? Good point, done.
Description was changed from ========== [Interpreter] Remove seperate Ignition snapshot. Removes the seperate Ignition snapshot and build the Ignition bytecode handlers in the default snapshot. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Remove separate Ignition snapshot. Removes the seperate Ignition snapshot and build the Ignition bytecode handlers in the default snapshot. BUG=v8:4280 LOG=N ==========
The CQ bit was checked by rmcilroy@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/1833643002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1833643002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM (on everything outside the build files). Thanks! https://codereview.chromium.org/1833643002/diff/1/tools/gyp/v8.gyp File tools/gyp/v8.gyp (right): https://codereview.chromium.org/1833643002/diff/1/tools/gyp/v8.gyp#newcode203 tools/gyp/v8.gyp:203: '--ignition', On 2016/03/24 12:10:38, rmcilroy wrote: > On 2016/03/24 11:36:25, Yang wrote: > > On 2016/03/24 11:29:32, Michael Achenbach wrote: > > > Does that mean we'll ship this in our current production snapshot? > > > > Why not just call Interpreter::Initialize() unconditionally in Isolate::Init? > > Good point, done. +1, I also prefer the new approach. Thanks for changing!
On 2016/03/24 13:19:25, Michael Starzinger wrote: > LGTM (on everything outside the build files). Thanks! > > https://codereview.chromium.org/1833643002/diff/1/tools/gyp/v8.gyp > File tools/gyp/v8.gyp (right): > > https://codereview.chromium.org/1833643002/diff/1/tools/gyp/v8.gyp#newcode203 > tools/gyp/v8.gyp:203: '--ignition', > On 2016/03/24 12:10:38, rmcilroy wrote: > > On 2016/03/24 11:36:25, Yang wrote: > > > On 2016/03/24 11:29:32, Michael Achenbach wrote: > > > > Does that mean we'll ship this in our current production snapshot? > > > > > > Why not just call Interpreter::Initialize() unconditionally in > Isolate::Init? > > > > Good point, done. > > +1, I also prefer the new approach. Thanks for changing! lgtm.
The CQ bit was checked by rmcilroy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from machenbach@chromium.org Link to the patchset: https://codereview.chromium.org/1833643002/#ps20001 (title: "unconditionally initialize interpreter")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1833643002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1833643002/20001
Message was sent while issue was closed.
Description was changed from ========== [Interpreter] Remove separate Ignition snapshot. Removes the seperate Ignition snapshot and build the Ignition bytecode handlers in the default snapshot. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Remove separate Ignition snapshot. Removes the seperate Ignition snapshot and build the Ignition bytecode handlers in the default snapshot. BUG=v8:4280 LOG=N ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [Interpreter] Remove separate Ignition snapshot. Removes the seperate Ignition snapshot and build the Ignition bytecode handlers in the default snapshot. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Remove separate Ignition snapshot. Removes the seperate Ignition snapshot and build the Ignition bytecode handlers in the default snapshot. BUG=v8:4280 LOG=N Committed: https://crrev.com/1798f3fe84faff32ba44e09f6aed79245dd98d80 Cr-Commit-Position: refs/heads/master@{#35058} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/1798f3fe84faff32ba44e09f6aed79245dd98d80 Cr-Commit-Position: refs/heads/master@{#35058}
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/1827143002/ by rmcilroy@chromium.org. The reason for reverting is: Makes nosnap bots timeout due to having to rebuild bytecode handlers..
Message was sent while issue was closed.
Description was changed from ========== [Interpreter] Remove separate Ignition snapshot. Removes the seperate Ignition snapshot and build the Ignition bytecode handlers in the default snapshot. BUG=v8:4280 LOG=N Committed: https://crrev.com/1798f3fe84faff32ba44e09f6aed79245dd98d80 Cr-Commit-Position: refs/heads/master@{#35058} ========== to ========== [Interpreter] Remove separate Ignition snapshot. Removes the seperate Ignition snapshot and build the Ignition bytecode handlers in the default snapshot. BUG=v8:4280 LOG=N Committed: https://crrev.com/1798f3fe84faff32ba44e09f6aed79245dd98d80 Cr-Commit-Position: refs/heads/master@{#35058} ==========
Relanding with a tweak to ensure handlers aren't built in nosnap builds unless --ignition is passed. Hopefully this should address the timeout issues.
The timeouts on the bots are also increased by now...
On 2016/04/08 12:01:49, Michael Achenbach wrote: > The timeouts on the bots are also increased by now... Great, thanks.
The CQ bit was checked by rmcilroy@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/1833643002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1833643002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rmcilroy@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/1833643002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1833643002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rmcilroy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yangguo@chromium.org, mstarzinger@chromium.org, machenbach@chromium.org Link to the patchset: https://codereview.chromium.org/1833643002/#ps60001 (title: "Add back interpreter initialization to tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1833643002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1833643002/60001
Message was sent while issue was closed.
Description was changed from ========== [Interpreter] Remove separate Ignition snapshot. Removes the seperate Ignition snapshot and build the Ignition bytecode handlers in the default snapshot. BUG=v8:4280 LOG=N Committed: https://crrev.com/1798f3fe84faff32ba44e09f6aed79245dd98d80 Cr-Commit-Position: refs/heads/master@{#35058} ========== to ========== [Interpreter] Remove separate Ignition snapshot. Removes the seperate Ignition snapshot and build the Ignition bytecode handlers in the default snapshot. BUG=v8:4280 LOG=N Committed: https://crrev.com/1798f3fe84faff32ba44e09f6aed79245dd98d80 Cr-Commit-Position: refs/heads/master@{#35058} ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [Interpreter] Remove separate Ignition snapshot. Removes the seperate Ignition snapshot and build the Ignition bytecode handlers in the default snapshot. BUG=v8:4280 LOG=N Committed: https://crrev.com/1798f3fe84faff32ba44e09f6aed79245dd98d80 Cr-Commit-Position: refs/heads/master@{#35058} ========== to ========== [Interpreter] Remove separate Ignition snapshot. Removes the seperate Ignition snapshot and build the Ignition bytecode handlers in the default snapshot. BUG=v8:4280 LOG=N Committed: https://crrev.com/1798f3fe84faff32ba44e09f6aed79245dd98d80 Cr-Commit-Position: refs/heads/master@{#35058} Committed: https://crrev.com/1b9066fdf1b40e77cd5edd4d09ed456ece5e25d5 Cr-Commit-Position: refs/heads/master@{#35359} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/1b9066fdf1b40e77cd5edd4d09ed456ece5e25d5 Cr-Commit-Position: refs/heads/master@{#35359} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
