Skip to content
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

Use mmap to accelerate SQLite file accesses #4275

Closed
wants to merge 2 commits into from

Conversation

bavison
Copy link
Contributor

@bavison bavison commented Feb 24, 2014

Here are a couple of changes that between them make SQLite searches about 41% faster in my tests on a Raspberry Pi.

They have only been tested with PR #4235 in place, and the timings were also done using SQLite version 3080301, but they may still work with the older version 3071000 (though you'll probably get at least some warnings about line offsets in the patch operation).

The time it takes to issue "SELECT * FROM tvshowview" (the first time the
TV Shows library is opened) with 345 TV shows on a non-overclocked
Raspberry Pi changed as follows. Times are in seconds:

      Before          After
      Mean   StdDev   Mean   StdDev  Confidence  Change
Time  3.352  0.132    2.550  0.021   100.0%      +31.5%

These times assume SQLite has already been upgraded to version 3080301
(see PR xbmc#4235).
The time taken to issue "SELECT * FROM tvshowview" in seconds improves
still further in this case.

      Before          After
      Mean   StdDev   Mean   StdDev  Confidence  Change
Time  2.550  0.021    2.375  0.043   100.0%      +7.4%

Note, this requires the standard SQLite amalgamation tarball to be patched
prior to building. The change may not be desirable further upstream due to
the fact that it might cause database access times to increase in usage
scenarios that differ from those employed by XBMC.
@elupus
Copy link
Contributor

elupus commented Feb 25, 2014

According to sqllite docs:
"This compile-time limit and the SQLITE_MAX_MMAP_SIZE can be modified at start-time using the sqlite3_config(SQLITE_CONFIG_MMAP_SIZE) call, or at run-time using the mmap_size pragma"

Can't we do this runtime instead so it takes effect on system libs too?

Also that seem like a somewhat large value for the define? I'm not sure what happen if sqllite runs out of memory.

@popcornmix
Copy link
Member

I believe settings the MMAP size large is safe. Eben investigated this and commented:
"I think you can make the mmap size (almost) arbitrarily large (say 256M), and rely on the kernel flushing pages back to disk under memory pressure. You're not burning actual memory, only virtual address space"

@MartijnKaijser MartijnKaijser added this to the Pending for inclusion milestone Feb 27, 2014
@jmarshallnz jmarshallnz modified the milestones: H* 14.0-alpha1, Pending for inclusion Mar 14, 2014
@jmarshallnz
Copy link
Contributor

@bavison: can this be done at runtime (the first commit)?

@bavison
Copy link
Contributor Author

bavison commented Apr 28, 2014

From the notes I made at the time, my first attempts did indeed use sqlite3_config(), but they weren't successful - they didn't have any statistically significant effect on timings. Part of the problem appears to have been that according to sqlite3.h, this call needed to be made before sqlite3_initialize(). sqlite3_initialize() is not explicitly called from anywhere in XBMC, but it is called from within sqlite3_open_v2(), which is called from dbiplus::SqliteDatabase::connect(). But that's called many times during XBMC startup, so there's no obvious place to put one-time initialisation of the SQLite engine as a whole, and a global "have we configured SQLite yet" flag seemed like a worse solution than changing a predefine.

@MartijnKaijser MartijnKaijser removed this from the Pending for inclusion milestone Jun 14, 2015
@MartijnKaijser
Copy link
Member

@bavison @popcornmix what's the story on this PR?

@popcornmix
Copy link
Member

I believe this PR gives a performance benefit and has been included in OpenELEC for over a year without issue:
OpenELEC/OpenELEC.tv@c1cb4a1
OpenELEC/OpenELEC.tv@59868b5

I think it should be merged. @bavison can you rebase?

@mkortstiege
Copy link
Member

@bavison, mind rebasing?

@stefansaraev
Copy link
Contributor

I picked this work in #7926

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants