RulesRegistryWithCache::ProcessChangedRules speed-up

RulesRegistryWithCache::ProcessChangedRules speed-up

Postpone rules->value conversion:
Currently the rules are converted to base::Value on the rules registry thread before ProcessChangedRules finishes, and then a task is posted to UI which will trigger writing that to a rule store on yet another thread.
This CL postpones the conversion to the task running on UI thread.

This change cuts down the running time for AddRules to 75%, but of course this is cheating, since the work only gets done later. The point is, however, that network requests are not blocked until the rules are written.

BUG=236368
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=202152

[vabr (Chromium), 2013-05-07 16:10:02]

Hi Dominic,

Another review for you :).

Now I pretty much agree that writing the rules to the storage on start-up
immediately after reading them is a waste of time.
This reading comes in two steps: convert rules to base::Value, and write that to
disk. The former blocked the start-up on rules registry thread.

In this CL, I'm postponing the converting to a separate task on the UI thread.
We could postpone it even further to the thread for writing to the disk,
although it probably does not belong to that thread. Even if we get rid of this
immediate write on start-up, we still need to consider the right time to convert
the rules, because some extensions re-register rules on start-up manually
anyway.

Thanks,
Vaclav

[battre, 2013-05-07 16:22:09]

https://codereview.chromium.org/14851011/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/declarative/rules_registry_with_cache.h
(right):

https://codereview.chromium.org/14851011/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/declarative/rules_registry_with_cache.h:180:
std::map<ExtensionId, RulesDictionary> rules_;
This variable is going to be deleted with my CL. I think there is no purpose
then of pursuing this CL, right?

[vabr (Chromium), 2013-05-07 16:40:41]

On 2013/05/07 16:22:09, battre wrote:
>
https://codereview.chromium.org/14851011/diff/1/chrome/browser/extensions/api...
> File chrome/browser/extensions/api/declarative/rules_registry_with_cache.h
> (right):
> 
>
https://codereview.chromium.org/14851011/diff/1/chrome/browser/extensions/api...
> chrome/browser/extensions/api/declarative/rules_registry_with_cache.h:180:
> std::map<ExtensionId, RulesDictionary> rules_;
> This variable is going to be deleted with my CL. I think there is no purpose
> then of pursuing this CL, right?

Sure, I reverted that part. Please take another look.

Vaclav

[battre, 2013-05-14 09:54:29]

Sorry for the delay. 

I think the CL description needs to be updated.

Overall, I don't see the value we get from doing the conversion on the UI
thread. Usually we want to have as little work as possible being done on the UI
thread to keep the UI responsive.

Best regards,
Dominic

[vabr (Chromium), 2013-05-14 13:55:23]

Thanks, Dominic.

On 2013/05/14 09:54:29, battre wrote:
> Sorry for the delay. 
> 
> I think the CL description needs to be updated.
Done.

> Overall, I don't see the value we get from doing the conversion on the UI
> thread. Usually we want to have as little work as possible being done on the
UI
> thread to keep the UI responsive.

The current state of running this on the rules registry thread is not right.
This is what happens:
On UI thread, ReadFromStorageCallback is called, and causes the following tasks
to be posted on the rules registry thread, in this order:
DeserializeAndAddRules (this includes the expensive RulesToValue call which I
want to postpone)
OnReady (this sets the ready_ flag, which gates the network requests blocked by
the loading of rules)

What I want to achieve is to make the RulesToValue run after, or in parallel
with OnReady.

These are the choices I see:
1. Current patch set 2. Cons: work done on the UI thread.
2. Postponing RulesToValue even further, to the store's thread.
3. Leaving RulesToValue on the rules registry thread, but postpone it after
OnReady (so that OnReady would trigger it somehow).

WDYT?

Vaclav

> 
> Best regards,
> Dominic

[vabr (Chromium), 2013-05-23 09:34:47]

Hi Dominic,

This CL appears to have been forgotten. Which of the 3 suggestions below would
you prefer for moving on with this?

Thanks,
Vaclav

On 2013/05/14 13:55:23, vabr (Chromium) wrote:
> Thanks, Dominic.
> 
> On 2013/05/14 09:54:29, battre wrote:
> > Sorry for the delay. 
> > 
> > I think the CL description needs to be updated.
> Done.
> 
> > Overall, I don't see the value we get from doing the conversion on the UI
> > thread. Usually we want to have as little work as possible being done on the
> UI
> > thread to keep the UI responsive.
> 
> The current state of running this on the rules registry thread is not right.
> This is what happens:
> On UI thread, ReadFromStorageCallback is called, and causes the following
tasks
> to be posted on the rules registry thread, in this order:
> DeserializeAndAddRules (this includes the expensive RulesToValue call which I
> want to postpone)
> OnReady (this sets the ready_ flag, which gates the network requests blocked
by
> the loading of rules)
> 
> What I want to achieve is to make the RulesToValue run after, or in parallel
> with OnReady.
> 
> These are the choices I see:
> 1. Current patch set 2. Cons: work done on the UI thread.
> 2. Postponing RulesToValue even further, to the store's thread.
> 3. Leaving RulesToValue on the rules registry thread, but postpone it after
> OnReady (so that OnReady would trigger it somehow).
> 
> WDYT?
> 
> Vaclav
> 
> > 
> > Best regards,
> > Dominic

[battre, 2013-05-24 09:36:49]

Sorry for the delay. I think that processing on the UI thread is not good. Do
you have a preference regarding option 2 and 3?

[vabr (Chromium), 2013-05-24 11:21:09]

On 2013/05/24 09:36:49, battre wrote:
> Sorry for the delay. I think that processing on the UI thread is not good. Do
> you have a preference regarding option 2 and 3?

The delay was actually beneficial, since now I have |ready_| as a OneShotEvent
from Jeffrey at hand.
So I went for option 3, please check it out.

Thanks!
Vaclav

[battre, 2013-05-24 11:46:10]

lgtm

[battre, 2013-05-24 11:46:43]

On 2013/05/24 11:46:10, battre wrote:
> lgtm

Uh... check out the unit tests... Maybe that was too early.

[vabr (Chromium), 2013-05-24 14:44:18]

On 2013/05/24 11:46:43, battre wrote:
> On 2013/05/24 11:46:10, battre wrote:
> > lgtm
> 
> Uh... check out the unit tests... Maybe that was too early.

Thanks, Dominic.

The WeakPtrFactory used to not be initialised for tests, but now it needs to
because of the increased number of posted tasks. It's a small change, but please
take look. The current failures look more like bot's problems now.

Cheers,
Vaclav

[battre, 2013-05-24 14:58:16]

I agree. LGTM

[commit-bot: I haz the power, 2013-05-24 15:18:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vabr@chromium.org/14851011/31001

[commit-bot: I haz the power, 2013-05-24 19:58:44]

Change committed as 202152