New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 868177 link

Starred by 3 users

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

getAll() limited by max IPC size, making it unreliable

Project Member Reported by dpa...@chromium.org, Jul 27

Issue description

This was revealed by https://github.com/google/lovefield/issues/240.

At [1] a check was added to prevent getAll() from exceeding the max IPC size. The relevant code is at [2] and the max IPC size is defined at 128MB at [3] as

static constexpr size_t kMaximumMessageSize = 128 * 1024 * 1024;

128MB seems like a pretty small limit for a database, and makes getAll() basically unreliable, as the user needs to know the size of their DB before deciding whether to use a cursor, or getAll().

Is there anything that can be done to restore the usefulness of getAll()?


[1] https://codereview.chromium.org/1321583002
[2] https://cs.chromium.org/chromium/src/content/browser/indexed_db/indexed_db_database.cc?l=1151-1156
[3] https://cs.chromium.org/chromium/src/ipc/ipc_channel.h?g=0&l=144
 
See [1] for a mitigation suggestion on a similar bug, pasting below:

"We should use shared memory (or better, a Mojo data
pipe) to send the data instead of encoding the data directly into a message."


[1] https://bugs.chromium.org/p/chromium/issues/detail?id=792655#c10
Using shared memory or mojo data pipes here isn't exactly easy though, as while in that other bug (and in some other places like that) we're just sending a bunch of bytes, here we're sending structured data, and currently mojo doesn't really have a easy way to transfer a mojom struct over a data pipe and/or shared memory.
Cc: roc...@chromium.org
Status: Available (was: Untriaged)
I'm not sure what the right long-term fix here is. I don't think we should unconditionally return successfully from getAll() -- if you try to get 2GB of stuff from the database to Blink in one getAll() call, you will OOM and your tab will crash.

Right now, I think the code should use count() before getAll(). Ideally, we'd have some limit option on getAll().
getAll() does have a limit option. :)

(It's on the number of records not the size of the payload, but the latter wouldn't necessarily be helpful since there's no way to know how much is too much.)
We (Lovefield) need to load everything to provide long transactions. The
thing is we don't know/can't know IPC limit, and this getAll was created to
optimize our use case. This regression literally disable the performance
improvement been done before and force us to fallback to slower cursor
based loading.
This limitation is unfortunate, but not a regression. We never supported splitting getAll() results over multiple IPC messages, so the size limit was always there.
jsbell@: Gah, sorry I didn't read the spec carefully!

WDYT about a "limit: whatever you think works" option? I think that'd be handy for apps / libraries that build an in-memory cache.

We'd need some sort of signal that no further paging is needed (perhaps an empty results list will have to do). The app would then keep calling getAll with an IDBRange.lowerbound(key_from_last_result) to page through the results, and the browser would get to pick the buffer size.
getAll() gets you the values not the keys, so knowing what the key_from_last_result is can be tricky.


Comment 10 Deleted

Yeah- the key would have to be stored in the value (I think we sometimes derive the key from the value if idb is set up that way).

Given that people can have gigs of data in an object store, this should definitely be set up to:
1. Allow small batches of getAll, where the user can ask for another batch. I'm guessing this is already possible by storing the id in the value, and using the count option.
2. Fetch everything, even if it is > IPC size, and try to store it all in the renderer before firing onSuccess. This can OOO if the stuff is too big for the renderer.
3. Fetch as much as possible? some option around 'get me as much as you can, but I understand if it can't all fit in memory', I'll figure out how to continue. The user would have to know if there is more, so they can do a second fetch (matching option 1)

#3 is following what Victor is talking about. If we want to also allow people who don't store their key in the value, then we would have to have a way to return the 'last key' so the user can fetch the next getAll using IDBRange.lowerbound
Cc: -roc...@chromium.org rockot@google.com
@dmurph: getAll() was added as an efficient way of bringing all contents of a database into memory. I think is OK to go with 2, and assume that everything fits in memory. This would restore the usefulness of getAll() for the majority case.


Sign in to add a comment