Ensure CSS scanner does not mark preloads as referenced

Ensure CSS scanner does not mark preloads as referenced

A recent metrics fix (r432864) also mistakenly broke this code by not
passing in a default argument. This CL fixes that problem and adds an
expectation in unit tests.

BUG=662999, 596676
Committed: https://crrev.com/8b58b651ee02a1ec7edde005e6d8a27cca9bae31
Cr-Commit-Position: refs/heads/master@{#435643}

[Charlie Harrison, 2016-12-01 16:25:14]

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

[commit-bot: I haz the power, 2016-12-01 16:25:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Harrison, 2016-12-01 16:31:33]

csharrison@chromium.org changed reviewers:
+ yoav@yoav.ws

[Charlie Harrison, 2016-12-01 16:31:33]

Hi Yoav, PTAL?

It is unfortunate that this error happened as a result of improperly adding
default arguments (it is swaying me in favor of not using them :/).

Additionally, metrics started to stabilize and are showing that the experiment
is dropping the CSS misses substantially (~1/3). Strangely, it is not dropping
them to zero.

If this change (which I will likely try to merge to M56) ends up changing
metrics back to a regressing state, we will know a bit more about why this is
happening. Namely, that lifetime issues are not to blame and the blame is
squarely in the mark as reference / do not mark as reference code.

[Yoav Weiss, 2016-12-01 16:44:10]

On 2016/12/01 16:31:33, Charlie Harrison wrote:
> Hi Yoav, PTAL?
> 
> It is unfortunate that this error happened as a result of improperly adding
> default arguments (it is swaying me in favor of not using them :/).
> 
> Additionally, metrics started to stabilize and are showing that the experiment
> is dropping the CSS misses substantially (~1/3). Strangely, it is not dropping
> them to zero.
> 
> If this change (which I will likely try to merge to M56) ends up changing
> metrics back to a regressing state, we will know a bit more about why this is
> happening. Namely, that lifetime issues are not to blame and the blame is
> squarely in the mark as reference / do not mark as reference code.

LGTM. I should have caught that while reviewing the other CL :/
I agree that without default args this wouldn't have happened.

Seems to me like it's possible r432864 falsely dropped the metrics (by marking
some CSS preloads as referenced), and this fix here might just return them to
their "natural" state.
To me ,this still doesn't necessarily mean that the blame for the metrics skew
is on the MarkAsReferenced code.

[Charlie Harrison, 2016-12-01 17:23:09]

On 2016/12/01 16:44:10, Yoav Weiss wrote:
> On 2016/12/01 16:31:33, Charlie Harrison wrote:
> > Hi Yoav, PTAL?
> > 
> > It is unfortunate that this error happened as a result of improperly adding
> > default arguments (it is swaying me in favor of not using them :/).
> > 
> > Additionally, metrics started to stabilize and are showing that the
experiment
> > is dropping the CSS misses substantially (~1/3). Strangely, it is not
dropping
> > them to zero.
> > 
> > If this change (which I will likely try to merge to M56) ends up changing
> > metrics back to a regressing state, we will know a bit more about why this
is
> > happening. Namely, that lifetime issues are not to blame and the blame is
> > squarely in the mark as reference / do not mark as reference code.
> 
> LGTM. I should have caught that while reviewing the other CL :/
> I agree that without default args this wouldn't have happened.
No worries it is very tricky. Yeah I wonder if merging chromium style will
disfavor this in the future. It is very tempting in the short term to rely on
these for quick and simpler patches, but it is dangerous!

> 
> Seems to me like it's possible r432864 falsely dropped the metrics (by marking
> some CSS preloads as referenced), and this fix here might just return them to
> their "natural" state.
> To me ,this still doesn't necessarily mean that the blame for the metrics skew
> is on the MarkAsReferenced code.

Yeah true, but at least we'll know that lifetime issues aren't in play.

Thanks for the review!

[Charlie Harrison, 2016-12-01 17:23:12]

The CQ bit was unchecked by csharrison@chromium.org

[Charlie Harrison, 2016-12-01 17:23:12]

The CQ bit was checked by csharrison@chromium.org

[commit-bot: I haz the power, 2016-12-01 17:23:19]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-01 17:46:16]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1480612992671010, "parent_rev":
"06782f29f61db69171f57c3d3c2d8033c8efbaa8", "commit_rev":
"1820e4d5dfefa1696c0cad39b2cba6ca918b0ff4"}

[commit-bot: I haz the power, 2016-12-01 17:46:40]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-12-01 17:49:33]

Description was changed from

==========
Ensure CSS scanner does not mark preloads as referenced

A recent metrics fix (r432864) also mistakenly broke this code by not
passing in a default argument. This CL fixes that problem and adds an
expectation in unit tests.

BUG=662999,596676
==========

to

==========
Ensure CSS scanner does not mark preloads as referenced

A recent metrics fix (r432864) also mistakenly broke this code by not
passing in a default argument. This CL fixes that problem and adds an
expectation in unit tests.

BUG=662999,596676

Committed: https://crrev.com/8b58b651ee02a1ec7edde005e6d8a27cca9bae31
Cr-Commit-Position: refs/heads/master@{#435643}
==========

[commit-bot: I haz the power, 2016-12-01 17:49:34]

Patchset 1 (id:??) landed as
https://crrev.com/8b58b651ee02a1ec7edde005e6d8a27cca9bae31
Cr-Commit-Position: refs/heads/master@{#435643}

[Charlie Harrison, 2016-12-05 16:27:01]

I confirmed that this PS regresses the Miss metric again, so our hypothesis
about holding Weak members is incorrect, and I'm confident
https://codereview.chromium.org/2542183002/ is the true fix.