New issue
Advanced search Search tips

Issue 704857 link

Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Improve performance of chromium_build_stat/ninja_log

Project Member Reported by tikuta@chromium.org, Mar 24 2017

Issue description

It should be good to write down the current number and your environment.
This will help you to prove how much you improve the trace.

Comment 2 by tikuta@chromium.org, Mar 27 2017

I will add benchmark for ninja_log and track it in Z840 Linux.
https://chromium-review.googlesource.com/c/459359/

Project Member

Comment 3 by bugdroid1@chromium.org, Mar 28 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/infra/+/c81534acdb218daa5c6b55ea17fb1a66e5373c3a

commit c81534acdb218daa5c6b55ea17fb1a66e5373c3a
Author: Takuto Ikuta <tikuta@google.com>
Date: Tue Mar 28 01:53:11 2017

Add cpu profile option and benchmark test for ninja_log

I want to get trace faster.
Currently, it takes 6.27s to get trace.html.
http://chromium-build-stats.appspot.com//ninja_log/upload/ninja_log.gwHlABebteF0GHy46SVgskAtYFZvhqvIfulY7VCdaSQ=.gz/trace.html

As a first step, I added cpuprofile option and benchmark test for ninja_log

# Benchmark result from goapp in Z840 Linux
$ ../../../goenv.sh goapp test -bench . -benchmem
PASS
BenchmarkParse-48            	    3000	    429390 ns/op	  131047 B/op	     666 allocs/op
BenchmarkDedup-48            	   10000	    110511 ns/op	   52977 B/op	      11 allocs/op
BenchmarkFlow-48             	   10000	    202809 ns/op	   42384 B/op	     261 allocs/op
BenchmarkToTraces-48         	   10000	    138063 ns/op	   60593 B/op	     260 allocs/op
BenchmarkDedupFlowToTraces-48	    2000	    819714 ns/op	  252440 B/op	    1196 allocs/op
ok  	ninjalog	7.663s

Bug:704857

Change-Id: I9e13de61835636946e4917eaae21721b88208e04
Reviewed-on: https://chromium-review.googlesource.com/459359
Commit-Queue: Takuto Ikuta <tikuta@google.com>
Reviewed-by: Fumitoshi Ukai <ukai@chromium.org>
Reviewed-by: Yoshisato Yanagisawa <yyanagisawa@chromium.org>
Reviewed-by: Shinya Kawanaka <shinyak@chromium.org>

[modify] https://crrev.com/c81534acdb218daa5c6b55ea17fb1a66e5373c3a/appengine/chromium_build_stats/cmd/ninja_log_trace_viewer/ninja_log_trace_viewer.go
[add] https://crrev.com/c81534acdb218daa5c6b55ea17fb1a66e5373c3a/appengine/chromium_build_stats/gopath/src/ninjalog/testdata/ninja_log
[modify] https://crrev.com/c81534acdb218daa5c6b55ea17fb1a66e5373c3a/appengine/chromium_build_stats/gopath/src/ninjalog/ninjalog_test.go

Project Member

Comment 4 by bugdroid1@chromium.org, Mar 28 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/infra/+/4c1b1d2ec087b36310b22a8b607c553473ae7dbb

commit 4c1b1d2ec087b36310b22a8b607c553473ae7dbb
Author: Takuto Ikuta <tikuta@google.com>
Date: Tue Mar 28 05:42:54 2017

Make slice using known capacity in ToTraces

I extracted code for ToTraces from 
https://chromium-review.googlesource.com/c/461597/

benchmark                         old ns/op     new ns/op     delta
BenchmarkParse-48                 422498        429327        +1.62%
BenchmarkDedup-48                 105125        109552        +4.21%
BenchmarkFlow-48                  183793        203844        +10.91%
BenchmarkToTraces-48              142831        117758        -17.55%
BenchmarkDedupFlowToTraces-48     866417        776622        -10.36%

benchmark                         old allocs     new allocs     delta
BenchmarkParse-48                 666            666            +0.00%
BenchmarkDedup-48                 11             11             +0.00%
BenchmarkFlow-48                  261            261            +0.00%
BenchmarkToTraces-48              260            252            -3.08%
BenchmarkDedupFlowToTraces-48     1196           1188           -0.67%

benchmark                         old bytes     new bytes     delta
BenchmarkParse-48                 131048        131047        -0.00%
BenchmarkDedup-48                 52977         52977         +0.00%
BenchmarkFlow-48                  42384         42384         +0.00%
BenchmarkToTraces-48              60593         36608         -39.58%
BenchmarkDedupFlowToTraces-48     252440        228453        -9.50%

Bug:704857

Change-Id: I72d45826518835b01ab30ffaf6448abd3062a1ed
Reviewed-on: https://chromium-review.googlesource.com/461677
Reviewed-by: Fumitoshi Ukai <ukai@chromium.org>
Commit-Queue: Takuto Ikuta <tikuta@google.com>

[modify] https://crrev.com/4c1b1d2ec087b36310b22a8b607c553473ae7dbb/appengine/chromium_build_stats/gopath/src/ninjalog/trace.go

Project Member

Comment 5 by bugdroid1@chromium.org, Mar 28 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/infra/+/4c1b1d2ec087b36310b22a8b607c553473ae7dbb

commit 4c1b1d2ec087b36310b22a8b607c553473ae7dbb
Author: Takuto Ikuta <tikuta@google.com>
Date: Tue Mar 28 05:42:54 2017

Make slice using known capacity in ToTraces

I extracted code for ToTraces from 
https://chromium-review.googlesource.com/c/461597/

benchmark                         old ns/op     new ns/op     delta
BenchmarkParse-48                 422498        429327        +1.62%
BenchmarkDedup-48                 105125        109552        +4.21%
BenchmarkFlow-48                  183793        203844        +10.91%
BenchmarkToTraces-48              142831        117758        -17.55%
BenchmarkDedupFlowToTraces-48     866417        776622        -10.36%

benchmark                         old allocs     new allocs     delta
BenchmarkParse-48                 666            666            +0.00%
BenchmarkDedup-48                 11             11             +0.00%
BenchmarkFlow-48                  261            261            +0.00%
BenchmarkToTraces-48              260            252            -3.08%
BenchmarkDedupFlowToTraces-48     1196           1188           -0.67%

benchmark                         old bytes     new bytes     delta
BenchmarkParse-48                 131048        131047        -0.00%
BenchmarkDedup-48                 52977         52977         +0.00%
BenchmarkFlow-48                  42384         42384         +0.00%
BenchmarkToTraces-48              60593         36608         -39.58%
BenchmarkDedupFlowToTraces-48     252440        228453        -9.50%

Bug:704857

Change-Id: I72d45826518835b01ab30ffaf6448abd3062a1ed
Reviewed-on: https://chromium-review.googlesource.com/461677
Reviewed-by: Fumitoshi Ukai <ukai@chromium.org>
Commit-Queue: Takuto Ikuta <tikuta@google.com>

[modify] https://crrev.com/4c1b1d2ec087b36310b22a8b607c553473ae7dbb/appengine/chromium_build_stats/gopath/src/ninjalog/trace.go

Project Member

Comment 6 by bugdroid1@chromium.org, Mar 28 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/infra/+/54271eb2cb88d0b8cefeb7129f0530ff652717a0

commit 54271eb2cb88d0b8cefeb7129f0530ff652717a0
Author: Takuto Ikuta <tikuta@google.com>
Date: Tue Mar 28 07:43:25 2017

Optimize lineToStep

* Stop to use strings.Split
* Use strconv.ParseUint instead of strconv.Atoi
  because Atoi is 2 times slower than ParseUint.

benchmark                         old ns/op     new ns/op     delta
BenchmarkParse-48                 390960        257516        -34.13%
BenchmarkDedup-48                 103194        105843        +2.57%
BenchmarkFlow-48                  193974        185199        -4.52%
BenchmarkToTraces-48              109839        101698        -7.41%
BenchmarkDedupFlowToTraces-48     792036        606747        -23.39%

benchmark                         old allocs     new allocs     delta
BenchmarkParse-48                 666            341            -48.80%
BenchmarkDedup-48                 11             11             +0.00%
BenchmarkFlow-48                  261            261            +0.00%
BenchmarkToTraces-48              252            252            +0.00%
BenchmarkDedupFlowToTraces-48     1188           863            -27.36%

benchmark                         old bytes     new bytes     delta
BenchmarkParse-48                 131047        105039        -19.85%
BenchmarkDedup-48                 52977         52977         +0.00%
BenchmarkFlow-48                  42384         42384         +0.00%
BenchmarkToTraces-48              36608         36608         +0.00%
BenchmarkDedupFlowToTraces-48     228453        202447        -11.38%

Bug:704857

Change-Id: I2a16c562e6c53a575e3c8a3d035f5682ffa5a0b9
Reviewed-on: https://chromium-review.googlesource.com/461698
Commit-Queue: Takuto Ikuta <tikuta@google.com>
Reviewed-by: Yoshisato Yanagisawa <yyanagisawa@chromium.org>
Reviewed-by: Fumitoshi Ukai <ukai@chromium.org>
Reviewed-by: Shinya Kawanaka <shinyak@chromium.org>

[modify] https://crrev.com/54271eb2cb88d0b8cefeb7129f0530ff652717a0/appengine/chromium_build_stats/gopath/src/ninjalog/ninjalog.go

Comment 7 by tikuta@chromium.org, Mar 28 2017

I deploy current master.

Comment 8 by tikuta@chromium.org, Mar 28 2017

Hmm, looks not improved for the page.
http://chromium-build-stats.appspot.com//ninja_log/upload/ninja_log.gwHlABebteF0GHy46SVgskAtYFZvhqvIfulY7VCdaSQ=.gz/trace.html

From app engine log, application itself took 4 seconds.
Need optimization for more larger test case?

Project Member

Comment 9 by bugdroid1@chromium.org, Mar 28 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/infra/+/9d9b92fbe3dad46be81c525bf2204483bc6e3f0b

commit 9d9b92fbe3dad46be81c525bf2204483bc6e3f0b
Author: Takuto Ikuta <tikuta@google.com>
Date: Tue Mar 28 09:31:29 2017

Upgrade instance from F1 to F2 for chromium_build_stats

Refereces
* https://cloud.google.com/appengine/docs/about-the-standard-environment#instance_classes
* https://cloud.google.com/appengine/docs/standard/go/config/appref

Bug: 704857
Change-Id: Ia3c1d1c6c155af6a4f42d4ec17c36f9064799cb4
Reviewed-on: https://chromium-review.googlesource.com/461578
Reviewed-by: Yoshisato Yanagisawa <yyanagisawa@chromium.org>
Reviewed-by: Shinya Kawanaka <shinyak@chromium.org>
Commit-Queue: Takuto Ikuta <tikuta@google.com>

[modify] https://crrev.com/9d9b92fbe3dad46be81c525bf2204483bc6e3f0b/appengine/chromium_build_stats/default/app.yaml

Upgrading instance looks not improve much.
Labels: -Pri-2 Pri-3
Status: Available (was: Started)

Comment 12 by ukai@chromium.org, Mar 29 2017

> Upgrading instance looks not improve much.

better to revert?


Project Member

Comment 13 by bugdroid1@chromium.org, Mar 29 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/infra/+/2502f07e6bdfd0fb0aadf28a946aa4526738eb3a

commit 2502f07e6bdfd0fb0aadf28a946aa4526738eb3a
Author: Takuto Ikuta <tikuta@google.com>
Date: Wed Mar 29 01:36:49 2017

Revert "Upgrade instance from F1 to F2 for chromium_build_stats"

This reverts commit 9d9b92fbe3dad46be81c525bf2204483bc6e3f0b.

Reason for revert: instance upgrade did not improved performance much.

Original change's description:
> Upgrade instance from F1 to F2 for chromium_build_stats
> 
> Refereces
> * https://cloud.google.com/appengine/docs/about-the-standard-environment#instance_classes
> * https://cloud.google.com/appengine/docs/standard/go/config/appref
> 
> Bug: 704857
> Change-Id: Ia3c1d1c6c155af6a4f42d4ec17c36f9064799cb4
> Reviewed-on: https://chromium-review.googlesource.com/461578
> Reviewed-by: Yoshisato Yanagisawa <yyanagisawa@chromium.org>
> Reviewed-by: Shinya Kawanaka <shinyak@chromium.org>
> Commit-Queue: Takuto Ikuta <tikuta@google.com>
> 

TBR=ukai@chromium.org,yyanagisawa@chromium.org,shinyak@chromium.org,tikuta@google.com,chromium-reviews@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true

Change-Id: Ice4beb06b14f42a62ab7049f40657ab116c03176
Reviewed-on: https://chromium-review.googlesource.com/461600
Reviewed-by: Takuto Ikuta <tikuta@google.com>
Commit-Queue: Takuto Ikuta <tikuta@google.com>

[modify] https://crrev.com/2502f07e6bdfd0fb0aadf28a946aa4526738eb3a/appengine/chromium_build_stats/default/app.yaml

If it's CPU bound, not sure why it didn't improve the performance.
It might be not CPU bound, or, F1 and F2 are not so different under CPU burst.



Project Member

Comment 15 by bugdroid1@chromium.org, Jul 4 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/infra/+/b44a974ed303ac934ec855fe12c5db32bad462aa

commit b44a974ed303ac934ec855fe12c5db32bad462aa
Author: Takuto Ikuta <tikuta@google.com>
Date: Tue Jul 04 05:17:25 2017

Update chromium_build_stats to go1.8

Go1.8 is available now.
https://cloud.google.com/appengine/docs/standard/go/release-notes

Motivation for this change is performance improvement on go's library, especially on strings.
https://golang.org/doc/go1.7#performance
https://golang.org/doc/go1.8#performance

Also I want to control runtime version explicitly.

Bug: 704857
Change-Id: I59e66dba3a9a4335523e2f4307be87104258dc69
Reviewed-on: https://chromium-review.googlesource.com/558598
Commit-Queue: Takuto Ikuta <tikuta@google.com>
Reviewed-by: Shinya Kawanaka <shinyak@chromium.org>
Reviewed-by: Yoshisato Yanagisawa <yyanagisawa@chromium.org>

[modify] https://crrev.com/b44a974ed303ac934ec855fe12c5db32bad462aa/appengine/chromium_build_stats/default/app.yaml

Project Member

Comment 16 by sheriffbot@chromium.org, Jul 4

Labels: Hotlist-Recharge-Cold
Status: Untriaged (was: Available)
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Status: Assigned (was: Untriaged)
This bug has an owner, thus, it's been triaged. Changing status to "assigned".

Sign in to add a comment