Remove snapshot log parsing and option from tools.

Remove snapshot log parsing and option from tools.

LOG=N

Committed: https://crrev.com/9e39a9fff1c2966a3f650a4c31dbbe533886d614
Cr-Commit-Position: refs/heads/master@{#35268}

[Stefano Sanfilippo, 2016-03-14 12:39:09]

ssanfilippo@chromium.org changed reviewers:
+ yangguo@chromium.org

[Yang, 2016-03-15 10:13:19]

Some places that you might have missed:
- tools/profviz/worker.js uses the tick processor
- test/mjsunit/tools/tickprocessor.js

https://codereview.chromium.org/1796863002/diff/1/tools/ll_prof.py
File tools/ll_prof.py (right):

https://codereview.chromium.org/1796863002/diff/1/tools/ll_prof.py#newcode340
tools/ll_prof.py:340: _SNAPSHOT_POSITION_TAG = "P"
This is no longer necessary?

https://codereview.chromium.org/1796863002/diff/1/tools/ll_prof.py#newcode426
tools/ll_prof.py:426: if tag == LogReader._SNAPSHOT_POSITION_TAG:
This whole if-block could be removed?

[Stefano Sanfilippo, 2016-03-16 10:28:10]

On 2016/03/15 10:13:19, Yang wrote:
> Some places that you might have missed:
> - tools/profviz/worker.js uses the tick processor
> - test/mjsunit/tools/tickprocessor.js

Thanks, I'll give it a look in a second.

> https://codereview.chromium.org/1796863002/diff/1/tools/ll_prof.py
> File tools/ll_prof.py (right):
> 
> https://codereview.chromium.org/1796863002/diff/1/tools/ll_prof.py#newcode340
> tools/ll_prof.py:340: _SNAPSHOT_POSITION_TAG = "P"
> This is no longer necessary?

Unfortunately no, the ll log still contains snapshot position serialized
structs, identified by this tag.

> https://codereview.chromium.org/1796863002/diff/1/tools/ll_prof.py#newcode426
> tools/ll_prof.py:426: if tag == LogReader._SNAPSHOT_POSITION_TAG:
> This whole if-block could be removed?

See above. Whether we skip or parse snapshot positions, we need to keep this
part.

--S

[Yang, 2016-03-17 11:40:56]

On 2016/03/16 10:28:10, Stefano Sanfilippo wrote:
> On 2016/03/15 10:13:19, Yang wrote:
> > Some places that you might have missed:
> > - tools/profviz/worker.js uses the tick processor
> > - test/mjsunit/tools/tickprocessor.js
> 
> Thanks, I'll give it a look in a second.
> 
> > https://codereview.chromium.org/1796863002/diff/1/tools/ll_prof.py
> > File tools/ll_prof.py (right):
> > 
> >
https://codereview.chromium.org/1796863002/diff/1/tools/ll_prof.py#newcode340
> > tools/ll_prof.py:340: _SNAPSHOT_POSITION_TAG = "P"
> > This is no longer necessary?
> 
> Unfortunately no, the ll log still contains snapshot position serialized
> structs, identified by this tag.
> 
> >
https://codereview.chromium.org/1796863002/diff/1/tools/ll_prof.py#newcode426
> > tools/ll_prof.py:426: if tag == LogReader._SNAPSHOT_POSITION_TAG:
> > This whole if-block could be removed?
> 
> See above. Whether we skip or parse snapshot positions, we need to keep this
> part.
> 
> --S

We can probably also drop the snapshot position events from the
serializer/deserializer.

[Yang, 2016-03-29 12:32:18]

On 2016/03/17 11:40:56, Yang wrote:
> On 2016/03/16 10:28:10, Stefano Sanfilippo wrote:
> > On 2016/03/15 10:13:19, Yang wrote:
> > > Some places that you might have missed:
> > > - tools/profviz/worker.js uses the tick processor
> > > - test/mjsunit/tools/tickprocessor.js
> > 
> > Thanks, I'll give it a look in a second.
> > 
> > > https://codereview.chromium.org/1796863002/diff/1/tools/ll_prof.py
> > > File tools/ll_prof.py (right):
> > > 
> > >
> https://codereview.chromium.org/1796863002/diff/1/tools/ll_prof.py#newcode340
> > > tools/ll_prof.py:340: _SNAPSHOT_POSITION_TAG = "P"
> > > This is no longer necessary?
> > 
> > Unfortunately no, the ll log still contains snapshot position serialized
> > structs, identified by this tag.
> > 
> > >
> https://codereview.chromium.org/1796863002/diff/1/tools/ll_prof.py#newcode426
> > > tools/ll_prof.py:426: if tag == LogReader._SNAPSHOT_POSITION_TAG:
> > > This whole if-block could be removed?
> > 
> > See above. Whether we skip or parse snapshot positions, we need to keep this
> > part.
> > 
> > --S
> 
> We can probably also drop the snapshot position events from the
> serializer/deserializer.

what's the status of this?

[Stefano Sanfilippo, 2016-04-04 14:51:56]

The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-04 14:52:07]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1796863002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1796863002/20001

[commit-bot: I haz the power, 2016-04-04 15:06:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-04 15:06:09]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel/builds/...)

[Stefano Sanfilippo, 2016-04-04 15:59:31]

The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-04 15:59:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1796863002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1796863002/40001

[commit-bot: I haz the power, 2016-04-04 16:19:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-04 16:19:23]

Dry run: This issue passed the CQ dry run.

[Stefano Sanfilippo, 2016-04-04 16:22:30]

Hopefully done with the scrapping, PTAL.

https://codereview.chromium.org/1796863002/diff/1/tools/ll_prof.py
File tools/ll_prof.py (right):

https://codereview.chromium.org/1796863002/diff/1/tools/ll_prof.py#newcode340
tools/ll_prof.py:340: _SNAPSHOT_POSITION_TAG = "P"
On 2016/03/15 10:13:19, Yang wrote:
> This is no longer necessary?

Removed, together with all the related code.

https://codereview.chromium.org/1796863002/diff/1/tools/ll_prof.py#newcode426
tools/ll_prof.py:426: if tag == LogReader._SNAPSHOT_POSITION_TAG:
On 2016/03/15 10:13:19, Yang wrote:
> This whole if-block could be removed?

Done.

[Yang, 2016-04-05 12:37:13]

LGTM! With one small left over in src/snapshot/mksnapshot.ccv

https://codereview.chromium.org/1796863002/diff/40001/BUILD.gn
File BUILD.gn (left):

https://codereview.chromium.org/1796863002/diff/40001/BUILD.gn#oldcode514
BUILD.gn:514: rebase_path("$target_gen_dir/snapshot.log", root_build_dir),
In src/snapshot/mksnapshot.cc, at the very start of main(), we set two flags for
logging. These should become unnecessary as well.

[Stefano Sanfilippo, 2016-04-05 14:06:09]

PTAL

https://codereview.chromium.org/1796863002/diff/40001/BUILD.gn
File BUILD.gn (left):

https://codereview.chromium.org/1796863002/diff/40001/BUILD.gn#oldcode514
BUILD.gn:514: rebase_path("$target_gen_dir/snapshot.log", root_build_dir),
On 2016/04/05 12:37:13, Yang wrote:
> In src/snapshot/mksnapshot.cc, at the very start of main(), we set two flags
for
> logging. These should become unnecessary as well.

Done.

[Stefano Sanfilippo, 2016-04-05 14:11:57]

The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-05 14:12:00]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1796863002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1796863002/60001

[Yang, 2016-04-05 14:28:39]

On 2016/04/05 14:12:00, commit-bot: I haz the power wrote:
> Dry run: CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1796863002/60001
> View timeline at
>  https://chromium-cq-status.appspot.com/patch-timeline/1796863002/60001

lgtm

[commit-bot: I haz the power, 2016-04-05 14:44:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-05 14:44:13]

Dry run: This issue passed the CQ dry run.

[Stefano Sanfilippo, 2016-04-05 14:51:57]

The CQ bit was checked by ssanfilippo@chromium.org

[commit-bot: I haz the power, 2016-04-05 14:52:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1796863002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1796863002/60001

[commit-bot: I haz the power, 2016-04-05 15:01:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-05 15:01:34]

Try jobs failed on following builders:
  v8_presubmit on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/13233)

[Stefano Sanfilippo, 2016-04-05 15:05:24]

The CQ bit was checked by ssanfilippo@chromium.org

[Stefano Sanfilippo, 2016-04-05 15:05:24]

The patchset sent to the CQ was uploaded after l-g-t-m from yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/1796863002/#ps80001
(title: "Rebase on master.")

[commit-bot: I haz the power, 2016-04-05 15:05:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1796863002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1796863002/80001

[commit-bot: I haz the power, 2016-04-05 15:29:59]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-04-05 15:31:44]

Description was changed from

==========
Remove snapshot log parsing and option from tools.

LOG=N
==========

to

==========
Remove snapshot log parsing and option from tools.

LOG=N

Committed: https://crrev.com/9e39a9fff1c2966a3f650a4c31dbbe533886d614
Cr-Commit-Position: refs/heads/master@{#35268}
==========

[commit-bot: I haz the power, 2016-04-05 15:31:45]

Patchset 5 (id:??) landed as
https://crrev.com/9e39a9fff1c2966a3f650a4c31dbbe533886d614
Cr-Commit-Position: refs/heads/master@{#35268}

[Benedikt Meurer, 2016-04-06 09:04:09]

bmeurer@chromium.org changed reviewers:
+ bmeurer@chromium.org

[Benedikt Meurer, 2016-04-06 09:04:10]

This breaks --disasm-top with ll_prof. I can no longer see the ticks in the
disassembly. Please fix that or revert this CL.

[Stefano Sanfilippo, 2016-04-06 11:10:51]

On 2016/04/06 09:04:10, Benedikt Meurer wrote:
> This breaks --disasm-top with ll_prof. I can no longer see the ticks in the
> disassembly. Please fix that or revert this CL.

Sorry about that, I am investigating the issue right now.

[jochen (gone - plz use gerrit), 2016-04-06 12:36:18]

On 2016/04/06 at 11:10:51, ssanfilippo wrote:
> On 2016/04/06 09:04:10, Benedikt Meurer wrote:
> > This breaks --disasm-top with ll_prof. I can no longer see the ticks in the
> > disassembly. Please fix that or revert this CL.
> 
> Sorry about that, I am investigating the issue right now.

this also breaks node.js:
https://build.chromium.org/p/client.v8.fyi/builders/V8%20-%20node.js%20integr...

[Stefano Sanfilippo, 2016-04-06 13:35:02]

On 2016/04/06 12:36:18, jochen - slow wrote:
> On 2016/04/06 at 11:10:51, ssanfilippo wrote:
> > On 2016/04/06 09:04:10, Benedikt Meurer wrote:
> > > This breaks --disasm-top with ll_prof. I can no longer see the ticks in
the
> > > disassembly. Please fix that or revert this CL.
> > 
> > Sorry about that, I am investigating the issue right now.
> 
> this also breaks node.js:
>
https://build.chromium.org/p/client.v8.fyi/builders/V8%20-%20node.js%20integr...

I might very well be wrong in reading the table, but isn't it the case that the
first failing build precedes this commit?

[Stefano Sanfilippo, 2016-04-06 14:26:46]

Description was changed from

==========
Remove snapshot log parsing and option from tools.

LOG=N

Committed: https://crrev.com/9e39a9fff1c2966a3f650a4c31dbbe533886d614
Cr-Commit-Position: refs/heads/master@{#35268}
==========

to

==========
Remove snapshot log parsing and option from tools.

LOG=N

Committed: https://crrev.com/9e39a9fff1c2966a3f650a4c31dbbe533886d614
Cr-Commit-Position: refs/heads/master@{#35268}
==========

[Stefano Sanfilippo, 2016-04-06 14:27:04]

On 2016/04/06 13:35:02, Stefano Sanfilippo wrote:
> On 2016/04/06 12:36:18, jochen - slow wrote:
> > On 2016/04/06 at 11:10:51, ssanfilippo wrote:
> > > On 2016/04/06 09:04:10, Benedikt Meurer wrote:
> > > > This breaks --disasm-top with ll_prof. I can no longer see the ticks in
> the
> > > > disassembly. Please fix that or revert this CL.
> > > 
> > > Sorry about that, I am investigating the issue right now.
> > 
> > this also breaks node.js:
> >
>
https://build.chromium.org/p/client.v8.fyi/builders/V8%20-%20node.js%20integr...
> 
> I might very well be wrong in reading the table, but isn't it the case that
the
> first failing build precedes this commit?

Nevermind, I found the build you were referring to.

[Stefano Sanfilippo, 2016-04-06 15:42:17]

On 2016/04/06 14:27:04, Stefano Sanfilippo wrote:
> On 2016/04/06 13:35:02, Stefano Sanfilippo wrote:
> > On 2016/04/06 12:36:18, jochen - slow wrote:
> > > On 2016/04/06 at 11:10:51, ssanfilippo wrote:
> > > > On 2016/04/06 09:04:10, Benedikt Meurer wrote:
> > > > > This breaks --disasm-top with ll_prof. I can no longer see the ticks
in
> > the
> > > > > disassembly. Please fix that or revert this CL.
> > > > 
> > > > Sorry about that, I am investigating the issue right now.
> > > 
> > > this also breaks node.js:
> > >
> >
>
https://build.chromium.org/p/client.v8.fyi/builders/V8%20-%20node.js%20integr...
> > 
> > I might very well be wrong in reading the table, but isn't it the case that
> the
> > first failing build precedes this commit?
> 
> Nevermind, I found the build you were referring to.

This should fix it https://codereview.chromium.org/1869453002/