Optimize FFI: Use unsafe FFI calling convention for column data accessors #20

Merged
merged 1 commit into from Dec 19, 2012

Projects

None yet

3 participants

@nurpax
Collaborator
nurpax commented Dec 18, 2012

FFI calling convention for C functions defaults to 'safe' which incurs
a heavy per-call overhead. Use of 'unsafe' should be fine for the
column data accessors as they do not callback to Haskell from C.
Furthermore, no I/O should be happening in these accessors, all these
functions do is move data over from C land to Haskell.

Performance delta in a synthetic benchmark such as
https://github.com/nurpax/db-bench is big -- almost 3x. The simplest
benchmark case speeds up from 2.4M rows/sec to 7.2M rows/sec. The
nice thing about this is that direct-sqlite now also beats Python
sqlite3 module by a factor of 2.8x whereas before they were about the
same speed.

@nurpax
Collaborator
nurpax commented Dec 18, 2012

This is a result of various work that was done with @Emm on sqlite-simple. @joeyadams might want to review this change too.

I think this is a safe change - I wanted to put this up for review while I dig up more information about the "unsafe" calling convention.

You will see I didn't make all functions into the C bindings unsafe. I think most of them could be unsafe, but it might be better to use it sparingly so that future changes have less chance to break things.

@nurpax
Collaborator
nurpax commented Dec 18, 2012

I found this article about unsafe: http://blog.ezyang.com/2010/07/safety-first-ffi-and-threading/

The kneejerk reaction is to panic about "unsafe" but reading Simon Marlow's first comment suggests that unsafe definitely has its uses.

@joeyadams
Contributor

Thanks for backing up this change with a benchmark.

However, sqlite3_step should definitely not be marked unsafe. It does the heavy lifting for the query. If the query takes a long time to return a row (e.g. SELECT sum(x) FROM foo), it could block the whole RTS during the operation.

Strictly speaking, some of the column accessors shouldn't be marked unsafe either, since they take a mutex lock. If one thread holds the sqlite3 mutex for a long time, then another thread does an unsafe sqlite3_column_type call, that could block the whole RTS until the first thread releases the lock. Fortunately, this is only a problem if the caller accesses the Database object from multiple threads, and does long-running queries with the same object. That wouldn't be a good idea anyway.

Functions that should definitely be marked safe, in my opinion:

  • sqlite3_step: blocks during query operation
  • sqlite3_exec: executes a sequence of queries, and may call back into Haskell
  • sqlite3_open, sqlite3_prepare_v2, sqlite3_close: not called enough to be worth the risk.

I think we can make the rest unsafe.

@nurpax
Collaborator
nurpax commented Dec 18, 2012

Oh, I actually didn't mean to make sqlite3_step unsafe - it accidentally slipped in this change. I'll re-run all my benchmarking without it and report back here.

Using the same Database object from multiple threads sounds like a bit of a bad idea anyway. This may open up opportunities for nasty bugs like trying to use a single prepared statement from multiple threads - this is not safe.

Agreed on your list of "definitely safe" functions.

I'll update this PR.

@nurpax
Collaborator
nurpax commented Dec 18, 2012

There's definitely a difference between safe & unsafe step.

safe:

benchmarking sqlite-simple: SELECT Ints
mean: 6.278979 ms, lb 6.167044 ms, ub 6.410193 ms, ci 0.950
std dev: 622.3236 us, lb 521.5497 us, ub 758.5229 us, ci 0.950
found 5 outliers among 100 samples (5.0%)
5 (5.0%) high mild
variance introduced by outliers: 78.982%
variance is severely inflated by outliers

unsafe:

benchmarking sqlite-simple: SELECT Ints
mean: 4.222646 ms, lb 4.125593 ms, ub 4.390202 ms, ci 0.950
std dev: 637.1770 us, lb 438.5706 us, ub 1.152351 ms, ci 0.950
found 1 outliers among 100 samples (1.0%)
1 (1.0%) high severe
variance introduced by outliers: 90.432%
variance is severely inflated by outliers

It's still around 2x faster than what we started with, so a good win anyway.

@nurpax nurpax Use unsafe FFI calling convention for sqlite3 entry points
FFI calling convention for C functions defaults to 'safe' which incurs
a heavy per-call overhead.  Use of 'unsafe' should be fine for several
of the sqlite3 column and binding accessors as they do not callback to
Haskell from C.  Furthermore, no I/O should be happening in these
accessors, all these functions do is move data over from C land to
Haskell.

The less frequently entry points are left in default 'safe' mode.
This is just a defensive move for future code maintenance.

Performance delta in a synthetic benchmark such as
https://github.com/nurpax/db-bench is significant.  Benchmarking the
direct-sqlite 'columns' call, we can see an improvement from 1.2M
rows/sec to 2.9M rows/sec.  The nice thing about this is that
direct-sqlite now also beats Python sqlite3 module by about 50%.
309536f
@nurpax
Collaborator
nurpax commented Dec 18, 2012

Force push updated the change (above).

From the commit message, updated perf results:

Performance delta in a synthetic benchmark such as
https://github.com/nurpax/db-bench is significant.  Benchmarking the
direct-sqlite 'columns' call, we can see an improvement from 1.2M
rows/sec to 2.9M rows/sec.  The nice thing about this is that
direct-sqlite now also beats Python sqlite3 module by about 50%.

Ran both direct-sqlite and sqlite-simple unit test suites on this change too, both passed.

P.S. As further incentive to merge this PR, I'm happy to inform reviewers that in my benchmark, direct-sqlite is now 34x faster than hdbc-sqlite. :)

@IreneKnapp
Owner

Whoa, a lot of conversation. There's a point I'd like to raise before I accept this change, although I'm inclined to do so anyway once it's been discussed.

Okay, so at present, we don't expose the portion of the sqlite API that lets us define custom SQL functions. But in principle, there could be arbitrary callbacks happening, which would indeed be C calling into Haskell. Now, it is my suspicion that these are all taken care of by sqlite3_step, which I see from the above discussion is kept safe, with the actual column accessors only reading out already-computed values. It would be great to have some confirmation of that though.

No, you know what, I'm going to merge this now and do a release as well. Everyone's done good work on the benchmarking and stuff, and I don't think the above is a serious concern. Still I'm leaving it in because I'd like there to be some record of it.

@IreneKnapp IreneKnapp merged commit c1bff45 into IreneKnapp:master Dec 19, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment