New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 618679 link

Starred by 2 users

Issue metadata

Status: WontFix
Owner:
Last visit > 30 days ago
Closed: Jul 2016
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

12.8%-40.4% regression in blink_perf.svg at 398015:398050

Project Member Reported by pmeenan@chromium.org, Jun 9 2016

Issue description

See the link to graphs below.
 
All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=618679

Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?keys=agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICgvNaPoQkM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICgvOWmsAoM


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

chromium-rel-win7-dual
chromium-rel-win7-single
Project Member

Comment 3 by 42576172...@developer.gserviceaccount.com, Jun 13 2016

Cc: maksim.s...@intel.com
Owner: maksim.s...@intel.com

=== Auto-CCing suspected CL author maksim.sisov@intel.com ===

Hi maksim.sisov@intel.com, the bisect results pointed to your CL below as possibly
causing a regression. Please have a look at this info and see whether
your CL be related.


===== BISECT JOB RESULTS =====
Status: completed


===== SUSPECTED CL(s) =====
Subject : Making cookies eviction quotas match spec
Author  : maksim.sisov
Commit description:
  
According to the section 3 and section 5 in
https://tools.ietf.org/html/draft-west-cookie-priority-
00#section-3, the old cookie monster implementation doesn't
maintain priority quotas correctly.

This CL makes sure there will always be at least 30 low
priority cookies, 50 mid priority and 70 high priority
cookies if there had been enough of them before  the eviction.
Please note, secure cookies are more important than non-secure
per https://tools.ietf.org/html/draft-west-leave-secure-cookies-alone.

A small example: if there are 70 cookies:
37 non-secure low priority and 33 secure low priority
cookies, 37 non-secure cookies are deleted during the first
round and other 3 secure low priority cookies are deleted
during the following round preserving 30 low priority
cookies according to the quota of those specific cookies.
For a bigger example that fully complies with the
implementation, check the unittests.

Before the fix, the unittests were just adjusted to the
behavior of cookies eviction.

For example, if we take the following unittest:
Round 1 => 10L; round 2 => 11M, 10L; round 3 => none.
TestPriorityCookieCase(cm.get(), "11HN 10MN 20LN 110MN 20LN
10HN", 20U,109U, 21U, 150U, 0U);
The problem here was that there were only 40 low priority
cookies, but the quota was not preserved for those cookies.
First, 10 low priority cookies were deleted and then more 10
low priority cookies were deleted leaving only 20 of them,
which was less than the quota (30 low priority cookies).
It happened because the eviction algorithm didn't know how
many cookies of a specific priority were deleted and
it had always started to delete all the cookies from the
beginning of the container removing even those cookies
that shouldn't have been deleted.

After we land this CL, we can have cookies in any order and
high priority cookies will be eventually deleted in order
to avoid excess of high priority cookies by some
applications within the same domain. Thus, after the
eviction algorithm runs, we should have at least 30 low, 50
mid and 70 high priority cookies if we had sufficient
amount of them in the beginning.

BUG= 609550 

Review-Url: https://codereview.chromium.org/1976073002
Cr-Commit-Position: refs/heads/master@{#398049}
Commit  : fe1fea6df040fff38a3a4567e20a3d5847e83170
Date    : Mon Jun 06 16:53:15 2016


===== TESTED REVISIONS =====
Revision         Mean     Std Dev   N   Good?
chromium@398014  245.96   1.72984   12  good
chromium@398032  244.764  0.767073  12  good
chromium@398041  245.417  1.18264   18  good
chromium@398046  245.383  1.2459    18  good
chromium@398048  245.336  1.46856   27  good
chromium@398049  246.23   1.38827   27  bad    <--
chromium@398050  247.208  1.57811   27  bad

Bisect job ran on: win_perf_bisect
Bug ID: 618679

Test Command: src/tools/perf/run_benchmark -v --browser=release --output-format=chartjson --upload-results --also-run-disabled-tests blink_perf.svg
Test Metric: Cowboy_transform/Cowboy_transform
Relative Change: 0.70%
Score: 99.9

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/win_perf_bisect/builds/6591
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9009962602721934944


Not what you expected? We'll investigate and get back to you!
  https://chromeperf.appspot.com/bad_bisect?try_job_id=5257476841144320

| 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!

Comment 4 by mmenke@chromium.org, Jun 14 2016

Cc: mmenke@chromium.org
The results of that bisect job look bogus - on the bot, we see an increase in time the test takes of over 10%, in the bisect bots, we see a difference in 0.5%.  Moreover, a change in the cookie eviction logic shouldn't affect a random SVG test that doesn't appear to even set any cookies.  I'm going to run another bisect, see if we can figure out what the problem really is here.
Project Member

Comment 5 by 42576172...@developer.gserviceaccount.com, Jun 14 2016


===== BISECT JOB RESULTS =====
Status: completed


===== SUSPECTED CL(s) =====
Subject : Making cookies eviction quotas match spec
Author  : maksim.sisov
Commit description:
  
According to the section 3 and section 5 in
https://tools.ietf.org/html/draft-west-cookie-priority-
00#section-3, the old cookie monster implementation doesn't
maintain priority quotas correctly.

This CL makes sure there will always be at least 30 low
priority cookies, 50 mid priority and 70 high priority
cookies if there had been enough of them before  the eviction.
Please note, secure cookies are more important than non-secure
per https://tools.ietf.org/html/draft-west-leave-secure-cookies-alone.

A small example: if there are 70 cookies:
37 non-secure low priority and 33 secure low priority
cookies, 37 non-secure cookies are deleted during the first
round and other 3 secure low priority cookies are deleted
during the following round preserving 30 low priority
cookies according to the quota of those specific cookies.
For a bigger example that fully complies with the
implementation, check the unittests.

Before the fix, the unittests were just adjusted to the
behavior of cookies eviction.

For example, if we take the following unittest:
Round 1 => 10L; round 2 => 11M, 10L; round 3 => none.
TestPriorityCookieCase(cm.get(), "11HN 10MN 20LN 110MN 20LN
10HN", 20U,109U, 21U, 150U, 0U);
The problem here was that there were only 40 low priority
cookies, but the quota was not preserved for those cookies.
First, 10 low priority cookies were deleted and then more 10
low priority cookies were deleted leaving only 20 of them,
which was less than the quota (30 low priority cookies).
It happened because the eviction algorithm didn't know how
many cookies of a specific priority were deleted and
it had always started to delete all the cookies from the
beginning of the container removing even those cookies
that shouldn't have been deleted.

After we land this CL, we can have cookies in any order and
high priority cookies will be eventually deleted in order
to avoid excess of high priority cookies by some
applications within the same domain. Thus, after the
eviction algorithm runs, we should have at least 30 low, 50
mid and 70 high priority cookies if we had sufficient
amount of them in the beginning.

BUG= 609550 

Review-Url: https://codereview.chromium.org/1976073002
Cr-Commit-Position: refs/heads/master@{#398049}
Commit  : fe1fea6df040fff38a3a4567e20a3d5847e83170
Date    : Mon Jun 06 16:53:15 2016


===== TESTED REVISIONS =====
Revision         Mean     Std Dev  N   Good?
chromium@398014  245.402  1.66663  18  good
chromium@398032  245.459  1.4398   18  good
chromium@398041  245.514  1.54037  18  good
chromium@398046  245.542  1.4928   27  good
chromium@398048  245.626  1.37871  41  good
chromium@398049  246.419  1.44259  41  bad    <--
chromium@398050  247.09   1.69621  41  bad

Bisect job ran on: win_perf_bisect
Bug ID: 618679

Test Command: src/tools/perf/run_benchmark -v --browser=release --output-format=chartjson --upload-results --also-run-disabled-tests blink_perf.svg
Test Metric: Cowboy_transform/Cowboy_transform
Relative Change: 0.42%
Score: 99.9

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/win_perf_bisect/builds/6593
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9009868295374426976


Not what you expected? We'll investigate and get back to you!
  https://chromeperf.appspot.com/bad_bisect?try_job_id=5782496998850560

| 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!
Project Member

Comment 6 by 42576172...@developer.gserviceaccount.com, Jun 15 2016


===== BISECT JOB RESULTS =====
Status: completed


===== SUSPECTED CL(s) =====
Subject : Making cookies eviction quotas match spec
Author  : maksim.sisov
Commit description:
  
According to the section 3 and section 5 in
https://tools.ietf.org/html/draft-west-cookie-priority-
00#section-3, the old cookie monster implementation doesn't
maintain priority quotas correctly.

This CL makes sure there will always be at least 30 low
priority cookies, 50 mid priority and 70 high priority
cookies if there had been enough of them before  the eviction.
Please note, secure cookies are more important than non-secure
per https://tools.ietf.org/html/draft-west-leave-secure-cookies-alone.

A small example: if there are 70 cookies:
37 non-secure low priority and 33 secure low priority
cookies, 37 non-secure cookies are deleted during the first
round and other 3 secure low priority cookies are deleted
during the following round preserving 30 low priority
cookies according to the quota of those specific cookies.
For a bigger example that fully complies with the
implementation, check the unittests.

Before the fix, the unittests were just adjusted to the
behavior of cookies eviction.

For example, if we take the following unittest:
Round 1 => 10L; round 2 => 11M, 10L; round 3 => none.
TestPriorityCookieCase(cm.get(), "11HN 10MN 20LN 110MN 20LN
10HN", 20U,109U, 21U, 150U, 0U);
The problem here was that there were only 40 low priority
cookies, but the quota was not preserved for those cookies.
First, 10 low priority cookies were deleted and then more 10
low priority cookies were deleted leaving only 20 of them,
which was less than the quota (30 low priority cookies).
It happened because the eviction algorithm didn't know how
many cookies of a specific priority were deleted and
it had always started to delete all the cookies from the
beginning of the container removing even those cookies
that shouldn't have been deleted.

After we land this CL, we can have cookies in any order and
high priority cookies will be eventually deleted in order
to avoid excess of high priority cookies by some
applications within the same domain. Thus, after the
eviction algorithm runs, we should have at least 30 low, 50
mid and 70 high priority cookies if we had sufficient
amount of them in the beginning.

BUG= 609550 

Review-Url: https://codereview.chromium.org/1976073002
Cr-Commit-Position: refs/heads/master@{#398049}
Commit  : fe1fea6df040fff38a3a4567e20a3d5847e83170
Date    : Mon Jun 06 16:53:15 2016


===== TESTED REVISIONS =====
Revision         Mean     Std Dev   N  Good?
chromium@398014  70.8434  0.652636  5  good
chromium@398032  70.1804  0.20227   5  good
chromium@398041  71.191   0.429549  5  good
chromium@398046  71.292   0.235165  5  good
chromium@398048  70.6748  0.695314  5  good
chromium@398049  78.813   0.382975  5  bad    <--
chromium@398050  78.5362  0.527054  5  bad

Bisect job ran on: win_perf_bisect
Bug ID: 618679

Test Command: src/tools/perf/run_benchmark -v --browser=release --output-format=chartjson --upload-results --also-run-disabled-tests blink_perf.svg
Test Metric: SvgHitTesting/SvgHitTesting
Relative Change: 10.86%
Score: 99.9

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/win_perf_bisect/builds/6596
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9009772617798253552


Not what you expected? We'll investigate and get back to you!
  https://chromeperf.appspot.com/bad_bisect?try_job_id=5776136420720640

| 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!
Project Member

Comment 7 by 42576172...@developer.gserviceaccount.com, Jun 15 2016


===== BISECT JOB RESULTS =====
Status: completed


===== SUSPECTED CL(s) =====
Subject : Making cookies eviction quotas match spec
Author  : maksim.sisov
Commit description:
  
According to the section 3 and section 5 in
https://tools.ietf.org/html/draft-west-cookie-priority-
00#section-3, the old cookie monster implementation doesn't
maintain priority quotas correctly.

This CL makes sure there will always be at least 30 low
priority cookies, 50 mid priority and 70 high priority
cookies if there had been enough of them before  the eviction.
Please note, secure cookies are more important than non-secure
per https://tools.ietf.org/html/draft-west-leave-secure-cookies-alone.

A small example: if there are 70 cookies:
37 non-secure low priority and 33 secure low priority
cookies, 37 non-secure cookies are deleted during the first
round and other 3 secure low priority cookies are deleted
during the following round preserving 30 low priority
cookies according to the quota of those specific cookies.
For a bigger example that fully complies with the
implementation, check the unittests.

Before the fix, the unittests were just adjusted to the
behavior of cookies eviction.

For example, if we take the following unittest:
Round 1 => 10L; round 2 => 11M, 10L; round 3 => none.
TestPriorityCookieCase(cm.get(), "11HN 10MN 20LN 110MN 20LN
10HN", 20U,109U, 21U, 150U, 0U);
The problem here was that there were only 40 low priority
cookies, but the quota was not preserved for those cookies.
First, 10 low priority cookies were deleted and then more 10
low priority cookies were deleted leaving only 20 of them,
which was less than the quota (30 low priority cookies).
It happened because the eviction algorithm didn't know how
many cookies of a specific priority were deleted and
it had always started to delete all the cookies from the
beginning of the container removing even those cookies
that shouldn't have been deleted.

After we land this CL, we can have cookies in any order and
high priority cookies will be eventually deleted in order
to avoid excess of high priority cookies by some
applications within the same domain. Thus, after the
eviction algorithm runs, we should have at least 30 low, 50
mid and 70 high priority cookies if we had sufficient
amount of them in the beginning.

BUG= 609550 

Review-Url: https://codereview.chromium.org/1976073002
Cr-Commit-Position: refs/heads/master@{#398049}
Commit  : fe1fea6df040fff38a3a4567e20a3d5847e83170
Date    : Mon Jun 06 16:53:15 2016


===== TESTED REVISIONS =====
Revision         Mean     Std Dev   N  Good?
chromium@398014  71.2734  1.0176    5  good
chromium@398032  69.905   0.529892  5  good
chromium@398041  70.9662  0.317326  5  good
chromium@398046  71.2004  0.908346  5  good
chromium@398048  70.2486  0.309871  5  good
chromium@398049  79.0814  0.2657    5  bad    <--
chromium@398050  79.2076  0.493024  5  bad

Bisect job ran on: win_perf_bisect
Bug ID: 618679

Test Command: src/tools/perf/run_benchmark -v --browser=release --output-format=chartjson --upload-results --also-run-disabled-tests blink_perf.svg
Test Metric: SvgHitTesting/SvgHitTesting
Relative Change: 11.13%
Score: 99.9

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/win_perf_bisect/builds/6597
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9009772511166455312


Not what you expected? We'll investigate and get back to you!
  https://chromeperf.appspot.com/bad_bisect?try_job_id=5881951274139648

| 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!

Comment 8 by mmenke@chromium.org, Jun 15 2016

Ok, those two bisects look much more convincing to me.

maksim:  I'm going to revert the CL.  If the regression goes away, we can investigate and reland.  If the perf regression is sill there, I'll just reland.
Ok, it might be due to the while loop in the CL. The previous implementation was a bit faster, but not fully compliant with the specs.
Matt, have you tried to run bisects after the patch has been reverted?
Labels: -performance-sheriff Performance-Sheriff
The graphs for the perf tests are here: https://chromeperf.appspot.com/group_report?bug_id=618679

Has it already been reverted?  I don't see the graphs recovering but it's possible they haven't run a cycle since the revert happened.
I reverted in position 398049, so we've had two runs with the revert, and neither has showed any recovery.  Weird that 4 bisects all blamed the CL.  Maybe some sort of code location memory, and just growing binary sized move things around the wrong way?  Anyhow, I'll plan to reland either this evening or tomorrow, just to give the per bots a few more cycles, out of paranoia.
Owner: ----
Status: Untriaged (was: Assigned)
Given that the revert didn't help, unassigning from maksim, and I'm going to reland the CL.
Owner: pmeenan@chromium.org
Taking ownership back to see if I can manually identify a CL in the range or an infrastructure change that may have caused it.
Owner: lanwei@chromium.org
Status: Assigned (was: Untriaged)
lanwei@ - I think this is probably related to your fix for the fact that the pointer events on windows were previously hard-coded to be mouse.  I kicked a bisect against the SVG Hit test regression but would you mind taking a look?

Given that it is fixing previously-incorrect behavior the regression is fine if there isn't a more performant way to do the check.
pmeenan@
My code is simple which is basically just one line (GetMessageExtraInfo() & SIGNATURE_MASK) != MOUSEEVENTF_FROMTOUCHPEN.

I do not think it will cause a performance regression and we do need this change to set the pointer type?

Can you please do another bisect, thanks?
Project Member

Comment 17 by sheriffbot@chromium.org, Jul 6 2016

Labels: -M-53 M-54 MovedFrom-53
Moving this nonessential bug to the next milestone.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Cc: -maksim.s...@intel.com -mmenke@chromium.org
Owner: pmeenan@chromium.org
Status: WontFix (was: Assigned)
Closing out as a likely infrastructure change.  It isn't bisecting well and the change logs aren't showing anything that could impact it (except possibly the switch to GN on the bots)

Sign in to add a comment