-
Notifications
You must be signed in to change notification settings - Fork 258
Add explainer for IndexedDB batch request proposal #1163
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
||
## Goals | ||
|
||
Improve throughput by decreasing transaction durations. Decrease latency of database reads. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: "improve throughput by decreasing transaction durations" seems slightly tautological? Perhaps something along the lines of "Improve throughput by reducing overhead."
Also I think we might need to define "latency" and "throughput" a little more concretely. I guess this is saying
- throughput, the max number of records that will be retrieved per second, increases, because a single txn that is reading multiple ranges will finish faster
- latency, the time between when a transaction is created and its (first) result comes back, goes down sometimes, because it might spend less time blocked on a now-batched transaction. (But another way you could interpret "latency" --- time to read a single record when the system is otherwise idle --- will not go down, so it feels a little like we're double-counting the same improvement here.)
|
||
Improve throughput by decreasing transaction durations. Decrease latency of database reads. | ||
|
||
By performing multiple operations in a single request, batching reduces the number of JavaScript events required to read records and complete transactions. Each JavaScript event runs as a task on the main JavaScript thread. These tasks can introduce overhead when transactions require a sequence of tasks that go back and forth between the main JavaScript thread and the IndexedDB I/O thread. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a sequence of tasks that go back and forth between the main JavaScript thread
By the use of the word "sequence", this kind of makes it seem like we are talking about a series of blocking steps, i.e. a page that creates a transaction, makes a request, waits for response, makes a new request, waits for response, makes a new request etc. However the more apt comparison is a bunch of requests that are all made at the "same" time (i.e. in a single javascript task). And that does seem to be what the original issue reporter was talking about since they said "The test I'm referring to used a single transaction with N getAll requests in parallel vs 1 prototype batchGetAll call with N ranges."
- Reading multiple ranges of records. | ||
- Deleting multiple ranges of records. | ||
|
||
## Goals |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are saying the motivation is performance (not, say, ergonomics), it seems like we should be putting some benchmark data front and center --- do we have a POC that we have tested?
I do see remarks here to the effect that there's a 3x speedup on the table, which is pretty extraordinary and should be mentioned pretty high up in the explainer if it's something we can reproduce!
As an aside, 3x is quite a bit more improvement than I would have expected and I would want to do some investigation to understand how parallel requests are so bad in comparison, in Chromium.
|
||
Combine read, write and delete operations into a single batched request. This proposal batches requests of the same type only with either batched reads, batched writes or batched deletes. | ||
|
||
## Batched reads: `getAll()`, `getAllKeys()`, `getAllRecords()`, `get()`, `getKey()` and `count()` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Batching get()
feels a little odd to me. Is there anything you can do with batched get
that you couldn't do with batched getAll
, or a reason there would be a perf difference between the two? (Would we even create the get
operation if we were writing IDB from scratch, or would we just rely on getAll
with a very specific key range?) This feels like it could cause devs to be confused over which thing they should be using.
Also it seems we may want to consider the merits of batching different methods separately. First, because we may find that it's measurably useful to batch one operation and not another, and second, because it will be more work to prototype and OT all the things rather than just the thing we think is most important (probably getAll*
?) This is not to say we shouldn't consider batching all the things, but perhaps sequentially rather than all at once, especially if we want to use field trials to help us measure perf impact.
{ count: 10, direction: 'prev' }, | ||
]); | ||
request.onsuccess = () => { | ||
// `first_ten_records` and `first_ten_records` are each arrays of `IDBRecord` results. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: first_ten_records
and last_ten_records
Link to explainer w/ formatted markdown.