All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=679530

Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?keys=agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDggKrSpAoM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICg_9OFqAoM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDggLCp6woM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICg_465ugsM


Bot(s) for this bug's original alert(s):

chromium-rel-win7-dual
chromium-rel-win7-gpu-intel
chromium-rel-win7-x64-dual
chromium-rel-win8-dual

Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8990899991787166784

=== Auto-CCing suspected CL author a.suchit@samsung.com ===

Hi a.suchit@samsung.com, the bisect results pointed to your CL, please take a look at the
results.


=== BISECT JOB RESULTS ===
Perf regression found with culprit

Suspected Commit
  Author : a.suchit
  Commit : 1060f916d9f7026fa22dd3bb6b563294ec7f8224
  Date   : Thu Jan 05 10:50:37 2017
  Subject: Made varied number of cells in each row based on row's requirement.

Bisect Details
  Configuration: win_perf_bisect
  Benchmark    : blink_perf.paint_slimmingpaintinvalidation
  Metric       : large-table-background-change-with-invisible-collapsed-borders/large-table-background-change-with-invisible-collapsed-borders
  Change       : 6.13% | 88.9228333333 -> 94.3751666667

Revision             Result                  N
chromium@441614      88.9228 +- 3.33535      30      good
chromium@441617      90.1455 +- 5.08564      30      good
chromium@441619      89.3483 +- 1.69578      30      good
chromium@441620      93.8033 +- 2.517        30      bad       <--
chromium@441625      94.3752 +- 3.26265      30      bad

To Run This Test
  src/tools/perf/run_benchmark -v --browser=release --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests blink_perf.paint_slimmingpaintinvalidation

Debug Info
  https://chromeperf.appspot.com/buildbucket_job_status/8990899991787166784

Is this bisect wrong?
  https://chromeperf.appspot.com/bad_bisect?try_job_id=5876825361743872


| O O | Visit http://www.chromium.org/developers/speed-infra/perf-bug-faq
|  X  | for more information addressing perf regression bugs. For feedback,
| / \ | file a bug with component Tests>AutoBisect.  Thank you!

I do not have idea how to proceed with this regression.

IMO, this should not give any performance regression because this have very small change.

This regression may be coming because of cells are not created for a row in one shot but those are created based on need.
Earlier, All rows would be having same number of cells counts (non-empty or empty).

If you can confirm that the CL indeed causes the regression, and we have no idea how to fix it, we should consider reverting the CL.

Yeah, if you try referting the CL and re-running the regression or running a bisect to see if this CL is, in fact, causing a regression, then we can talk about what to do to fix it.

Before reverting the patch :
avg 1634.4242499999998 ms
median 1634.1450000000114 ms
stdev 3.3581641581604456 ms
min 1628.50499999999 ms
max 1641.2799999999843 ms

After reverting the patch :
avg 1535.5769999999995 ms
median 1533.8524999999936 ms
stdev 4.992153949212605 ms
min 1528.5449999999983 ms
max 1545.9199999999983 ms

As per my understanding that ./third_party/WebKit/PerformanceTests/Paint/large-table-background-change-with-invisible-collapsed-borders.html is just checking rendering time and out change is for layout part.
It should not really effect the rendering performance.
Many places, we are used the numCols() as part for for loop conditional check. which may reduce the performance in paint files because it is accessing the very big vector table again and again in every iteration of the loop.
I will just try out and check if it is the real problem.

Result is nearly same if tried to remove the numCols() at many for loops. :(

Does anyone have idea why this patch may affect the performance ?

wangxianzhu: Do you know who owns blink_perf.paint_slimmingpaintinvalidation and could help a.suchit understand the performance impact of the patch?

The test is mainly for measuring paint invalidation and paint performance, but as it measures times of the whole rendering cycles, it also covers style and layout stages. However, the test changes background only, so there should be no layout, so I guess this is still a paint invalidation and/or paint performance regression. We also call some methods of LayoutObjects during paint invalidation and paint, so changes to LayoutObjects may also affect paint invalidation and paint performance.

Note that though this bug was filed for blink_perf.paint_slimmingpaintinvalidation only, the regression also occurred for blink_perf.paint (e.g. https://chromeperf.appspot.com/report?sid=4e11d33b064791d79f67a4d1a6390a4822d0a935af889f21f1b2e0213256a45a). blink_perf.paint_slimmingpaintinvalidation has been removed, but we still need to address the regression of blink_perf.paint.

I think a partial revert might be helpful. For example, just revert the change in TableSectionPainter and see the result.

Test results might be overwhelmed by noise when run locally. You can try the CL with numCols() extracted from the loops on the perf try bots to see the result. You can upload the CL then run 'tools/perf/run_benchmark trybot all_win blink_perf.paint'.

I pushed the patch https://codereview.chromium.org/2630723002/.
Started the trybot for all-win but trybot duplicates jobs are keep adding again an again on the patch.

The duplicated builder issue is bug 679620, but it should not affect the result.
Fwiw, you can view the result with the following steps:

1. Click one of the *_perf_bisect boxes;
2. Find the "Results" step and click 'stdio' link below it; (There should be a "HTML Results" link but some bug makes it disappear.)
3. Find "View HTML results online at " in the page, and open the URL after it.

It seems that the patch doesn't improve performance. I guess the change in LayoutTableSection::primaryCell in the original CL affected performance because it was added a condition. Can you try removing the condition in primaryCell temporarily to see the result?

It seems that I do not have access for stdio part in "Results" step. (https://build.chromium.org/p/tryserver.chromium.perf/builders/win_perf_bisect/builds/7101/steps/Results/logs/stdio)

I am getting error that This site can not be reached.
I tried with all my accounts : a.suchit@chromium.org, a.suchit@samsung.com, suchit.agrawal@gmail.com

I am trying some other options also.

Not sure if the stdio access is because of permission of temporary server issue. I also encounter issue accessing stdio sometimes.

I checked results on several bots and the results show improvement about 5% without the condition in primaryCell.

I think we need to extract numCols() calls from inner loops, including the one in primaryCell(). Perhaps we can have two versions of primaryCell(), or require all the caller to check numCols() before calling primaryCell().

I am not getting how can we have 2 versions of primary cells because checking is required in function primaryCellAt OR before calling of primaryCellAt(), both will give the same impact on the performance.

Major impact is coming from the file TableSectionPainter.cpp because this file have many calls of primaryCellAt().

We can only do numCols() out of the inner loop. Changes for primaryCellAt() (As suggested above by you) would not improve the performance.

I think your experiments have proved that the condition in the primaryCellAt() affected performance.

I meant to convert some code like:

  for (row: rows) {
    for (col: cols) {
      if (auto* cell = section.primaryCellAt(row, col)) 
        doSomthing(cell);
    }
  }

to
  for (row: rows) {
    unsigned numCols = section.numCols(row);
    for (col: 0..numCols) {
      if (auto* coll = section.primaryCellAt(row, col))
        doSomething(cell);
    }
  }

or (e.g. for some loops in TableSectionPainter):
  for (row: rows) {
    unsigned numCols = section.numCols(row);
    for (col: cols) {
      if (col < numCols) {
        if (auto* coll = section.primaryCellAt(row, col))
          doSomething(cell);
      }
    }
  }

In this way we extract the numCols() from primaryCellAt() out of the inner loops. We would require all callers of primaryCellAt() to check for validity of col before calling.

Patch is updated https://codereview.chromium.org/2630723002/.

@ wangxianzhu, Can you please update the improvement in performance in final patch set...

Thanks

The results shows various improvement, between 0.n% - 7%, for the test.

There is a good news: Now the "HTML Results" link is back in the perf bisect results, and it's public accessible, after bug 681047 was fixed.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3173daa005795ac280b23e2f89c612f0d93892fb

commit 3173daa005795ac280b23e2f89c612f0d93892fb
Author: a.suchit <a.suchit@samsung.com>
Date: Thu Feb 02 17:28:11 2017

Fix blink_perf.paint regression about tables

Performance was getting down because vector is accessing very frequently
which
becomes very costly when vector size is very big.

PrimaryCellAt() is using very frequently and it was accessed same vector twice (first
time used by numCols() and another one directly.). So now vector is accessed
once and same reference is used for both statements/operations.
numCols() is accessing the vector for getting the vector size and it
was used
in the loop iteration. So now it calls only once before the loop.

BUG= 679530 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2630723002
Cr-Commit-Position: refs/heads/master@{#447786}

[modify] https://crrev.com/3173daa005795ac280b23e2f89c612f0d93892fb/third_party/WebKit/Source/core/layout/LayoutTable.cpp
[modify] https://crrev.com/3173daa005795ac280b23e2f89c612f0d93892fb/third_party/WebKit/Source/core/layout/LayoutTableSection.cpp
[modify] https://crrev.com/3173daa005795ac280b23e2f89c612f0d93892fb/third_party/WebKit/Source/core/layout/LayoutTableSection.h
[modify] https://crrev.com/3173daa005795ac280b23e2f89c612f0d93892fb/third_party/WebKit/Source/core/paint/TableSectionPainter.cpp
[modify] https://crrev.com/3173daa005795ac280b23e2f89c612f0d93892fb/third_party/WebKit/Source/modules/accessibility/AXTable.cpp