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

Add GVL mode #39

Closed
wants to merge 8 commits into from
Closed

Add GVL mode #39

wants to merge 8 commits into from

Conversation

noteflakes
Copy link
Contributor

@noteflakes noteflakes commented Dec 20, 2023

In response to #38, this PR adds three different GVL modes for use when running queries:

  • :hybrid - the GVL is released when preparing a stmt, when fetching the first record, and held on subsequent records. This is the default mode.
  • :release - the GVL is released when preparing a stmt and on fetching each record.
  • :hold - the GVL is held while preparing a stmt and fetching records.

The backup API always releases the GVL while backing up the DB.

@noteflakes noteflakes marked this pull request as ready for review December 21, 2023 08:19
@noteflakes
Copy link
Contributor Author

Hey @gschlager would you like to have a look and tell me what you think?

Some more thoughts:

  • An additional feature we might want to add is the ability to release the GVL every X rows, e.g.:

    db.gvl_release_threshold = 1000 # release GVL after reading 1000 records
  • I'm still not sure about the DX as relates to the API. It might make more sense to have the gvl mode as a global method, e.g. Extralite.gvl_mode = :hold, instead of per database.

  • If we add the gvl_release_threshold setting, we might as well just keep the hybrid mode and get rid of the other modes, since they can both be done by just changing the threshold.

@gschlager
Copy link
Collaborator

Overall I like these changes. Though the tests look a bit brittle and it doesn't seem like the tests are able to detect the difference between :release and :hybrid right now.

I kinda like the idea of having a gvl_release_threshold and I think it's fine to keep only the :hybrid mode in this case. I like the flexibility of it. But lets suggest good values for various use cases in the documentation.

I'd say having the setting on the DB instead of globally is a good choice. I can imagine a use case where an app switches between single and multithreading and in that case it might make sense to configure this on the DB connection level instead of globally.

@noteflakes
Copy link
Contributor Author

Though the tests look a bit brittle and it doesn't seem like the tests are able to detect the difference between :release and :hybrid right now.

I agree. I couldn't find a way to make SQLite sleep or make a long calculation on each row. Any idea?

@gschlager
Copy link
Collaborator

I agree. I couldn't find a way to make SQLite sleep or make a long calculation on each row. Any idea?

Unfortunately not. I tried a couple of things, but :hybrid mode seems impossible to test reliably. I wonder if the tests are any good if they can't reliably test the gvl_mode.

@fractaledmind
Copy link

@dbackeus shared this technique with me for getting SQLite to "sleep":

-- Since SQLite doesn't have a sleep function, we'll use a recursive CTE
-- which takes approximately 1 second to execute.
WITH RECURSIVE r(i) AS (
  VALUES(0)
  UNION ALL
  SELECT i FROM r
  LIMIT 10000000
)
SELECT i FROM r WHERE i = 1;

@noteflakes
Copy link
Contributor Author

Closed in favor of #46.

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