Gregory Szorc [:gps] Reporter
	Description • 11 years ago

I'm mass filing large ticket items from FHR's performance review: https://wiki.mozilla.org/Performance/FHR. This bug is for the first item:

7. Spinning of event loop at shutdown We shouldn't be spinning the event loop at shutdown. It seems event-loop spinning was added to allow an FHR upload to finish before Firefox shuts down, but we could achieve the same effect by cancelling the upload and updating state atomically at the end of the upload. 

Please note the above comment isn't entirely correct. We do not spin to wait for upload to finish. We only spin to wait for already-initiated SQLite operations to finish. The chances of this occurring are slim (a collection would have to occur just before shutdown).

We spin as a means to protect against database "corruption." By that, I mean partial data being inserted into the database.

Removing our event loop spinning will require async shutdown observers or some other means to gracefully tell shutdown to wait on an already-initiated async event to complete. See my detailed comments in bug 722648 on this issue.

	Benjamin Smedberg
	Comment 1 • 11 years ago

Why do we believe that spinning the event loop is a bad thing? I mean, all of the "async shutdown observers" proposals are basically doing just that, but centralizing the mechanism. That's a performance optimization but doesn't really change the core mechanism and isn't necessary.

	Gregory Szorc [:gps] Reporter
	Comment 2 • 11 years ago

I've been told there are potential security issues with creating a naive nested event loop (at least from JS).

	Benjamin Smedberg
	Comment 3 • 11 years ago

If there's content JS on the stack, that may be true. But if you're just doing that from a shutdown notification, I don't think that's accurate. Perhaps people are over-generalizing a "don't spin event loops" rule which really applies only in some cases?

	Richard Newman [:rnewman]
	Comment 4 • 11 years ago

Given that:

* We typically don't spin on shutdown
* Spinning isn't necessarily bad
* This might be addressed by Bug 722648, or that bug will find that it's not worth addressing

I'm going to mark this as P4, and we'll come back to it when we know more.

	Gregory Szorc [:gps] Reporter
	Comment 5 • 11 years ago

Now that Telemetry is saving and reporting histograms recorded on shutdown, I can now confirm my suspicions: the fallback of spinning on shutdown is rare. On the Nightly channel, we have a grand total of 10 reports of the event loop being spun on shutdown against ~250,000 uploads for the period I measured. That's 0.004%. And, no single spinning time was more than 1 second.

Thus, I'm going to assert our P4 priority is justified.

	Alessio Placitelli [:Dexter]
	Comment 6 • 9 years ago

FHR is going away per bug 1209088, wontfix.