Olli Pettay [:smaug][bugs@pettay.fi] Reporter
	Description • 11 years ago

I was profiling event creation time and noticed defining the one
[Unforgeable] property (.isTrusted) seems to take over 40% of the time.

I wonder if bindings layer should define properties in some different way or whether
there is something to optimize in jseng.

	Jason Orendorff [:jorendorff]
	Comment 1 • 11 years ago

What exactly is this profiling? How big was the sample?

Is it expected that we can make firing an event 40% faster by optimizing this? Because that would be huge.

	Olli Pettay [:smaug][bugs@pettay.fi] Reporter
	Comment 2 • 11 years ago

This bug is just about creating an object which has [Unforgeable] in the interface.

http://mozilla.pettay.fi/moztests/events/event_speed_2.1_createEvent.html is the testcase.

Not sure what you mean with "firing". There is event object creation and event dispatch and event handling. (Event objects can be reused, so dispatched and handled several times)
Event dispatch is already faster than in other browsers, event creation and event handling certainly aren't. There are bugs open to make event handling (webidl callback handling) faster.
Making .isTrusted [Unforgeable] is a new thing, and apparently it regressed object creation quite a 
bit.

	Olli Pettay [:smaug][bugs@pettay.fi] Reporter
	Comment 3 • 9 years ago

Looks like setting up the unforgeable attribute takes now >50% of Event creation time.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 4 • 9 years ago

So looking at a profile, you're right: that JS_DefineProperties call is now 52% of event creation time.

Focusing on time under JS_DefineProperties, it breaks down like so, with all percentages relative to the overall JS_DefineProperties time:

13% PropertySpecNameToId.  Mostly js::Atomize, strlen, and self time.
36% js::NewFunctionWithProto, presumably allocating the getter function.  Lots of
    ExclusiveContext::getSingletonType, ExclusiveContext::getNewType, JSObject::create
    (mostly GC here), EmptyShape::getInitialShape.
49% under DefinePropertyOrElement.  Some UpdateShapeTypeAndValue and CallAddPropertyHook
    (because this stuff is cced; of course we don't actually want our hook invoked in
    this case, but we have no way to prevent it) but mostly NativeObject::putProperty.
    This is almost all PropertyTree::getChild, doing Shape::new_ (hey, more GC) and
    hashtable ops.

There's got to be a better way of making this stuff work....

	Jan de Mooij [:jandem]
	Comment 5 • 9 years ago

(In reply to Boris Zbarsky [:bz] from comment #4)
>     This is almost all PropertyTree::getChild, doing Shape::new_ (hey, more
> GC) and hashtable ops.

This is madness. We have a Shape with like 300,000 children, then we GC and start all over again :(

The problem is that the getter object is different each time so we allocate a new shape for the property. Before bug 1073700 it was probably even worse because we also had to allocate a new BaseShape.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 6 • 9 years ago

So I was going to check how Chrome implements this, but they don't implement isTrusted at all as far as I can tell.

isTrusted _does_ need to be an accessor, because its value can change but it needs to be readonly and non-configurable.

So one viable option is to share a single getter function across all Event instances.  Basically, stash it in a reserved slot on Event.prototype and then define props on individual events with that cached getter.  That's observably different from what we do right now, and means we can't just use the property spec directly (as in, we have to manually reify things or add new JSAPI or something) but that might be OK.  Cameron, thoughts on the spec end of this?  Having to allocate a separate function per each Event instance does seem pretty sadfaces...

	Tom S [:evilpie]
	Comment 7 • 9 years ago

Boris, just for curiosity sake. I thought isTrusted just depends on who threw the exception, why does it need to change?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 8 • 9 years ago

This is for events, not exceptions.

It needs to change if the event is redispatched, so you can't take a trusted event, save it in an event listener and then later redispatch it and have it be treated as trusted.  See https://dom.spec.whatwg.org/#dom-eventtarget-dispatchevent step 2.

	Cameron McCormack (:heycam)
	Comment 9 • 9 years ago

I guess Event is the only object we create a lot of that has [Unforgeable] things on it.

I'd rather not have different behaviour for isTrusted and all other [Unforgeable] things.  And I wouldn't be opposed to sharing the getter's Function value across all instances (it is just like sharing non-[Unforgeable] getters/setters on a prototype).  And Functions for IDL operations too.

Probably we don't have anyone relying on the fact that each instance gets its own [Unforgeable] Function values, yeah?

	Olli Pettay [:smaug][bugs@pettay.fi] Reporter
	Comment 10 • 9 years ago

Yeah, I would be _very_ surprised if someone relies on each instance to get separate functions for [Unforgeable]s

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 11 • 9 years ago

In practice the only interfaces where you can observe sharing vs not right now are Event and Document, I think, since Location and Window are per-global singletons...

We should probably do the sharing thing in the spec.  Implementing it might not be too bad.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 12 • 9 years ago

Hmm.

So in terms of implementation, the simplest way might actually be to:

1)  Not actually remove unforgeable holders (per bug 1133760) but instead create them for _all_ cases when there are unforgeable props.

2)  JS_CopyPropertiesFrom stuff from the unforgeable holder to the instance object (or expando in the DOM proxy case).

Peter, thoughts?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 13 • 9 years ago

OK, with sufficient JS engine hackery, the approach of comment 12 seems to work pretty well.  Specifically, of the time under js::InvokeConstructor, 63% ends up being under Event::Constructor, 3% converting the event type to a DOMString, 6% looking up the right global for the event wrapper (we should really speed this up), 7% creating the binding object, and 2% copying the unforgeable props onto it.

I'll be filing bugs blocking this one, I guess.  Peter, I'd still like to hear back from you whether you prefer that I remove holders in bug 1133760 and then reinstate them or just roll the patches for this bug into that bug.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 14 • 9 years ago

Just uploading this as a backup

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 16 • 9 years ago

And one more thing: with that last bit I can get rid of the hackery with ordering unforgeables before/after setting the wrapper.  The new copying code won't trigger addProperty hooks, so won't preserve the wrapper.  The cases that don't use it are globals, which have a trivial addProperty hook.  So we can always just do unforgeables, then set the wrapper in the wrappercache.

	Jeff Walden [:Waldo]
	Comment 20 • 9 years ago

Comment on attachment 8568285 [details] [diff] [review]
Make defining unforgeable properties on objects faster by just copying them from an unforgeable holder object

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

Frankly, I'm not sure you should consider my Codegen.py reviewing here all that competent -- you probably should get someone else to look at it, too, who understands this code a whole lot better than I do.

::: dom/bindings/BindingUtils.h
@@ +3035,5 @@
>  nsresult
>  RegisterDOMNames();
>  
>  template <class T, ProtoHandleGetter GetProto>
> +JS::Handle<JSObject*>

Convention is to return raw pointers, not handles, for methods producing JS objects.  I'm not much certain what returning a handle does exactly, except maybe add an extra dereference in places.

::: dom/bindings/Codegen.py
@@ +2721,5 @@
> +            assert needInterfacePrototypeObject
> +            setUnforgeableHolder = CGGeneric(fill(
> +                """
> +                if (*protoCache) {
> +                  js::SetReservedSlot(*protoCache, DOM_INTERFACE_PROTO_SLOTS_BASE,

Could you add a #define for DOM_INTERFACE_UNFORGEABLE_TEMPLATE or something, rather than depending on a different name with less-clear meaning?

::: xpcom/base/CycleCollectedJSRuntime.cpp
@@ +618,5 @@
>    } else {
>      const DOMJSClass* domClass = GetDOMClass(aObj);
>      if (domClass) {
>        NS_CYCLE_COLLECTION_NOTE_EDGE_NAME(aCb, "UnwrapDOMObject(obj)");
> +      // It's possible that out object is an unforgeable holder object, in

our?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 21 • 9 years ago

Comment on attachment 8568285 [details] [diff] [review]
Make defining unforgeable properties on objects faster by just copying them from an unforgeable holder object

> Frankly, I'm not sure you should consider my Codegen.py reviewing here all that competent 

Er, sorry!  This was supposed to be peterv's thing, not yours.

> I'm not much certain what returning a handle does exactly

Avoids creating an extra Rooted in the caller.

> Could you add a #define for DOM_INTERFACE_UNFORGEABLE_TEMPLATE or something

Could do.  This is actually restoring the code that got removed in bug 1133760.

> our?

Yes.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 22 • 9 years ago

Try sez that XPCConvert::GetISupportsFromJSObject can see the holder objects (via the memory reporter machinery).  The simplest way to make that not fail is to just use UnwrapPossiblyNotInitializedDOMObject in UnwrapDOMObjectToISupports, since the latter is not super-performance-sensitive.

	Peter Van der Beken [:peterv]
	Comment 23 • 9 years ago

Comment on attachment 8569134 [details] [diff] [review]
Make defining unforgeable properties on objects faster by just copying them from an unforgeable holder object

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

::: dom/bindings/BindingUtils.h
@@ +3035,5 @@
>  nsresult
>  RegisterDOMNames();
>  
>  template <class T, ProtoHandleGetter GetProto>
> +JS::Handle<JSObject*>

Needs documentation about what it returns (I'd have expected the global that was created).

::: dom/bindings/Codegen.py
@@ +2585,5 @@
>  
> +        if self.descriptor.hasUnforgeableMembers:
> +            assert needInterfacePrototypeObject
> +            # We want to use the same JSClass and prototype as the object we'll
> +            # end up defining the unforgeable properties on in the end.  In the

Maybe mention that this is because we want to use JS_InitializePropertiesFromCompatibleNativeObject for performance (right?).

@@ +2587,5 @@
> +            assert needInterfacePrototypeObject
> +            # We want to use the same JSClass and prototype as the object we'll
> +            # end up defining the unforgeable properties on in the end.  In the
> +            # case of proxies that's null, because the expando object is a
> +            # vanilla object, but in the case of other DOM object's it's

s/object's/objects/

@@ +3152,5 @@
> +          $*{cleanup}
> +          return false;
> +        }
> +        """,
> +        name=descriptor.name,

Don't think you need this.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 24 • 9 years ago

> Needs documentation about what it returns (I'd have expected the global that was created).

Done:

  // The return value is whatever the ProtoHandleGetter we used
  // returned.  This should be the DOM prototype for the global.

Addressed the other comments too.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 25 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/8477c9f45172

	Carsten Book [:Tomcat]
	Comment 26 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/8477c9f45172