1143308 - Use SQLite's mmap'ed I/O mode for IndexedDB

Status: NEW

(Core :: Storage: IndexedDB, defect, P3)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1

This gives us some speed wins in my perf testing. Talked about the risks with sicking and he's ok with them. Details at https://www.sqlite.org/mmap.html

Comment 1

[Andrew Sutherland [:asuth] (he/him)]

I was under the impression that especially on 32-bit windows we're experiencing address space shortages and fragmentation problems, especially with video card drivers doing video memory mmaps in our address space.  Maybe it'd be a good idea to check on dev-platform if this is something that should only be enabled when we're running 64-bit for desktop builds?

Comment 2

[Jan Varga [:janv]]

Comment on attachment 8577618 [details] [diff] [review]
Patch, v1

Review of attachment 8577618 [details] [diff] [review]:
-----------------------------------------------------------------

Do you know how it affects quota handling ?

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Andrew Sutherland [:asuth] from comment #1)
> should only be enabled when we're running 64-bit for desktop builds?

Yeah, this could be 64bit only.

(In reply to Jan Varga [:janv] from comment #2)
> Do you know how it affects quota handling ?

It doesn't, this all happens below our quota VFS layer.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

"An I/O error on a memory-mapped file cannot be caught and dealt with by SQLite. Instead, the I/O error causes a signal which, if not caught by the application, results in a program crash."

What kind of IO errors are we talking here?  Is it something as mundane as sqlite wanted to remove a temp file, but windows has it locked?

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

No, this is about the way POSIX deals with mmap'd I/O errors via the SIGBUS signal.

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

There's a Windows flavor of the problem too, but it's still just about I/O errors reading/writing a mapped file.

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8577618 [details] [diff] [review]
Patch, v1

SQLite has a Windows bug in mmap mode where database size isn't set correctly in xTruncate, which causes our quota handling to go haywire. This will need to wait for a SQLite fix.

Comment 8

[Simon Giesecke [:sg] [he/him]]

(In reply to Ben Turner (not reading bugmail, use the needinfo flag!) from comment #7)

Comment on attachment 8577618 [details] [diff] [review]
Patch, v1

SQLite has a Windows bug in mmap mode where database size isn't set
correctly in xTruncate, which causes our quota handling to go haywire. This
will need to wait for a SQLite fix.

Has this SQLite bug been fixed in the meantime? I couldn't find any related issue in the SQLite issue tracker.

Comment 9

[Jan Varga [:janv]]

We can try this again, but we need to be careful. Also, it would be good to do some preliminary performance testing and see if this actually has any significant effect on performance. I know the comment 0 says there are some perf wins, but that was 5 years ago. If there's now only negligible improvement it's not worth the hassle with handling I/O errors and quota tracking. There might be differences in mmap perf across platforms (or specific problems happening only on some platforms), in that case we would enable it on some platforms only.
Also, we might need to write new tests for quota tracking to be 100% sure it's safe to enable mmap.

Comment 10

[Jan Varga [:janv]]

See also:
https://bugs.chromium.org/p/chromium/issues/detail?id=489784
https://bugs.chromium.org/p/chromium/issues/detail?id=537742