1016130 - Store proxy handler in the object class

Status: RESOLVED DUPLICATE of bug 966518

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: patch

Proxy handlers are not created on demand, so it seems to make more sense to store them as part of the Class than in instance objects.  This is more efficient in memory usage but more importantly this is the main step in removing all predefined slots in proxies.  If proxies don't have any predefined slots then they can be used in more cases, particularly (hopefully) for typed objects, the only remaining non-native non-proxy class.

The attached patch makes this change.  It's generally straightforward, except for wrapper renewing.  The tests we do to decide whether we can reuse wrappers when e.g. nuking a compartment don't really apply when there can be many classes for the various wrappers, and it's not clear to me what the purpose of these tests is.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Is this different from bug 966518?

Comment 2

[Boris Zbarsky [:bzbarsky]]

And if not, does it have the static initializer issues that bug was trying to avoid?

Comment 3

[Brian Hackett [Laid off!]]

(In reply to Boris Zbarsky [:bz] from comment #2)
> And if not, does it have the static initializer issues that bug was trying
> to avoid?

This does look much the same, why did progress in that bug stall?  This bug doesn't change the behavior of proxy handlers at all, unlike that stuff.

I don't see anything about static initializers in that bug.  This does add more js::Class global variables, though there wouldn't be any new constructors or anything that run at startup.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> why did progress in that bug stall?

Eric?

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

There was a GC issue in bug 966518. It looks like this patch has the same problem. Let's all three of us talk about it tomorrow. We really need to make progress on this.

The issue is that the SetClassAndProto call when renewing wrappers can cause us to trigger a write barrier on an object that's supposed to be dead. The code is really hairy here. Given that Eric and Brian have both run into it, I think it might be time to consider a better fix.

Comment 6

[Eric Faust [:efaust]]

(In reply to Bill McCloskey (:billm) from comment #5)
> There was a GC issue in bug 966518. It looks like this patch has the same
> problem. Let's all three of us talk about it tomorrow. We really need to
> make progress on this.
> 
> The issue is that the SetClassAndProto call when renewing wrappers can cause
> us to trigger a write barrier on an object that's supposed to be dead. The
> code is really hairy here. Given that Eric and Brian have both run into it,
> I think it might be time to consider a better fix.

If that went away, and we could cleanly replace shapes, I could resurrect this patch set really easily. I don't really care who does it, but I agree it needs to be done.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8428928 [details] [diff] [review]
patch

It sounds like we're going with Eric's patch.