If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Can we make TI treat ListBase proxies as DOM objects?

RESOLVED FIXED in mozilla31

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

(Blocks: 1 bug)

unspecified
mozilla31
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Consider this (browser) testcase:

  var l = document.getElementsByTagName("*");
  var count = 100000;
  for (var i = 0; i < count; ++i) {
    l.item(0);
  }

Right now we don't end up doing specialized calls there, because our typeset for the "this" object of the item() call is unknownObject().  And the reason for that is that "l" is a proxy, presumably.

I believe all the DOM specialization code really needs to know about the "this" object here is:

1)  It's guaranteed to be a JSCLASS_IS_DOMJSCLASS or a ListBase proxy.
2)  It has never had its prototype mutated after it was originally created.
3)  Its prototype claims that things that use it as their original proto and
    satisfy condition #1 are safe to call the method on.

(We need #2 and #3 because type doesn't cover class or proxy handler; if it did we wouldn't have to worry about the proto, but would still need to check that the DOMClass here is ok.)  We use TI to guarantee these things right now, I believe.

So I guess the question is whether TI can guarantee us the above three things about ListBase proxies somehow without breaking other stuff...
Blocks: 920508
Per discussion with Eric right now, the other thing we need to do to successfully use this stuff is to prove that .getElementsByTagName returns the right thing.  That's a bit tricky for document, because of the shadowing behavior it has.

Eric was wondering how often the generation changes and hence might cause us to recompile if we're not using an IC setup.  It looks like we potentially increment the generation of the document any time one of the following happens:

1)  An <img>, <form>, <applet>, <embed>, or <object> with a nonempty "name" attribute is
    added to or removed from the DOM.
2)  An <img>, <applet>, <embed>, or <object> with a nonempty "id" attribute is added to or
    removed from the DOM.

In both cases, only HTML elements matter, of course.  I say "potentially", because if you already have one <img name="foo"> in the document and then add another one we don't bump the generation.

Thinking about this some more, though, it seems like we don't really need to recompile on generation changes in general.  Conceptually, something like this should work:

1)  Use the existing IC for the .getElementsByTagName get.
2)  Guard in the jitcode that we got an object and it's the same object as we expected; if
    not bail out and recompile or something.
3)  Now we know we have a definite function we're calling, and we just need to worry about
    the "this" value.

and it looks like we can in fact do that sort of thing already.  I just tried this silly testcase:

  var count = 100000;
  for (var i = 0; i < 100000; ++i)
    document.getElementsByTagName("aaa");

and makeCallHelper is called with a non-null target, so we're successfully handling this as a monomorphic callsite.

Comment 2

4 years ago
OK, this doesn't sound undoable in general.

The past few days have been troubling for my understanding of what happens in TI for various proxies, and ListBase proxies in particular.

If we really already get the target, then we just need the proxies properly marked as DOM objects, as you say, and all will be well, but I want to understand /why/ before hacking any more on this.

Brian, are you available at some point next week to talk in more depth about the proxy handling in TI nowadays?
Flags: needinfo?(bhackett1024)
Flags: needinfo?(bhackett1024)
So I poked around at this a bit.

Right now, proxies end up with an "anyobject" type because of two things:

1) An explicit MarkTypeObjectUnknownProperties(cx, proxy->type()) in ProxyObject::New
2) The JSObject::setNewTypeUnknown(cx, clasp, protoObj) in that same method.

note that neither one happens in the singleton case.

If I just comment out those two calls and fix TemporaryTypeSet::isDOMClass to not force fail on proxies, I get the behavior I want on simple cases.

That said, it seems like ideally we would in fact keep not tracking _properties_ on these objects, right?  We just want to track the JSClass...

Does that mean we need to rejigger the set of concepts of "unknown" TI uses?  At first glance we does.
Flags: needinfo?(bhackett1024)
Blocks: 820848
We mark non-singleton proxies as unknown so that if a JSObject::swap happens on them then we aren't required to crawl all type sets in the compartment to mark type sets containing the proxies as unknown --- swapping two objects (or mutating an object's prototype) changes its type so we need type sets containing those objects to be marked as having any object.

So it should be fine for correctness if you remove those two calls, and might be ok for performance too; I just don't know how much those swap() crawls will affect things.  If their cost is significant though then we'd need some notion for proxies-that-are-likely/unlikely-to-be-swapped and mark properties as unknown accordingly.
Flags: needinfo?(bhackett1024)
We could skip the calls in the case of DOM proxies only.  The only DOM proxies that are likely to get swapped are forms and documents (and the latter only on document.open())...

That said, if I just remove the calls I run into some problems in jitcode.  Specifically, in GetPropertyIC::tryAttachProxy we get false for monitoredResult() and hence don't attach a stub.

It looks like the issue is that PropertyReadNeedsTypeBarrier() returns true right now when the object type has unknown properties.  But if we stop labeling it as having unknown properties, then it returns false, which is presumably wrong here.

I could add something to TypeObjectKey that returns true if maybeType() and the type includes some proxy type...  Not sure whether that's the right thing to do here.
I guess what worries me is what other places might need similar changes.  :(
(In reply to Boris Zbarsky [:bz] from comment #5)
> We could skip the calls in the case of DOM proxies only.  The only DOM
> proxies that are likely to get swapped are forms and documents (and the
> latter only on document.open())...
> 
> That said, if I just remove the calls I run into some problems in jitcode. 
> Specifically, in GetPropertyIC::tryAttachProxy we get false for
> monitoredResult() and hence don't attach a stub.
> 
> It looks like the issue is that PropertyReadNeedsTypeBarrier() returns true
> right now when the object type has unknown properties.  But if we stop
> labeling it as having unknown properties, then it returns false, which is
> presumably wrong here.
> 
> I could add something to TypeObjectKey that returns true if maybeType() and
> the type includes some proxy type...  Not sure whether that's the right
> thing to do here.

PropertyReadNeedsTypeBarrier should just return true for objects which are proxy classes.  You can use TypeObjectKey::clasp() rather than looking at maybeType().  The monitoredResult() test will still preserve correctness as we already need to deal with the case where proxies might not have unknown properties (since singleton proxies have this property).
Another things that might need some changes, from a quick audit of unknownProperties() callers is jit::PropertyReadIsIdempotent.  I'm not sure what it's doing or how the changes here would affect it, but at first glance assuming non-idempotent for proxies might make sense...
Created attachment 8400739 [details] [diff] [review]
Keep track of DOM proxies in TI, like other DOM objects, so we can do the same call/get/set optimizaations with them.
Attachment #8400739 - Flags: review?(efaustbmo)
Assignee: general → bzbarsky
Status: NEW → ASSIGNED
Whiteboard: [need review]
Try run for this: https://tbpl.mozilla.org/?tree=Try&rev=a12a021b6be8
Comment on attachment 8400739 [details] [diff] [review]
Keep track of DOM proxies in TI, like other DOM objects, so we can do the same call/get/set optimizaations with them.

Review of attachment 8400739 [details] [diff] [review]:
-----------------------------------------------------------------

I am convinced this should be OK. r=me.

::: js/src/jsinfer.cpp
@@ +1760,5 @@
>  
>      unsigned count = getObjectCount();
>      for (unsigned i = 0; i < count; i++) {
>          const Class *clasp = getObjectClass(i);
> +        if (clasp && !clasp->isDOMClass())

This is good, but we should be aware of the consequences of this, relative to the accessor optimizations.
Attachment #8400739 - Flags: review?(efaustbmo) → review+
> but we should be aware of the consequences of this, relative to the accessor
> optimizations.

To be clear, in IRC conversation we determined that this does NOT activate the accessor optimizations for DOM proxies, for at least two reasons:

1) commonGetPropFunction and commonSetPropFunction return null, because they don't know about the DOM proxy IC stub types.

2) testCommonGetterSetter returns null, because it calls objectsHaveCommonPrototype, which returns false.  It does that because it calls ClassHasEffectlessLookup on all the object types, which returns false if !clasp->isNative(), which is the case for proxies.
https://hg.mozilla.org/integration/mozilla-inbound/rev/17f84a2187f2
Whiteboard: [need review]
https://hg.mozilla.org/mozilla-central/rev/17f84a2187f2
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.