Refactor budget computation to be more tuneable.

Refactor budget computation to be more tuneable.

Previously, the budget computation used a single master value,
kBudgetDurationInHours to control both how long awards were valid and
also indirectly, how much budget an origin received per hour. The fixed
value in the system was that an origin received SES_score budget per
kBudgetDurationInHours hours.

The side effect of this indirect effect is that if chromium wants
for awards to last shorter periods of time, then the origin will get
larger grants of budget. So modifying the duration increases the amount
of work the origin can do, which is usually unintended.

A previous CL did exactly that, decreasing the duration and unintentionally
increasing the amount of work that can be done.

This CL modifies the calculation to use two values. The duration is still
there and is used to determine how much budget the origin has been granted
recently and how long new grants should last. A new value provides a target
budget for a maximally engaged origin, is used to provide a fixed upper
limit for budget granted to the origin. Finally, this CL reverses the earlier
unintentional increase in daily budget. Before this patch, full engagement
origins received 100 budget per 4 days, now they receive 48 per 4 days.

Tests were also updated to use the new computation.

BUG=617971
Review-Url: https://codereview.chromium.org/2620393002
Cr-Commit-Position: refs/heads/master@{#443874}
Committed: https://chromium.googlesource.com/chromium/src/+/ccdc3f3aef6ac41aba49b35a754e24961ace471a

[harkness, 2017-01-11 17:31:00]

harkness@chromium.org changed reviewers:
+ peter@chromium.org

[harkness, 2017-01-11 17:31:20]

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

[commit-bot: I haz the power, 2017-01-11 17:31:57]

Dry run: 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, 2017-01-11 18:23:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-11 18:23:31]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[harkness, 2017-01-12 14:24:11]

Modified browser tests for the new computation and added a call to floor() in
the Blink layer, so that we give developers integral values instead of the
internal doubles.

[Peter Beverloo, 2017-01-12 14:40:04]

https://codereview.chromium.org/2620393002/diff/40001/chrome/browser/budget_s...
File chrome/browser/budget_service/budget_database.cc (right):

https://codereview.chromium.org/2620393002/diff/40001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_database.cc:33: // This allows 6 silent
push messages a day for a maximal site.
Can we make this comment slightly clearer, since it assumes knowledge that the
cost of a silent push is 2.0, which is defined in another file?

// The amount of budget that a maximally engaged site should receive per hour.
// This value allows for up to 6 silent push messages, which have a cost of 2,
// per day for a fully engaged site.

(Even better if these values could be defined in the same file.)

https://codereview.chromium.org/2620393002/diff/40001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_database.cc:310: base::TimeDelta elapsed =
base::TimeDelta::FromDays(kBudgetDurationInDays);
nit: I think the "By default..." comment would still be very useful to explain
why we use kBudgetDurationInDays for uncached origins.

https://codereview.chromium.org/2620393002/diff/40001/chrome/browser/budget_s...
File chrome/browser/budget_service/budget_database_unittest.cc (right):

https://codereview.chromium.org/2620393002/diff/40001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_database_unittest.cc:8: 
style nit: no newline

https://codereview.chromium.org/2620393002/diff/40001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_database_unittest.cc:31: const double
kDefaultEngagement = 25;
Is this true? The amount of bootstrap points in SiteEngagementScore is set to
24.

Would be better to use SiteEngagementScore::GetBootstrapPoints().

https://codereview.chromium.org/2620393002/diff/40001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_database_unittest.cc:33: const double
kMaxSES = 100;
kMaxSES -> SiteEngagementScore::kMaxPoints

https://codereview.chromium.org/2620393002/diff/40001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_database_unittest.cc:33: const double
kMaxSES = 100;
constexpr

Would also be good to document whether these are duplicated values, and if they
are, with what.

https://codereview.chromium.org/2620393002/diff/40001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_database_unittest.cc:272:
EXPECT_EQ(floor(full_award), buckets[0].min);
Out of interest, why is this necessary?

https://codereview.chromium.org/2620393002/diff/40001/chrome/browser/budget_s...
File chrome/browser/budget_service/budget_manager_browsertest.cc (right):

