Closed Bug 1320301 Opened 8 years ago Closed 7 years ago

Add partial support to sqlite3_interrupt

Categories

(Toolkit :: Storage, defect, P1)

Product:
Toolkit
Component:
Storage
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Project Flags:
Performance Impact none
Tracking Flags:
Tracking Status
firefox53 --- affected
firefox57 --- fixed

People

(Reporter: mak, Assigned: mak)

References

(Blocks 1 open bug,
URL
)

Details

(Keywords: perf, Whiteboard: [fxsearch])

Attachments

(1 file)

Bug 1320301 - Add partial support to sqlite3_interrupt.
59 bytes, text/x-review-board-request
asuth
: review+
Details 
The use-case is something like the urlbar, that needs to quickly reply to user input.
What happens currently is that there's a long-running query from a previous input ongoing, the user changes input, we want to run a new query, but that gets enqueued to the already running one, whose results are now pointless.

A partial support to sqlite3_interrupt may be useful for this use-case.
1. Only for read-only connections (it's too scary to interrupt random writes)
2. Maybe only for async statements, otherwise we need a third thread and threads synchronization.

Indeed:
"The sqlite3_interrupt(D) call is in effect until all currently running SQL statements on database connection D complete. Any new SQL statements that are started after the sqlite3_interrupt() call and before the running statements reaches zero are interrupted as if they had been running prior to the sqlite3_interrupt() call. "

We should basically block the async thread until the statements count reaches 0.
Priority: -- → P3
this is very much like a problem I encountered while working for ATT with connections to both Oracle and Sybase.  The problem was such that each subsequent query superseded the need for the preceding query.  The solution could not be intrusive because the client code that executed the queries was not under the control of the group that needed the superseding behavior.

The solution I used was to create a new sub-class of the existing database connection abstraction called AutoCancelConnection.  This subclass overrode the "executeStatement" method such that it would cancel any pending queries on the connection before submitting a new statement. I don't recall the detail on pending callbacks for canceled queries, but vaguely recall spoofing them with empty collections as result sets.

Since the client code already created the connections it needed via a reference to a connection factory object, it was easy to pass it a new connection factory that created these AutoCancelConnection instances instead of standard connections.

The upshot of this was to get a new behavior out of the existing code without modifying that code.  I would imagine that a construct like this could be applied to this situation.
(In reply to K Lars Lohn [:lars] [:klohn] from comment #1)
> The upshot of this was to get a new behavior out of the existing code
> without modifying that code.  I would imagine that a construct like this
> could be applied to this situation.

Yes, there are various possible solutions I thought about:

1. just expose an interruptQueries API from mozIStorageAsyncConnection, that works on r/o connections and throws otherwise. The problem is that this API may appear confusing, and it's not very common in high level DB abstractions. This should be forwarded from Sqlite.jsm.

2. create a new connection type that is read only and always interrupts the previous query. This doesn't fit our case cause we have a set of queries running (not a single one), and we want to interrupt whatever of those is currently running, then start a new set of queries. We only want the interruption to happen at certain times, not for every execution.

3. create a new executeNow API. The problem is that we have executeAsync, ExecuteSimpleSQL from idl, and in Sqlite.jsm we additionally have execute and executeCached. We may need any of these for an instant query, and that may bring to an explosion of APIs. Or alternatively each API could grow an optional argument. Though this sounds like it may confuse users, it's a quite specific option for a single use-case.

4. Use a comment in the SQL query itself, similar to the "/* do not warn" we already have, something like /* interrupt */. This is an hackish solution, but doesn't require any public API. Though it requires extracting and parsing the SQL string every time, that is not free.

For now, the less invasive solution sounds like 1, but any brainstorming and feedback is welcome. We could even expose the thing differently between idl and Sqlite.jsm (the latter is just a wrapper of the former).
Ah, the problem with 4 is also that the call would only be evaluated when the previous query is done, so it's a no-go, it'd not interrupt anything.
Priority: P3 → P1
Whiteboard: [fxsearch]
It seems worthwhile to track this for photon-perf.
Blocks: photon-performance-triage
Whiteboard: [fxsearch] → [fxsearch][photon]
"perf" key word?
Keywords: perf
Whiteboard: [fxsearch][photon] → [fxsearch][photon][qf]
No longer blocks: photon-performance-triage
Whiteboard: [fxsearch][photon][qf] → [fxsearch] [qf]
Minusing for QuantumFlow since as far as I understand this won't reduce the amount of time we run code on the main thread...
Whiteboard: [fxsearch] [qf] → [fxsearch] [qf-]
(In reply to :Ehsan Akhgari (super long backlog, slow to respond) from comment #6)
> Minusing for QuantumFlow since as far as I understand this won't reduce the
> amount of time we run code on the main thread...

Right, this reduces pauses when typing in the locationbar, but it's not related to spending time on the main-thread.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Just dumping some thoughts:

There's another alternative, that is to use sqlite3_progress_handler(). The advantage compared to sqlite3_interrupt() is that it is a little bit "safer", in the sense it will only interrupt a single statement. On the other side sqlite3_interrupt() will interrupt any statement started after its invocation and the first point where the pending statements count is zero. That means potentially if thread B starts executing an unrelated statement before thread A statement has been canceled, both A & B will be interrupted.

To use setProgressHandler from Sqlite.jsm, we'd need a thread-safe handler in cpp, exposed through xpidl. But we already have mozIStoragePendingStatement already that provides a cancel() interface. Its problem is that it only interrupts a query at the first returned result. We could tweak it to use a progress handler. Then Sqlite.jsm could easily cancel() the currently running query and we'd not need any new Storage API.

The downside is that in general having a progress handler will slow down a bit the queries, because the Sqlite VM will have to stop every N instructions to invoke it. So maybe it's not the best bet for something that is trying to handle queries as fast as possible.
As I understand it, the only argument to the progress handler is the one we pass in.  So other than knowing it's associated with a single sqlite3*, it has no context to understand which statement it will interrupt when it returns non-zero.  sqlite3_interrupt similarly occurs at a sqlite3* granularity.  So the choice is between randomly interrupting one of the two statements A/B (progress handler), or interrupting both (sqlite3_interrupt).

Looking at how the isInterrupted check works in sqlite3VdbeExec, it's branch predictor friendly and involves no external function calls, so it seems likely to be a measurable (although likely small) win for performance over the progress handler.  Also, one less complexity to worry about.

If Places is still doing things with the same sqlite3* on multiple threads that could encounter this problem, it seems appropriate to have Places spin up another clone for things it wants to interrupt.  That is not only a win for sqlite3_interrupt ambiguity, but also for mutex contention on that sqlite3*.
I'm actually thinking to limit the interrupt() API to async-only read-only connections, and in such a case I think we are safe, since they can only run queries on the async thread.
Brw, I think the advantage of the progress handler is that it is fired by a long step(), and it can interrupt only that step(), so more or less you have some control over what you are interrupting, vs "everything". But I agree sqlite3_interrupt looks simpler to handle for our case and it's something we don't expose yet.
Yeah, the progress handler definitely with a high VM instruction count seems like it enables a number of neat things.  Like providing a hard stop on overly expensive queries.  I could see that being useful for conservatively exposing Places' information to WebExtensions.  (But for something like Places and autocomplete, it seems more appropriate to aggressively shard the search space and use something more like an "iterative deepening" approach to searches, rather than doing "whoops, that was too expensive, I give up".)

Which is to say, I'm on board for exposing the progress handler (maybe providing some extra context to any custom C++ handlers), but I think we should bias that towards high VM count values, and explicitly use sqlite3_interrupt for interruption purposes.
This is ready for a first check.
Comment on attachment 8893286 [details]
Bug 1320301 - Add partial support to sqlite3_interrupt.

https://reviewboard.mozilla.org/r/164342/#review170158

Your thoroughness in testing is always appreciated!

::: storage/test/unit/test_connection_interrupt.js:12
(Diff revision 2)
> +  // Interrupt can only be used on async connections.
> +  let db = getOpenedDatabase();
> +  Assert.throws(() => db.interrupt(),
> +                /NS_ERROR_ILLEGAL_VALUE/,
> +                "interrupt() should throw if invoked on a synchronous connection");
> +  await db.close();

nit: close() is void, it feels misleading to await it, although I get this is probably because linters may get mad if async functions don't await something.

::: storage/test/unit/test_connection_interrupt.js:32
(Diff revision 2)
> +  Assert.throws(() => db.interrupt(),
> +                /NS_ERROR_NOT_INITIALIZED/,
> +                "interrupt() should throw if invoked on a closed connection");
> +});
> +
> +add_task(async function test_async_conn() {

This test and its Sqlite.jsm twin unfortunately may intermittently fail.  The wisdom of the progress-handler approach is now much more apparent! :)

Specifically, we need one of the following things to happen to make the test reliable:

- Backpressure so that the async execution pauses if more than N result rows are outstanding.  (Noting that our Promises micro-task queue implementation is a little sketchy right now and it might be safer to directly invoke the interrupt from inside the result handler.)
- Create a variant on the C++ test_true_async.cpp AsyncCancellation test, but interrupt instead of canceling.  Also, work around sqlite3_interrupt() being a no-op if nothing is running by leaving an in-progress statement around on the main thread so the count doesn't hit 0.
- Implement an nsIThreadObserver in C++ so that we can set it on the main thread and listen to OnDispatchedEvent() and have that trigger the interrupt when the async thread dispatches the event result.
- Some kind of trickery involving a VFS layer that we can use to involve stalls.  That requires the test to actually read from a database and for us to have flushed the page cache, plus other ugly stuff.

There's probably some other possible options too.  My inclination would be to file a bug about implementing backpressure, then disable those tests citing the bug number in the code, and mentioning on that bug that the tests should be turned back on.  We would still want to have run the tests at least once against the core state of the final patch.

::: toolkit/components/places/UnifiedComplete.js:2362
(Diff revision 2)
>    },
>  
>    stopSearch() {
>      if (this._currentSearch) {
> +      if (!SwitchToTabStorage.updating && this._promiseDatabase) {
> +        this.getDatabaseHandle().then(conn => conn.interrupt())

I'm not sure if you meant to include the Places changes or not.  They look fine to me if you wanted me to review (delegated from you).
Attachment #8893286 - Flags: review?(bugmail) → review+
(In reply to Andrew Sutherland [:asuth] from comment #16)
> nit: close() is void, it feels misleading to await it, although I get this
> is probably because linters may get mad if async functions don't await
> something.

I think I just got confused between Sqlite.jsm .close() and conn.close(). We evaluated an eslint rule to force await in async functions, but gave up on it cause it disallows designing fake-async APIs.

> This test and its Sqlite.jsm twin unfortunately may intermittently fail. 
> - Backpressure so that the async execution pauses if more than N result rows
> are outstanding.  (Noting that our Promises micro-task queue implementation
> is a little sketchy right now and it might be safer to directly invoke the
> interrupt from inside the result handler.)

How are we supposed to create such a backpressure to be 100% sure our interrupt() is executed exactly during a sqlite3_step() call? It sounds hard.

> - Create a variant on the C++ test_true_async.cpp AsyncCancellation test,
> but interrupt instead of canceling.  Also, work around sqlite3_interrupt()
> being a no-op if nothing is running by leaving an in-progress statement
> around on the main thread so the count doesn't hit 0.

Pure-async connections don't allow to run main-thread statements.

> - Implement an nsIThreadObserver in C++ so that we can set it on the main
> thread and listen to OnDispatchedEvent() and have that trigger the interrupt
> when the async thread dispatches the event result.

that wouldn't still guarantee that interrupt() runs exactly during a sqlite3_step() call, afaict, it would only guarantee it runs close to it.

> - Some kind of trickery involving a VFS layer that we can use to involve
> stalls.  That requires the test to actually read from a database and for us
> to have flushed the page cache, plus other ugly stuff.

I guess it would be easier to create a bogus query for which step() enters an infinite loop, rather than a query that creates infinite results. Maybe I could give a try at that.
(In reply to Marco Bonardo [::mak] (Away 4-20 August) from comment #17)
> > This test and its Sqlite.jsm twin unfortunately may intermittently fail. 
> > - Backpressure so that the async execution pauses if more than N result rows
> > are outstanding.  (Noting that our Promises micro-task queue implementation
> > is a little sketchy right now and it might be safer to directly invoke the
> > interrupt from inside the result handler.)
> 
> How are we supposed to create such a backpressure to be 100% sure our
> interrupt() is executed exactly during a sqlite3_step() call? It sounds hard.

The goal of the backpressure in the test is for us to ensure that the async thread can't run to completion.  We needn't trigger it inside the sqlite3_step call from my perspective.  I was under the impression this is partially why you didn't have the interrupt call act like the user had called Cancel().  Because then we are exclusively testing that the signaling happened via SQLite, rather than potentially having that broken and instead relying on our cancellation logic.

But anywho...I originally posed back-pressure as a memory-saving device, but there's also the issue of whether or not we're spamming the main thread in such a way that we're inducing jank.  :bkelly introduced https://searchfox.org/mozilla-central/source/xpcom/threads/ThrottledEventQueue.h as a way of making sure Workers don't spam the main thread with postMessages in a way that makes the browser/page unresponsive.  It's essential for content, but the idea holds for chrome-privileged code as well.

I hadn't originally envisioned using it for this purpose, but since its public interface includes Length() and AwaitIdle() "Block the current thread until the queue is empty.", it's actually quite perfect for our purposes.  In notifyResults we'd add logic like the following up top:
  if (mCallingThread->Length() >= BACKPRESSURE_DEPTH_MAYBE_FROM_A_PREF) {
    mCallingThread->AwaitIdle();
  }
We'd need to have mCallingThread be a RefPtr to a ThrottledEventQueue, but that's it.

Our test logic would:
- Assert ThrottledEventQueue::IsEmpty() or spin the event loop to get it empty.
- Dispatch the test method.  Ensure the number of results is such that we can guarantee the backpressure situation will be hit.  Specifically (NUM_OPS / MAX_ROWS_PER_RESULT) >= BACKPRESSURE_DEPTH_MAYBE_FROM_A_PREF.
- Wait for ThrottledEventQueue::Length() to >= 1 but *not spinning the event loop so result ops back up*.  We just want to make sure the statement is executing, we don't need to wait for backpressure to activate.  (But we do rely on the backpressure to avoid the statement from completing entirely.)

I really like what ThrottledEventQueue does for us here.  We don't need to involve our own monitors or anything like that.
 
> > - Create a variant on the C++ test_true_async.cpp AsyncCancellation test,
> > but interrupt instead of canceling.  Also, work around sqlite3_interrupt()
> > being a no-op if nothing is running by leaving an in-progress statement
> > around on the main thread so the count doesn't hit 0.
> 
> Pure-async connections don't allow to run main-thread statements.

I just meant we would use the same thread wedging infrastructure and all that.  The test would look similar to that one, although using a non-async connection.

> > - Implement an nsIThreadObserver in C++ so that we can set it on the main
> > thread and listen to OnDispatchedEvent() and have that trigger the interrupt
> > when the async thread dispatches the event result.
> 
> that wouldn't still guarantee that interrupt() runs exactly during a
> sqlite3_step() call, afaict, it would only guarantee it runs close to it.

Agreed.  As per the above, I was viewing this as fine.  I think we can rely on SQLite to obey their sqlite3_interrupt contract.  Our unit test is more about our plumbing.

> > - Some kind of trickery involving a VFS layer that we can use to involve
> > stalls.  That requires the test to actually read from a database and for us
> > to have flushed the page cache, plus other ugly stuff.
> 
> I guess it would be easier to create a bogus query for which step() enters
> an infinite loop, rather than a query that creates infinite results. Maybe I
> could give a try at that.

Now I'm really liking the backpressure route, but yeah, an infinite loop is looking attractive.  Especially if we can make it sleep periodically, like by registering a sleep() function on the connection.
From a quick test (just for fun), something like this may do:
SELECT 0
UNION ALL
SELECT * FROM (
  WITH RECURSIVE test(n) AS (
    VALUES(1)
    UNION ALL
    SELECT n + 1 FROM test
  )
  SELECT t.n
  FROM test,test AS t
)
LIMIT 2

The first step returns soon, the second step just hangs forever. So when we are notified the first result, we can interrupt, the promise tick should help us getting close to an actual step() ongoing (we can't be sure, but that's fine as said). I'd be prone to take a decent approach now, than to look for perfection, since we are a bit resource constrained.

The idea of throttling results is sound, but looks a little bit over-engineered for this bug alone, and we don't know if it may open a can of worms and block this nice improvement for 57. It's worth its own investigation in a separate bug, that I can file.
You indeed suggested to land this with a disabled test and file a follow-up, I actually think with just a few changes we can already land a non-intermittent test, and still file the bug for the throttling solution.
Agreed, your revised test case looks great, and back-pressure/throttling really is another bug.  The test is a little CPU-hungry in a pathological scheduling situation, but even rr's chaos mode should yield control back to the main thread eventually, and it's worth burning a little test machine CPU to help Places be ultra-responsive.
Small drive-by review notes:

- I imagine SwitchToTabStorage.delete should set |this.updating = true;| before performing its query.
- Can two SwitchToTabStorage.{add,delete} calls be active at the same time? In that case |updating| should probably be a counter.
- In "this.getDatabaseHandle().then(conn => conn.interrupt())", the code passed to "then" is called asynchronously. While the timing window between is incredibly small, it might still be worth re-checking "!SwitchToTabStorage.updating" within for added safety?
(In reply to Simon Lindholm from comment #21)
> Small drive-by review notes:

Thank you, those are useful hints! I'll look into making a final patch today.
The problem with the "1 result + hanging query" approach is that we don't notify results until MAX_ROWS_PER_RESULT or MAX_MILLISECONDS_BETWEEN_RESULTS. My first attempt was in an Sqlite app that didn't have this chunking and that's why it worked there.
We can't hit the timeout because we just hang the async thread after the first result and the timeout works more like a delay check than a watchdog. This is not great since a query could indeed return some results very fast, and others after a while, that makes the first results inherit the worst time.
It is particularly bad for the Location Bar, since we never return more than MAX_ROWS_PER_RESULT, that means every query will wait at least 75ms before returning results, even if it could be faster.
Sounds like we need to really revise the whole results notification process :( But I'm not willing to do that here, this fix has its own benefit and the test is mostly a nice-to-have, no reason to block on a much larger and scarier refactoring, imo.
ThrottledEventQueue is a possible approach, though it seems to only send one notification at a time, what we need instead is more like a queue where we add results and every n ms we notify all the collected results (if any), and all of this logic should not be on the async thread (could be blocked) nor on the calling thread (don't introduce jank).
Sounds like we may need to add a third thread to the plan, so we send notifications to NotificationsThread and every n ms that thread notifies the calling thread? I have no idea if ThrottledEventQueue can be patched to do that.

Let's first try to push this improvement out of the door, then I'll file the relevant bug to discuss approaches further.
While I could modify the query to return 15 results then hang, that'd be too fragile against people changing MAX_ROWS_PER_RESULT, or even ourselves changing the notifications chunking code.
I'm thinking to just run the hanging query and interrupt it after a 500ms timeout, it should work in 99+% of cases, but it's clearly not rock solid. On the other side I assume our testing machines should be able to start a query in less than that time, unless they have pegged CPUs. I'd land the timeout and keep an eye on intermittents.
Comment on attachment 8893286 [details]
Bug 1320301 - Add partial support to sqlite3_interrupt.

Andrew, could you please tell me if this is acceptable, modulo all the previous discussions?

Simon, I implemented your suggestions, and changed things around a little bit so I can keep just using the search.stop() call, what do you think?
Attachment #8893286 - Flags: review?(simon.lindholm10)
Attachment #8893286 - Flags: review?(bugmail)
Attachment #8893286 - Flags: review+
Comment on attachment 8893286 [details]
Bug 1320301 - Add partial support to sqlite3_interrupt.

I'm sorry, mozreview madness.
Attachment #8893286 - Flags: review?(bugmail)
Attachment #8893286 - Flags: feedback?(simon.lindholm10)
Comment on attachment 8893286 [details]
Bug 1320301 - Add partial support to sqlite3_interrupt.

https://reviewboard.mozilla.org/r/164342/#review176894

Forgot about the batching snafu.  Agreed that's work for another bug.  I'll find a bug to comment on about that rather than cluttering this bug up any further.

(I'm explicitly not reviewing the Places stuff since Simon is handling that.)

::: storage/test/unit/test_connection_interrupt.js:63
(Diff revision 5)
> +  });
> +
> +  // Wait for the statement to be executing.
> +  // This is not rock-solid, see the discussion in bug 1320301. A better
> +  // approach will be evaluated in a separate bug.
> +  await new Promise(resolve => do_timeout(500, resolve));

I'm worried about Android emulators and 500 not being enough there.  Given that we don't expect this functionality to operate any differently there, how about adding the following to the add_task prolog of this function so it's:

```
add_task({
  // We use a timeout in the test that may be insufficient on Android emulators.
  // We don't really need the Android coverage, so skip on Android.
  skip_if: () => AppConstants.platform == "android"
},
function test_async_conn() {
...sweet test logic...
});
```

with a `
Cu.import("resource://gre/modules/AppConstants.jsm");` up at the top of the file if we don't already have one somewhere in a head.js.
Attachment #8893286 - Flags: review?(bugmail) → review+
Comment on attachment 8893286 [details]
Bug 1320301 - Add partial support to sqlite3_interrupt.

The UnifiedComplete.js parts look good to me.

One potential thing to change is to make SwitchToTabStorage.initDatabase do all the queued |add| calls in parallel with "await Promise.all(...);", to avoid lots of back-and-forth message passing between threads that blocks initialization when a user has many tabs. Or to not "await" the calls at all (especially since the first query shouldn't need to cancel any previous ones).
Attachment #8893286 - Flags: feedback?(simon.lindholm10) → feedback+
(Apropos of initialization: oughtn't we call |getDatabaseHandle()| on idle just after Firefox startup? Seems like it would be nice to make the first search faster.)

One thing I don't fully grasp about the patch is how it guarantees that queries started after |interrupt()| don't get cancelled. Don't we need special handling for the quiescence period required by sqlite?
(In reply to Simon Lindholm from comment #30)
> One potential thing to change is to make SwitchToTabStorage.initDatabase do
> all the queued |add| calls in parallel with "await Promise.all(...);", to
> avoid lots of back-and-forth message passing between threads that blocks
> initialization when a user has many tabs. Or to not "await" the calls at all
> (especially since the first query shouldn't need to cancel any previous
> ones).

I feel like it wouldn't really make a difference, since all the queries on the Sqlite side are serialized regardless.
Surely we could not wait at all for "add" but then the first query may return wrong results, afaict.

(In reply to Simon Lindholm from comment #31)
> (Apropos of initialization: oughtn't we call |getDatabaseHandle()| on idle
> just after Firefox startup? Seems like it would be nice to make the first
> search faster.)

We should first confirm your theory with data, I suspect using the location bar is one of the first things most users do on opening a session, so idle may not bring any improvement there.

> One thing I don't fully grasp about the patch is how it guarantees that
> queries started after |interrupt()| don't get cancelled. Don't we need
> special handling for the quiescence period required by sqlite?

All the queries are serialized on a thread, and interrupt happens on the main-thread, when it happens either there's no query in-flight (and it's a no-op) or there's a query in-flight and it will be interrupted when it reaches the next vdbe check point.
Since a new query would be serialized to the current one, it should happen when the queries count has reached 0. Basically, there should never be 2 queries started at the same time.
This is in theory, then we'll see what happens in reality :)
(In reply to Marco Bonardo [::mak] from comment #32)
> > One thing I don't fully grasp about the patch is how it guarantees that
> > queries started after |interrupt()| don't get cancelled. Don't we need
> > special handling for the quiescence period required by sqlite?
> 
> All the queries are serialized on a thread, and interrupt happens on the
> main-thread, when it happens either there's no query in-flight (and it's a
> no-op) or there's a query in-flight and it will be interrupted when it
> reaches the next vdbe check point.
> Since a new query would be serialized to the current one, it should happen
> when the queries count has reached 0. Basically, there should never be 2
> queries started at the same time.
> This is in theory, then we'll see what happens in reality :)

Aha, makes sense! So this is even a bit conservative: at most one query queued before |interrupt| may be interrupted.

> (In reply to Simon Lindholm from comment #30)
> > One potential thing to change is to make SwitchToTabStorage.initDatabase do
> > all the queued |add| calls in parallel with "await Promise.all(...);", to
> > avoid lots of back-and-forth message passing between threads that blocks
> > initialization when a user has many tabs. Or to not "await" the calls at all
> > (especially since the first query shouldn't need to cancel any previous
> > ones).
> 
> I feel like it wouldn't really make a difference, since all the queries on
> the Sqlite side are serialized regardless.

Possibly. Depends on how time is spent during SQL execution, and how much is thread-bouncing + JS execution. I'll do some measurements.

> Surely we could not wait at all for "add" but then the first query may
> return wrong results, afaict.

Hm. Aren't queries still executed in the order |executeCached| is called?
(In reply to Simon Lindholm from comment #33)
> Hm. Aren't queries still executed in the order |executeCached| is called?

Yes, that's true!
(In reply to Simon Lindholm from comment #33)
> Possibly. Depends on how time is spent during SQL execution, and how much is thread-bouncing + JS execution. I'll do some measurements.

So with 13 tabs, the add calls in initDatabase take:

With individual "await"s:
5.625
5.385
6.105
11.6
8.375
10.255
6.155
5.6
9.28
6.23
9.005

With a single "await Promise.all(promises)":
10.525
9.055
4.835
9.205
8.125
4.91
4.985
4.75
4.83
15.965
5.57

With no "await"s at all:

5.495
11.975
3.69
3.645
7.005
3.71
6.685
8.265


So no large difference between the two first options, while the last one wins a millisecond on initialization (which is then possibly lost in the first "await"ed SQL query after that, who knows).
(All in milliseconds.) The entire getDatabaseHandle takes ~10-20 ms, which is faster than I would have thought. Makes doing it on startup less meaningful, too.
Btw, in the latest patch I removed the await. Thank you for the constant insights.
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/34018fbd4096
Add partial support to sqlite3_interrupt. r=asuth
See Also: → 1393440
https://hg.mozilla.org/mozilla-central/rev/34018fbd4096
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Isn't this worthy of an uplift to Firefox 56 beta? A faster updating urlbar is rather desired for someone like me with a lot of history and bookmarks.
nope, too risky.
Performance Impact: --- → -
Whiteboard: [fxsearch] [qf-] → [fxsearch]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: