1217115 - Consider some sort of optimizing out of success/error events on IDB requests

Status: NEW

(Core :: Storage: IndexedDB, defect, P3)

Description

[Boris Zbarsky [:bzbarsky]]

I'm not sure how much we can do here, because the spec is so underdefined.  For example, http://w3c.github.io/IndexedDB/#steps-for-asynchronously-executing-a-request doesn't say whether the "queuing" it does is meant to be a task or not, and if so which task source it's coming from.

So I'm going to assume that the spec is brought to sanity by defining that all async DOM event firing happens off tasks and all the tasks are posted to the "database access task source".

So there are a few places where we could try to optimize:

1)  If DOMEventTargetHelper::HasListenersFor returns false, no need to create and fire an event.  This one is obvious: this event not firing is not observable.  This lets us avoid some of the work of firing the event, but not the work of posting a runnable to the main thread.

2)  If we're adding the first task in the "database access task source" and the relevant event target has no listeners for that task (this CANNOT be implemented via DOMEventTargetHelper::HasListenersFor, because we're posting the task from another thread and DETH is not threadsafe; it involves the target keeping the state in a member that can be accessed atomically) and the task that caused the operation that is now queueing the database access task source task has completed, then we can optimize out queuing a task to the database access task source.  AKA not post a runnable to the main thread at all.

This one needs a bit more explanation.  The idea is that if we're in a state where the task that started the database operation has completed, and we're about to post a task to the database access task source, then we're in a state in which the spec offers no guarantees about any code getting to run between now and when that database access task source task runs, except for earlier tasks in the database input task source.  So if we have no listeners yet, there are no guarantees about being able to add them before we fire the event, so it's safe to not fire the event at all, or in fact to queue the mainthread task to fire it.

Now this setup has two drawbacks.  First, telling that the "task that started the database operation has completed" is a bit nontrivial.  Second, having something that _does_ have a listener makes us fall off a cliff: suddenly we have to post tasks for not only that thing but for everything that completes before that thing's listener has fired and it's fallen out of the event queue task source, because its event listener may add listeners to other things and whatnot.

Any other ideas?

Comment 1

[Boris Zbarsky [:bzbarsky]]

> out of the event queue task source

I meant out of the database access task source.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Note that some IDB events bubble, so we have to check more than just the target in certain cases.

Also note that "first task in the "database access task source"" is not something that is easy to determine in Gecko right now since we don't actually keep track of task sources.

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I'm also fairly confident that we generally need to return to the main thread to do bookkeeping of various sorts after each operation on the database threads, even if we do elide event dispatch entirely.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

One difficulty with optimizing out the runnable on the main thread is that its scheduled via PBackground IPC.  It basically means we need the parent process to determine if an event should be fired or not.

Also, there is some state information getting set on the request in order to support later getters.  So we would have somehow lazily get this off the parent process later if we don't want to schedule the runnable.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> is not something that is easy to determine in Gecko right now 

We can basically count the number of posted-but-not-fired IDB events to determine this, fwiw.

But ok, sounds like optimizing out the runnable is a non-starter.  So we should probably throttle the runnables and do the "no event listener" optimization.

And I think we should start keeping track of task sources.  Adding a separate task source just for IDB seems nice and small and self-contained, right?  And would let us do the throttling...

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Don't fire IDBRequest events with no listeners. r=khuey

This seems to improve the benchmark time by about 10% on a debug build.  Not sure if its correct given bubbling.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8677023 [details] [diff] [review]
Don't fire IDBRequest events with no listeners. r=khuey

So as far as bubbling goes...  Looks like the event bubbles up to the transaction from the request (see IDBRequest::PreHandleEvent).  So you have to fire the event if either the request or the transaction have listeners.

Comment 8

[Boris Zbarsky [:bzbarsky]]

And from transaction it bubbles to the IDBDatabase....

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

I think only error bubbles.  See:

  http://w3c.github.io/IndexedDB/#fire-a-success-event

"The event does not bubble and is not cancelable."

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8677023 [details] [diff] [review]
Don't fire IDBRequest events with no listeners. r=khuey

Success events don't bubble, so this would be fine there, I think. But error events do ...

We would also still need to do the logging and some of the ancillary stuff in the success case.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Doesn't matter whether they bubble.  You can have capturing listeners.

That said, I would expect those to be rare, so if we had a good way to check for lack of those, that would be good.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Don't fire IDBRequest success events with no listeners. r=khuey

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

I'm doing an opt build to see how useful this really is.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

We could coalesce the request success/error messages into a bulk IPC message.  This would schedule just a single runnable that might then break out into multiple events.

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8677031 [details] [diff] [review]
Don't fire IDBRequest success events with no listeners. r=khuey

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

r=me, I guess, although I'd like to see that there's a real perf improvement before we check this in.

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8677031 [details] [diff] [review]
Don't fire IDBRequest success events with no listeners. r=khuey

This does not show a statistically significant improvement in opt builds.  I ran 20 tries with and without patch.  The resulting t-test had an alpha of 0.21.