Issue metadata
Sign in to add a comment
|
12.8%-40.4% regression in blink_perf.svg at 398015:398050 |
||||||||||||||||||
Issue descriptionSee the link to graphs below.
,
Jun 13 2016
,
Jun 13 2016
=== 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!
,
Jun 14 2016
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.
,
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!
,
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!
,
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!
,
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.
,
Jun 15 2016
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.
,
Jun 16 2016
Matt, have you tried to run bisects after the patch has been reverted?
,
Jun 16 2016
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.
,
Jun 16 2016
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.
,
Jun 17 2016
Given that the revert didn't help, unassigning from maksim, and I'm going to reland the CL.
,
Jun 17 2016
Taking ownership back to see if I can manually identify a CL in the range or an infrastructure change that may have caused it.
,
Jun 27 2016
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.
,
Jun 28 2016
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?
,
Jul 6 2016
Moving this nonessential bug to the next milestone. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jul 6 2016
,
Jul 6 2016
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 |
|||||||||||||||||||
Comment 1 by pmeenan@chromium.org
, Jun 9 2016