Disables MinorGC from collecting object wrappers

Disables MinorGC from collecting object wrappers

The current minor GC collects all unmodified wrappers that are not active. It does not take care about references. Hence, it is not safe to mark any object that might contain a reference as not active. Several object wrappers could have references, so disabled collecting them during minor GC.

BUG=553054
Committed: https://crrev.com/94d243b66bbd558b2ebb1be5eea1b69b8de27a3b
Cr-Commit-Position: refs/heads/master@{#361339}

[mythria, 2015-11-20 11:35:53]

mythria@chromium.org changed reviewers:
+ haraken@chromium.org, jochen@chromium.org

[mythria, 2015-11-20 11:35:54]

I disabled minor gc from collecting object wrappers since we do not update
references during minor gc. I did this to fix this Issue:
https://code.google.com/p/chromium/issues/detail?id=553054 

Could you please review my changes. 

Thanks,
Mythri

[jochen (gone - plz use gerrit), 2015-11-20 13:36:37]

lgtm

[haraken, 2015-11-20 14:08:28]

> Since minor GC does not update references, it is not safe to collect object
wrappers. Updating references during minor GC can be time consuming.

Would you help me understand why it's not safe to collect object wrappers? Also
what does "updating references" mean?

(I was assuming that we collect object wrappers in minor GCs, which enables us
to remove the notion of MarkIndependent.)

[jochen (gone - plz use gerrit), 2015-11-20 14:39:48]

On 2015/11/20 at 14:08:28, haraken wrote:
> > Since minor GC does not update references, it is not safe to collect object
wrappers. Updating references during minor GC can be time consuming.
> 
> Would you help me understand why it's not safe to collect object wrappers?
Also what does "updating references" mean?
> 
> (I was assuming that we collect object wrappers in minor GCs, which enables us
to remove the notion of MarkIndependent.)

we'd have to put all the wrapper references in place like we do during a major
gc

[haraken, 2015-11-20 14:47:51]

On 2015/11/20 14:39:48, jochen wrote:
> On 2015/11/20 at 14:08:28, haraken wrote:
> > > Since minor GC does not update references, it is not safe to collect
object
> wrappers. Updating references during minor GC can be time consuming.
> > 
> > Would you help me understand why it's not safe to collect object wrappers?
> Also what does "updating references" mean?
> > 
> > (I was assuming that we collect object wrappers in minor GCs, which enables
us
> to remove the notion of MarkIndependent.)
> 
> we'd have to put all the wrapper references in place like we do during a major
> gc

Why don't we need to store node wrapper references as well? I'm wondering why
you want to specialize the behavior for node wrappers.

BTW, are you assuming that node wrappers >> object wrappers? Or object wrappers
>> node wrappers? (I don't have any number in hand.)

[jochen (gone - plz use gerrit), 2015-11-20 14:49:59]

On 2015/11/20 at 14:47:51, haraken wrote:
> On 2015/11/20 14:39:48, jochen wrote:
> > On 2015/11/20 at 14:08:28, haraken wrote:
> > > > Since minor GC does not update references, it is not safe to collect
object
> > wrappers. Updating references during minor GC can be time consuming.
> > > 
> > > Would you help me understand why it's not safe to collect object wrappers?
> > Also what does "updating references" mean?
> > > 
> > > (I was assuming that we collect object wrappers in minor GCs, which
enables us
> > to remove the notion of MarkIndependent.)
> > 
> > we'd have to put all the wrapper references in place like we do during a
major
> > gc
> 
> Why don't we need to store node wrapper references as well? I'm wondering why
you want to specialize the behavior for node wrappers.
> 

minor gc used to be specialized to node wrappers only before as well, no?

> BTW, are you assuming that node wrappers >> object wrappers? Or object
wrappers >> node wrappers? (I don't have any number in hand.)

no idea

[jochen (gone - plz use gerrit), 2015-11-20 14:59:00]

(anyways, processing all references is also an option)

[haraken, 2015-11-20 15:03:53]

> minor gc used to be specialized to node wrappers only before as well, no?

Yes, but I was assuming that the new minor GC got rid of the limitation :)

So the situation is that V8 is updating references only for node wrappers but
not yet for object wrappers. Assuming that behavior, this CL LGTM.

> (anyways, processing all references is also an option)

Yeah, I think it's worth investigating. Collecting object wrappers in minor GCs
(and removing the notion of independent wrappers) can be a win. I'm not familiar
with the cost of updating references, but it sounds a bit strange that if it is
a win for node wrappers but not a win for object wrappers.

[mythria, 2015-11-20 15:23:27]

Description was changed from

==========
Disables MinorGC from collecting object wrappers

Currently, minor GC collects all unmodified wrappers. Since minor GC does
not update references, it is not safe to collect object wrappers. Updating
references during minor GC can be time consuming. So, disabled collection
of object wrappers during minor GC.

BUG=553054
==========

to

==========
Disables MinorGC from collecting object wrappers

Currently, minor GC collects all unmodified wrappers. Since minor GC does not
update object references, disabled collection of object wrappers during minor
GC.

BUG=553054
==========

[mythria, 2015-11-20 16:06:51]

Description was changed from

==========
Disables MinorGC from collecting object wrappers

Currently, minor GC collects all unmodified wrappers. Since minor GC does not
update object references, disabled collection of object wrappers during minor
GC.

BUG=553054
==========

to

==========
Disables MinorGC from collecting object wrappers

Currently, minor GC collects all unmodified wrappers. Minor GC does not update
object references. So, disabled collection of object wrappers during minor GC.

BUG=553054
==========

[haraken, 2015-11-24 02:16:59]

We've been observing a bunch of suspicious crashes after landing the new minor
GC.

Although my CL (https://codereview.chromium.org/1475493002/) fixed most of the
crashes, if this CL is also needed to fix a wrong behavior of the minor GC,
please land & merge it to M48.

[mythria, 2015-11-24 09:42:02]

Description was changed from

==========
Disables MinorGC from collecting object wrappers

Currently, minor GC collects all unmodified wrappers. Minor GC does not update
object references. So, disabled collection of object wrappers during minor GC.

BUG=553054
==========

to

==========
Disables MinorGC from collecting object wrappers

The current minor GC collects all unmodified wrappers that are not active. It
does not take care about references. Hence, it is not safe to mark any object
that might contain a reference as not active. Several object wrappers could have
references, so disabled collecting them during minor GC.

BUG=553054
==========

[mythria, 2015-11-24 11:50:19]

I was looking at updating references for object class Ids. According to my
understanding, the new minor GC does not handle references both for node or
object wrappers. In the node wrappers, if it has any event listeners we ignore
them during minor GC. Similarly, we need to ignore any object wrappers that
might contain references. I added a check to do that. Though I am not sure if
that is sufficient. Please have a look at it and let me know if my understanding
is correct. 

Thanks,
Mythri

https://codereview.chromium.org/1464783002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp (right):

https://codereview.chromium.org/1464783002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:150:
v8::Persistent<v8::Object>::Cast(*value).MarkActive();
Would this check be sufficient to filter out all object wrappers that could
contain a reference? If it is not then I will revert it back to the earlier
code, where we ignore all object wrappers in minor GC.

[jochen (gone - plz use gerrit), 2015-11-24 12:44:24]

lgtm

[haraken, 2015-11-24 12:59:08]

Given that we need to merge this change to M48, I'd propose landing PS1 for
safety.

Just to confirm: Even if we skip collecting non-Node wrappers (i.e., PS1), the
new minor GC can collect non-Node wrappers that are marked as independent,
right?

https://codereview.chromium.org/1464783002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp (right):

https://codereview.chromium.org/1464783002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:149: if
(type->visitDOMWrapperFunction) {

Unfortunately this check is not correct. type->visitDOMWrapperFunction means
that this wrapper has a reference to some wrapper, not that this object is
referenced by some wrapper.

[haraken, 2015-11-24 12:59:53]

On 2015/11/24 11:50:19, mythria wrote:
> I was looking at updating references for object class Ids. According to my
> understanding, the new minor GC does not handle references both for node or
> object wrappers. In the node wrappers, if it has any event listeners we ignore
> them during minor GC. Similarly, we need to ignore any object wrappers that
> might contain references. I added a check to do that. Though I am not sure if
> that is sufficient. Please have a look at it and let me know if my
understanding
> is correct. 

Regarding node wrappers that have event listeners, it is taken care of by line
128.

[blink-reviews, 2015-11-24 13:48:53]

On Tue, Nov 24, 2015 at 12:59 PM, <haraken@chromium.org> wrote:

> Given that we need to merge this change to M48, I'd propose landing PS1 for
> safety.


> Just to confirm: Even if we skip collecting non-Node wrappers (i.e., PS1),
> the
> new minor GC can collect non-Node wrappers that are marked as independent,
> right?


Yes, it collects non-node wrappers that are marked independent.

>
>
>
>
>
>
https://codereview.chromium.org/1464783002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp
> (right):
>
>
>
https://codereview.chromium.org/1464783002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:149: if
> (type->visitDOMWrapperFunction) {
>
> Unfortunately this check is not correct. type->visitDOMWrapperFunction
> means that this wrapper has a reference to some wrapper, not that this
> object is referenced by some wrapper.
>
> https://codereview.chromium.org/1464783002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2015-11-24 13:48:53]

On Tue, Nov 24, 2015 at 12:59 PM, <haraken@chromium.org> wrote:

> Given that we need to merge this change to M48, I'd propose landing PS1 for
> safety.


> Just to confirm: Even if we skip collecting non-Node wrappers (i.e., PS1),
> the
> new minor GC can collect non-Node wrappers that are marked as independent,
> right?


Yes, it collects non-node wrappers that are marked independent.

>
>
>
>
>
>
https://codereview.chromium.org/1464783002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp
> (right):
>
>
>
https://codereview.chromium.org/1464783002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:149: if
> (type->visitDOMWrapperFunction) {
>
> Unfortunately this check is not correct. type->visitDOMWrapperFunction
> means that this wrapper has a reference to some wrapper, not that this
> object is referenced by some wrapper.
>
> https://codereview.chromium.org/1464783002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[haraken, 2015-11-24 14:12:26]

Thanks for the clarification. PS3 LGTM.

Let's land the PS3 and merge it to M48. Then let's consider making the minor GC
even smarter :)

[mythria, 2015-11-24 14:21:44]

Thanks for your reviews. I reverted my new changes and will land the new patch.
with this patch, we ignore all object wrappers that are not marked independent
during minor gc. I will try to see if we have a easy way of checking if the
wrapper is referenced or not and update minor GC in a different cl.

https://codereview.chromium.org/1464783002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp (right):

https://codereview.chromium.org/1464783002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:149: if
(type->visitDOMWrapperFunction) {
On 2015/11/24 12:59:08, haraken wrote:
> 
> Unfortunately this check is not correct. type->visitDOMWrapperFunction means
> that this wrapper has a reference to some wrapper, not that this object is
> referenced by some wrapper.

Thank you for your review. Reverted this change.

[mythria, 2015-11-24 15:03:19]

The CQ bit was checked by mythria@chromium.org

[mythria, 2015-11-24 15:03:19]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1464783002/#ps40001
(title: "Reverted the earlier patch. Back to PS1")

[commit-bot: I haz the power, 2015-11-24 15:04:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1464783002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1464783002/40001

[commit-bot: I haz the power, 2015-11-24 15:09:26]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-11-24 15:10:03]

Patchset 3 (id:??) landed as
https://crrev.com/94d243b66bbd558b2ebb1be5eea1b69b8de27a3b
Cr-Commit-Position: refs/heads/master@{#361339}