https://codereview.chromium.org/2620393002/diff/40001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_manager_browsertest.cc:133: LoadTestPage();
 // Reload to build site engagement.
This will likely contribute to making this test flaky. Can we fake this somehow
without reloading the page?

https://codereview.chromium.org/2620393002/diff/40001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_manager_browsertest.cc:136: EXPECT_EQ("true
- is controlled", script_result);
This should be an ASSERT.

We use ASSERT_* when it's a requirement. If isControlled() says that the page is
*not* controlled, then workerGetBudget() won't be able to do anything. That
means it makes no sense to continue.

https://codereview.chromium.org/2620393002/diff/40001/chrome/browser/budget_s...
File chrome/browser/budget_service/budget_manager_unittest.cc (right):

https://codereview.chromium.org/2620393002/diff/40001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_manager_unittest.cc:162: EXPECT_EQ(12,
num_samples);
Please clarify in the description *how* this changes initial budget.

https://codereview.chromium.org/2620393002/diff/40001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_browsertest.cc (right):

https://codereview.chromium.org/2620393002/diff/40001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_browsertest.cc:1222: // cost 2
each.
I'd love to be able to use constants for this. Can we use something like
BudgetFoo::GetMaxSilentPushCost() or something like that?

(Doing this later is fine.)

https://codereview.chromium.org/2620393002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/budget/BudgetService.cpp (right):

https://codereview.chromium.org/2620393002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/budget/BudgetService.cpp:115: // Copy the
chunks into the budget array.
nit: Please document why we're flooring.

[harkness, 2017-01-12 17:39:22]

Description was changed from

==========
Refactor budget computation to be more tuneable.

Previously, the budget computation used a single master value,
kBudgetDurationInHours to control both how long awards were valid and
also indirectly, how much budget an origin received per hour. The fixed
value in the system was that an origin received SES_score budget per
kBudgetDurationInHours hours.

The side effect of this indirect effect is that if chromium wants
for awards to last shorter periods of time, then the origin will get
larger grants of budget. So modifying the duration increases the amount
of work the origin can do, which is usually unintended.

This CL modifies the calculation to use two values. The duration is still
there and is used to determine how much budget the origin has been granted
recently and how long new grants should last. A new value provides a target
budget for a maximally engaged origin, is used to provide a fixed upper
limit for budget granted to the origin.

Tests were also updated to use the new computation.

BUG=617971
==========

to

==========
Refactor budget computation to be more tuneable.

Previously, the budget computation used a single master value,
kBudgetDurationInHours to control both how long awards were valid and
also indirectly, how much budget an origin received per hour. The fixed
value in the system was that an origin received SES_score budget per
kBudgetDurationInHours hours.

The side effect of this indirect effect is that if chromium wants
for awards to last shorter periods of time, then the origin will get
larger grants of budget. So modifying the duration increases the amount
of work the origin can do, which is usually unintended.

A previous CL did exactly that, decreasing the duration and unintentionally
increasing the amount of work that can be done.

This CL modifies the calculation to use two values. The duration is still
there and is used to determine how much budget the origin has been granted
recently and how long new grants should last. A new value provides a target
budget for a maximally engaged origin, is used to provide a fixed upper
limit for budget granted to the origin. Finally, this CL reverses the earlier
unintentional increase in daily budget. Before this patch, full engagement
origins received 100 budget per 4 days, now they receive 48 per 4 days.

Tests were also updated to use the new computation.

BUG=617971
==========

[harkness, 2017-01-12 17:44:15]

https://codereview.chromium.org/2620393002/diff/40001/chrome/browser/budget_s...
File chrome/browser/budget_service/budget_database.cc (right):

https://codereview.chromium.org/2620393002/diff/40001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_database.cc:33: // This allows 6 silent
push messages a day for a maximal site.
On 2017/01/12 14:40:04, Peter Beverloo wrote:
> Can we make this comment slightly clearer, since it assumes knowledge that the
> cost of a silent push is 2.0, which is defined in another file?
> 
> // The amount of budget that a maximally engaged site should receive per hour.
> // This value allows for up to 6 silent push messages, which have a cost of 2,
> // per day for a fully engaged site.
> 
> (Even better if these values could be defined in the same file.)

