1353928 - CacheIR: Ignore addProperty hook on DOM objects

Status: NEW

(Core :: JavaScript Engine: JIT, enhancement, P3)

Description

[Tom S [:evilpie]]

Attachment: Ignore addProperty hook on DOM objects when attachin CacheIR stub

We can't add AddSlot or AddElement stubs when the obj has an AddProperty hook. However I believe this hook only needs to be called once on a DOM object to preserve its wrapper. In theory this should improve bug 1328858, but it probably won't work, because that IC site is too polymorphic.

This seems to work on GDocs, which attaches new properties to DOM objects. (Again some sites there seem megamorphic, maybe we could come up with a megamorphic IC for AddSlot)

Can we do this?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8855098 [details] [diff] [review]
Ignore addProperty hook on DOM objects when attachin CacheIR stub

The problem is this.  Say we have a function like so:

  function f(arg) {
    arg.expando = 0;
  }

and we have a bunch of DOM objects all of the same class flow in, all with no properties before they're passed to the function.

We need to call the addProperty hook on all of them, right?  None of their wrappers are preserved at entry into our function.  Why would that happen in this new setup?

If we added explicit preservation, that would certainly solve the problem.  But explicit preservation is actually kinda hard right now, because it needs access to the wrapper cache pointer, which is not that easy to get to from the JS object.  I can try to think of ways to do this from jitcode if we want to go the explicit-preservation way.

Comment 2

[Jan de Mooij [:jandem]]

Would it be possible to remove the addProperty hook and instead call a new JSAPI like |bool JS::ObjectHasNoProperties(obj)| at the point where we decide whether we want to preserve the wrapper?

Comment 3

[Jan de Mooij [:jandem]]

(I filed bug 1354015 for a somewhat-related wrapper preservation bug.)

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Would it be possible to remove the addProperty hook and instead call a new JSAPI like |bool
> JS::ObjectHasNoProperties(obj)| at the point where we decide whether we want to preserve the wrapper?

I would love it if we had a better API than addProperty to use here.

A hook that gets triggered on a certain set of conditions instead would be lovely.

ObjectHasNoProperties would unfortunately not do quite the right thing for objects with unforgeable properties, because we set up those properties by calling JS_InitializePropertiesFromCompatibleNativeObject, which adds properties to them without calling addProperty.  I wonder whether we could have some sort of shape flag we set up for DOM objects when we create them, which JS_InitializePropertiesFromCompatibleNativeObject preserves, and which indicates "we can recover this state if this object is collected".  Anything that reshapes an object with such a flag would trigger a callback to Gecko to preserve it.  Or something.

Comment 5

[Jan de Mooij [:jandem]]

We *could* add a new hook, let's say "objectMutated". This hook would be infallible (just a JSObject* argument and void return type) and it would be called when adding a property or when __proto__ is mutated.

Then it would be easy to add a CacheIR instruction to call this hook (masm.callWithABI), it shouldn't have that much overhead.

Unfortunately the addProperty hook is also used by NPAPI and some XPConnect objects, so we can't remove it, and adding another branch to the add-property path isn't great. Maybe we could use a Class flag or a union or something...

Comment 6

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #5)
> Unfortunately the addProperty hook is also used by NPAPI and some XPConnect
> objects, so we can't remove it, and adding another branch to the
> add-property path isn't great. Maybe we could use a Class flag or a union or
> something...

The simplest thing would be to add a new struct, something like:

  struct AddOrMutateHook {
      JSAddPropertyOp addProperty;
      JSMutateObjectOp mutateObject;
  };

Then JS::Class would store a pointer to this struct instead of JSAddPropertyOp.