getAll() limited by max IPC size, making it unreliable |
||||
Issue descriptionThis 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
,
Jul 27
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.
,
Jul 30
,
Aug 8
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().
,
Aug 9
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.)
,
Aug 10
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.
,
Aug 30
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.
,
Aug 30
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.
,
Aug 30
getAll() gets you the values not the keys, so knowing what the key_from_last_result is can be tricky.
,
Sep 4
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
,
Oct 17
,
Nov 7
@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 |
||||
Comment 1 by dpa...@chromium.org
, Jul 27