I updated the comment here. I'd like to keep the cost values in BudgetManager,
since the database shouldn't have to know how much various actions cost.

https://codereview.chromium.org/2620393002/diff/40001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_database.cc:310: base::TimeDelta elapsed =
base::TimeDelta::FromDays(kBudgetDurationInDays);
On 2017/01/12 14:40:04, Peter Beverloo wrote:
> nit: I think the "By default..." comment would still be very useful to explain
> why we use kBudgetDurationInDays for uncached origins.

Done.

https://codereview.chromium.org/2620393002/diff/40001/chrome/browser/budget_s...
File chrome/browser/budget_service/budget_database_unittest.cc (right):

https://codereview.chromium.org/2620393002/diff/40001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_database_unittest.cc:8: 
On 2017/01/12 14:40:04, Peter Beverloo wrote:
> style nit: no newline

Done.

https://codereview.chromium.org/2620393002/diff/40001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_database_unittest.cc:31: const double
kDefaultEngagement = 25;
On 2017/01/12 14:40:04, Peter Beverloo wrote:
> Is this true? The amount of bootstrap points in SiteEngagementScore is set to
> 24.
> 
> Would be better to use SiteEngagementScore::GetBootstrapPoints().

That's just the value we set for the purposes of the test. I've renamed to
kEngagement.

https://codereview.chromium.org/2620393002/diff/40001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_database_unittest.cc:33: const double
kMaxSES = 100;
On 2017/01/12 14:40:04, Peter Beverloo wrote:
> constexpr
> 
> Would also be good to document whether these are duplicated values, and if
they
> are, with what.

Done.

https://codereview.chromium.org/2620393002/diff/40001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_database_unittest.cc:33: const double
kMaxSES = 100;
On 2017/01/12 14:40:04, Peter Beverloo wrote:
> kMaxSES -> SiteEngagementScore::kMaxPoints

Done.

https://codereview.chromium.org/2620393002/diff/40001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_database_unittest.cc:272:
EXPECT_EQ(floor(full_award), buckets[0].min);
On 2017/01/12 14:40:04, Peter Beverloo wrote:
> Out of interest, why is this necessary?

The buckets in the histograms have a min set to integral values, and the full
award might not be integral.

https://codereview.chromium.org/2620393002/diff/40001/chrome/browser/budget_s...
File chrome/browser/budget_service/budget_manager_browsertest.cc (right):

https://codereview.chromium.org/2620393002/diff/40001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_manager_browsertest.cc:133: LoadTestPage();
 // Reload to build site engagement.
On 2017/01/12 14:40:04, Peter Beverloo wrote:
> This will likely contribute to making this test flaky. Can we fake this
somehow
> without reloading the page?

I don't know of a way to do it. Browser tests are supposed to test the way a
test is used by a developer and there's no tool we have to bump up the site
engagement other than normal use. Is there a way to inject user input or playing
media into the test?

https://codereview.chromium.org/2620393002/diff/40001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_manager_browsertest.cc:136: EXPECT_EQ("true
- is controlled", script_result);
On 2017/01/12 14:40:04, Peter Beverloo wrote:
> This should be an ASSERT.
> 
> We use ASSERT_* when it's a requirement. If isControlled() says that the page
is
> *not* controlled, then workerGetBudget() won't be able to do anything. That
> means it makes no sense to continue.

Done.

https://codereview.chromium.org/2620393002/diff/40001/chrome/browser/budget_s...
File chrome/browser/budget_service/budget_manager_unittest.cc (right):

https://codereview.chromium.org/2620393002/diff/40001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_manager_unittest.cc:162: EXPECT_EQ(12,
num_samples);
On 2017/01/12 14:40:04, Peter Beverloo wrote:
> Please clarify in the description *how* this changes initial budget.

Done.

https://codereview.chromium.org/2620393002/diff/40001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_browsertest.cc (right):

https://codereview.chromium.org/2620393002/diff/40001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_browsertest.cc:1222: // cost 2
each.
On 2017/01/12 14:40:04, Peter Beverloo wrote:
> I'd love to be able to use constants for this. Can we use something like
> BudgetFoo::GetMaxSilentPushCost() or something like that?
> 
> (Doing this later is fine.)

