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

perf(sqlite): disable pagecache overflow stat #1017

Merged

Conversation

yuvalp-k2view
Copy link
Contributor

I suggest adding SQLITE_DISABLE_PAGECACHE_OVERFLOW_STAT to the compilation flags.

Maintaining this stat, requires a global lock, which reduces performance in multi thread applications.

Since we don't expose any API to see stats, this is rather safe.

The reasoning here is very similar to -DSQLITE_DEFAULT_MEMSTATUS=0 which is already part of our config flags.

@gotson
Copy link
Collaborator

gotson commented Nov 13, 2023

i can't find any mention of SQLITE_DISABLE_PAGECACHE_OVERFLOW_STAT on the sqlite website. Do you have some sources about this?

@yuvalp-k2view
Copy link
Contributor Author

yuvalp-k2view commented Nov 13, 2023

Got this from one of the sqlite maintainers as I was exploring contention when running in a multi thread multi connection scenario. With pure C sqlite I saw noticeable improvement when I compiled with this flag.

I can see the following in the sqlite code:

#ifndef SQLITE_DISABLE_PAGECACHE_OVERFLOW_STATS
    if( p ){
      int sz = sqlite3MallocSize(p);
      sqlite3_mutex_enter(pcache1.mutex);
      sqlite3StatusHighwater(SQLITE_STATUS_PAGECACHE_SIZE, nByte);
      sqlite3StatusUp(SQLITE_STATUS_PAGECACHE_OVERFLOW, sz);
      sqlite3_mutex_leave(pcache1.mutex);
    }
#endif

@yuvalp-k2view
Copy link
Contributor Author

Copying and pasting the code, I noticed I had the "define" wrong. Fixed and rebuilt correctly.

@pyckle
Copy link
Contributor

pyckle commented Nov 16, 2023

@gotson - there's some docs on status here: https://www.sqlite.org/c3ref/c_status_malloc_count.html
I don't think these are exposed, so there's no reason to compute them, especially if it can lead to performance issues. Based on a quick read, one would only need this status info to diagnose low level performance issues. This has yet to be a use case for this driver.

@gotson
Copy link
Collaborator

gotson commented Nov 16, 2023

the issue i have is that there is no documentation to back this change: https://www.sqlite.org/search?s=d&q=SQLITE_DISABLE_PAGECACHE_OVERFLOW_STATS

@yuvalp-k2view
Copy link
Contributor Author

Got this from Dan:

hadn't noticed it wasn't documented. Next time the website is published, this section:

https://sqlite.org/compile.html#_options_to_disable_features_normally_turned_on

Will include the text:

SQLITE_DISABLE_PAGECACHE_OVERFLOW_STATS:
This option disables the collection of the sqlite3_status() SQLITE_STATUS_PAGECACHE_OVERFLOW and SQLITE_STATUS_PAGECACHE_SIZE statistics. Setting this option has been shown to increase performance in high concurrency multi-threaded applications.

@yuvalp-k2view
Copy link
Contributor Author

@gotson, based on the above from sqlite dev, maybe we can go forward?

@gotson
Copy link
Collaborator

gotson commented Nov 23, 2023

@gotson, based on the above from sqlite dev, maybe we can go forward?

Can you commit only the change in the Makefile ? There's a new verison of sqlite out, and i will upgrade to that and release, so i will recompile the binaries anyway.

@yuvalp-k2view
Copy link
Contributor Author

Can you commit only the change in the Makefile ? There's a new verison of sqlite out, and i will upgrade to that and release, so i will recompile the binaries anyway.

@gotson done

@gotson gotson changed the title Sqlite disable pagecache overflow stat perf(sqlite): disable pagecache overflow stat Nov 27, 2023
@gotson gotson merged commit 31b7659 into xerial:master Nov 27, 2023
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.

3 participants