Make a process-wide cache for the v8::Value representation of extension APIs.

Make a process-wide cache for the v8::Value representation of extension APIs.


TEST=browser_tests


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=127939

[not at google - send to devlin, 2012-03-07 10:07:22]

Same deal as the other patch -- this depends on
http://codereview.chromium.org/9460002/

[Aaron Boodman, 2012-03-07 21:07:10]

lgtm

https://chromiumcodereview.appspot.com/9616055/diff/5001/chrome/renderer/exte...
File chrome/renderer/extensions/extension_dispatcher.h (right):

https://chromiumcodereview.appspot.com/9616055/diff/5001/chrome/renderer/exte...
chrome/renderer/extensions/extension_dispatcher.h:205: // Process-wide (since
ExtensionDispatcher is process wide) cache for the v8
Don't add the bit about 'process-wide'. That is repeating information that is
already evident by the way that ExtensionDispatcher itself is used.

https://chromiumcodereview.appspot.com/9616055/diff/5001/chrome/renderer/exte...
File chrome/renderer/extensions/v8_schema_registry.cc (right):

https://chromiumcodereview.appspot.com/9616055/diff/5001/chrome/renderer/exte...
chrome/renderer/extensions/v8_schema_registry.cc:22: // around it before, but I
don't see how this could be false, so yeah...
Makes sense. Note: for these kind of review comments, you can just add a note in
Rietveld.

https://chromiumcodereview.appspot.com/9616055/diff/5001/chrome/renderer/exte...
File chrome/renderer/extensions/v8_schema_registry.h (right):

https://chromiumcodereview.appspot.com/9616055/diff/5001/chrome/renderer/exte...
chrome/renderer/extensions/v8_schema_registry.h:20: class V8SchemaRegistry {
I guess it's not really a 'registry' since nobody ever calls Register() on it.
Not sure what is is...

[not at google - send to devlin, 2012-03-08 00:00:33]

http://codereview.chromium.org/9616055/diff/5001/chrome/renderer/extensions/e...
File chrome/renderer/extensions/extension_dispatcher.h (right):

http://codereview.chromium.org/9616055/diff/5001/chrome/renderer/extensions/e...
chrome/renderer/extensions/extension_dispatcher.h:205: // Process-wide (since
ExtensionDispatcher is process wide) cache for the v8
On 2012/03/07 21:07:10, Aaron Boodman wrote:
> Don't add the bit about 'process-wide'. That is repeating information that is
> already evident by the way that ExtensionDispatcher itself is used.

Done.

http://codereview.chromium.org/9616055/diff/5001/chrome/renderer/extensions/v...
File chrome/renderer/extensions/v8_schema_registry.cc (right):

http://codereview.chromium.org/9616055/diff/5001/chrome/renderer/extensions/v...
chrome/renderer/extensions/v8_schema_registry.cc:22: // around it before, but I
don't see how this could be false, so yeah...
On 2012/03/07 21:07:10, Aaron Boodman wrote:
> Makes sense. Note: for these kind of review comments, you can just add a note
in
> Rietveld.

Ok.

http://codereview.chromium.org/9616055/diff/5001/chrome/renderer/extensions/v...
File chrome/renderer/extensions/v8_schema_registry.h (right):

http://codereview.chromium.org/9616055/diff/5001/chrome/renderer/extensions/v...
chrome/renderer/extensions/v8_schema_registry.h:20: class V8SchemaRegistry {
On 2012/03/07 21:07:10, Aaron Boodman wrote:
> I guess it's not really a 'registry' since nobody ever calls Register() on it.
> Not sure what is is...

Is is as is.

[not at google - send to devlin, 2012-03-12 07:02:17]

Hi guys, PTAL.

After debugging koz's performance regression with him (caused by the use of
v8::Persistent) I reckon that the caching of these schema objects would have the
same problem.

So I've rewritten the caching to use hidden values set on the context that
V8SchemaRegistry owns.  This is what koz did for his patch too, which seems to
work.

[Aaron Boodman, 2012-03-12 07:09:47]

Can you give me a pointer to more information about the performance problem?

[not at google - send to devlin, 2012-03-12 07:19:11]

On 2012/03/12 07:09:47, Aaron Boodman wrote:
> Can you give me a pointer to more information about the performance problem?

We should talk about it tomorrow when koz is around.  I don't fully understand
the problem, but I (we?) put it down to garbage collection problems.

In koz's case we were holding onto a reference to the require() function to pass
it into scripts when they're loaded.  Reference to the require() function was
being kept in a v8::Persistent member function of ModuleSystem (require_).  When
we changed this to be a chromeHidden style of sharing data (on the hidden value
in the context) the performance regression went away (and it was huge; doubled
the running time of one of the page cycler tests).

I figure that if holding onto a single v8::Persistent v8::Function can cause
problems, probably holding onto v8::Persistent v8::Objects can't be great either
(and I think every usage of v8::Persistent is just for v8::Contexts).

[koz (OOO until 15th September), 2012-03-13 00:27:56]

lgtm

I'm not sure why there's such a big performance penalty for allocating
Persistent cells. Maybe it's because V8 has to potentially rearrange everything
that is captured by a closure when that closure (require()) is made persistent?

[Aaron Boodman, 2012-03-13 06:36:28]

+ager

Mads, we were surprised to see that creating one persistent handle to a function
had a massive impact on page cycler performance.

Here is the original change:

http://git.chromium.org/gitweb/?p=chromium.git;a=blob;f=chrome/renderer/modul...

This causes a ~70% increase in page cyclers.

By process of elimination, James tracked it to creating the persistent handle,
and changed it to:

http://git.chromium.org/gitweb/?p=chromium.git;a=blob;f=chrome/renderer/modul...

This still has page cycler impact, but much much smaller (~2%, which we are
still investigating).

I'm confused that the persistent handle has any perf impact at all. It must be
very common in Chrome to create per-context persistent handles. Any ideas?

[Aaron Boodman, 2012-03-13 06:38:49]

Here's the 70% spike, just for fun:

http://build.chromium.org/f/chromium/perf/xp-release-dual-core/intl2/report.h...

:-).

[Mads Ager (google), 2012-03-13 08:56:07]

Hi Aaron,

I don't see anything in the code that gets rid of the Persistent handle again?
Whenever you create a Persistent handle, you have to make sure that Dispose is
called on that handle! Otherwise you will forever leak the memory pointed to. In
this case the thing you are pointing to is a function. That functions keeps its
context alive. That context keeps the global object alive and therefore probably
the entire DOM tree as well. Therefore, when running the page cycler (which, I
think, loads all the pages in one process) my guess would be that you keep the
memory for all the pages alive in the VM heap which will hurt badly. In the wild
this would probably lead to a lot of out-of-memory situations.

Using hidden properties gets around this issue because the reference is inside
of JavaScript and is taken care of by V8. If you would rather use Persistent
handles, you need to make the handle weak. That will give you a callback when
the function is no longer reachable in JavaScript code and you can then call
Dispose on the Persistent handle which will allow V8 to GC the memory
transitively pointed to.

Cheers,    -- Mads

On 2012/03/13 06:36:28, Aaron Boodman wrote:
> +ager
> 
> Mads, we were surprised to see that creating one persistent handle to a
function
> had a massive impact on page cycler performance.
> 
> Here is the original change:
> 
>
http://git.chromium.org/gitweb/?p=chromium.git;a=blob;f=chrome/renderer/modul...
> 
> This causes a ~70% increase in page cyclers.
> 
> By process of elimination, James tracked it to creating the persistent handle,
> and changed it to:
> 
>
http://git.chromium.org/gitweb/?p=chromium.git;a=blob;f=chrome/renderer/modul...
> 
> This still has page cycler impact, but much much smaller (~2%, which we are
> still investigating).
> 
> I'm confused that the persistent handle has any perf impact at all. It must be
> very common in Chrome to create per-context persistent handles. Any ideas?

[Aaron Boodman, 2012-03-13 19:12:58]

On Tue, Mar 13, 2012 at 1:56 AM,  <ager@google.com> wrote:
> I don't see anything in the code that gets rid of the Persistent handle
> again?
> Whenever you create a Persistent handle, you have to make sure that Dispose
> is
> called on that handle! Otherwise you will forever leak the memory pointed
> to.

Ah yes, of course. Thanks.

As long as I have you: one API point I have never completely
understood. It seems like for a given JavaScript object, you can both:

a) Create a new persistent handle (v8::Persistent::New(thingy)).
b) Copy persistent handles by value.

My understanding is that each handle created by (a) - even if they all
point to the same object - must be explicitly disposed. But the
handles created by (b) do not need to be disposed.

Is this correct?

- a

[Mads Ager (chromium), 2012-03-14 08:35:41]

On Tue, Mar 13, 2012 at 8:12 PM, Aaron Boodman <aa@chromium.org> wrote:
> On Tue, Mar 13, 2012 at 1:56 AM,  <ager@google.com> wrote:
>> I don't see anything in the code that gets rid of the Persistent handle
>> again?
>> Whenever you create a Persistent handle, you have to make sure that Dispose
>> is
>> called on that handle! Otherwise you will forever leak the memory pointed
>> to.
>
> Ah yes, of course. Thanks.
>
> As long as I have you: one API point I have never completely
> understood. It seems like for a given JavaScript object, you can both:
>
> a) Create a new persistent handle (v8::Persistent::New(thingy)).
> b) Copy persistent handles by value.
>
> My understanding is that each handle created by (a) - even if they all
> point to the same object - must be explicitly disposed. But the
> handles created by (b) do not need to be disposed.
>
> Is this correct?

Yes, that is correct. Copying persistent handles can be dangerous as
the copies all point to the same cell in the VM. Calling dispose on
any of the copies invalidates all of them.

persistent = Persistent::New(thingy);
copy = persistent;
copy.Dispose();
// persistent is invalid!

This situation has come up multiple times. If you want to hold on to
an object that you get from a weak persistent handle, you should use
Local::New(persistent) which will create a new local handle which will
be valid even if the persistent handle gets disposed.

This definitely is hairy stuff. :(

Cheers,    -- Mads

[not at google - send to devlin, 2012-03-21 09:41:42]

Ok, rebased this change and went back to the old (originally LGTM'd way) of
doing this with Persistent handles.  Thank for all that info!

[commit-bot: I haz the power, 2012-03-21 09:41:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/9616055/21002

[commit-bot: I haz the power, 2012-03-21 11:48:12]

Change committed as 127939