I think that's a great idea, and I will absolutely do it later.

https://codereview.chromium.org/2620393002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/budget/BudgetService.cpp (right):

https://codereview.chromium.org/2620393002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/budget/BudgetService.cpp:115: // Copy the
chunks into the budget array.
On 2017/01/12 14:40:04, Peter Beverloo wrote:
> nit: Please document why we're flooring.

Done.

[Peter Beverloo, 2017-01-12 17:49:41]

lgtm

https://codereview.chromium.org/2620393002/diff/40001/chrome/browser/budget_s...
File chrome/browser/budget_service/budget_manager_browsertest.cc (right):

https://codereview.chromium.org/2620393002/diff/40001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_manager_browsertest.cc:133: LoadTestPage();
 // Reload to build site engagement.
On 2017/01/12 17:44:15, harkness wrote:
> On 2017/01/12 14:40:04, Peter Beverloo wrote:
> > This will likely contribute to making this test flaky. Can we fake this
> somehow
> > without reloading the page?
> 
> I don't know of a way to do it. Browser tests are supposed to test the way a
> test is used by a developer and there's no tool we have to bump up the site
> engagement other than normal use. Is there a way to inject user input or
playing
> media into the test?

Probably, but that will also need a fair bit of plumbing of its own. I was
hoping that we might be able to use SiteEngagementScore::SiteEngagementService()
here.

https://codereview.chromium.org/2620393002/diff/60001/chrome/browser/budget_s...
File chrome/browser/budget_service/budget_database_unittest.cc (right):

https://codereview.chromium.org/2620393002/diff/60001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_database_unittest.cc:35: const double
kMaxSES = SiteEngagementScore::kMaxPoints;
micro nit: just refer SES::kMaxPoints instead of copying?

https://codereview.chromium.org/2620393002/diff/60001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_database_unittest.cc:288: 2 * cost *
kMaxSES / kDefaultExpirationInDays / kMaxDailyBudget;
nit: for future changes, while the precedence rules are clear, this would read
much easier by adding a bunch of parenthesis

[harkness, 2017-01-13 11:54:45]

https://codereview.chromium.org/2620393002/diff/40001/chrome/browser/budget_s...
File chrome/browser/budget_service/budget_manager_browsertest.cc (right):

https://codereview.chromium.org/2620393002/diff/40001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_manager_browsertest.cc:133: LoadTestPage();
 // Reload to build site engagement.
On 2017/01/12 17:49:41, Peter Beverloo (OOO- Jan 19th) wrote:
> On 2017/01/12 17:44:15, harkness wrote:
> > On 2017/01/12 14:40:04, Peter Beverloo wrote:
> > > This will likely contribute to making this test flaky. Can we fake this
> > somehow
> > > without reloading the page?
> > 
> > I don't know of a way to do it. Browser tests are supposed to test the way a
> > test is used by a developer and there's no tool we have to bump up the site
> > engagement other than normal use. Is there a way to inject user input or
> playing
> > media into the test?
> 
> Probably, but that will also need a fair bit of plumbing of its own. I was
> hoping that we might be able to use
SiteEngagementScore::SiteEngagementService()
> here.

Done!

https://codereview.chromium.org/2620393002/diff/60001/chrome/browser/budget_s...
File chrome/browser/budget_service/budget_database_unittest.cc (right):

https://codereview.chromium.org/2620393002/diff/60001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_database_unittest.cc:35: const double
kMaxSES = SiteEngagementScore::kMaxPoints;
On 2017/01/12 17:49:41, Peter Beverloo (OOO- Jan 19th) wrote:
> micro nit: just refer SES::kMaxPoints instead of copying?

Done.

https://codereview.chromium.org/2620393002/diff/60001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_database_unittest.cc:288: 2 * cost *
kMaxSES / kDefaultExpirationInDays / kMaxDailyBudget;
On 2017/01/12 17:49:41, Peter Beverloo (OOO- Jan 19th) wrote:
> nit: for future changes, while the precedence rules are clear, this would read
> much easier by adding a bunch of parenthesis

I agree completely, updated.

[harkness, 2017-01-13 11:54:52]

The CQ bit was checked by harkness@chromium.org

[harkness, 2017-01-13 11:54:52]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org
Link to the patchset: https://codereview.chromium.org/2620393002/#ps80001
(title: "Remove reloads from browser test and fix nits.")

[commit-bot: I haz the power, 2017-01-13 11:55:06]

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, 2017-01-13 12:10:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-13 12:11:01]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[harkness, 2017-01-13 13:11:26]

The CQ bit was checked by harkness@chromium.org

[harkness, 2017-01-13 13:11:26]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org
Link to the patchset: https://codereview.chromium.org/2620393002/#ps100001
(title: "Fix derp")

[commit-bot: I haz the power, 2017-01-13 13:11:39]

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, 2017-01-13 14:10:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-13 14:10:14]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[harkness, 2017-01-16 11:26:37]

The CQ bit was checked by harkness@chromium.org

[harkness, 2017-01-16 11:26:37]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org
Link to the patchset: https://codereview.chromium.org/2620393002/#ps120001
(title: "Changed LayoutTest to expect integral budgets.  (Also rebased)")

[commit-bot: I haz the power, 2017-01-16 11:26:54]

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, 2017-01-16 12:42:22]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1484565997181010,
"parent_rev": "f9ab0b53de5da5dc207c87b1a743b5b7b2ddedf0", "commit_rev":
"ccdc3f3aef6ac41aba49b35a754e24961ace471a"}

[commit-bot: I haz the power, 2017-01-16 12:42:51]

Description was changed from

==========
Refactor budget computation to be more tuneable.

Previously, the budget computation used a single master value,
kBudgetDurationInHours to control both how long awards were valid and
also indirectly, how much budget an origin received per hour. The fixed
value in the system was that an origin received SES_score budget per
kBudgetDurationInHours hours.

The side effect of this indirect effect is that if chromium wants
for awards to last shorter periods of time, then the origin will get
larger grants of budget. So modifying the duration increases the amount
of work the origin can do, which is usually unintended.

A previous CL did exactly that, decreasing the duration and unintentionally
increasing the amount of work that can be done.

This CL modifies the calculation to use two values. The duration is still
there and is used to determine how much budget the origin has been granted
recently and how long new grants should last. A new value provides a target
budget for a maximally engaged origin, is used to provide a fixed upper
limit for budget granted to the origin. Finally, this CL reverses the earlier
unintentional increase in daily budget. Before this patch, full engagement
origins received 100 budget per 4 days, now they receive 48 per 4 days.

Tests were also updated to use the new computation.

BUG=617971
==========

to

==========
Refactor budget computation to be more tuneable.

Previously, the budget computation used a single master value,
kBudgetDurationInHours to control both how long awards were valid and
also indirectly, how much budget an origin received per hour. The fixed
value in the system was that an origin received SES_score budget per
kBudgetDurationInHours hours.

The side effect of this indirect effect is that if chromium wants
for awards to last shorter periods of time, then the origin will get
larger grants of budget. So modifying the duration increases the amount
of work the origin can do, which is usually unintended.

A previous CL did exactly that, decreasing the duration and unintentionally
increasing the amount of work that can be done.

This CL modifies the calculation to use two values. The duration is still
there and is used to determine how much budget the origin has been granted
recently and how long new grants should last. A new value provides a target
budget for a maximally engaged origin, is used to provide a fixed upper
limit for budget granted to the origin. Finally, this CL reverses the earlier
unintentional increase in daily budget. Before this patch, full engagement
origins received 100 budget per 4 days, now they receive 48 per 4 days.

Tests were also updated to use the new computation.

BUG=617971

Review-Url: https://codereview.chromium.org/2620393002
Cr-Commit-Position: refs/heads/master@{#443874}
Committed:
https://chromium.googlesource.com/chromium/src/+/ccdc3f3aef6ac41aba49b35a754e...
==========

[commit-bot: I haz the power, 2017-01-16 12:42:52]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/ccdc3f3aef6ac41aba49b35a754e...