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

Rework import order for SQLCipher; add test conditionals for Android #1708

Open
wants to merge 8 commits into
base: development
Choose a base branch
from

Conversation

marcprux
Copy link

This is a less ambitious alternative to #1701 that enables adding a SQLCipher source dependency to the Package.swift based on the GRDBCIPHER environment variable. Tests can be run with the dependency with a command like:

GRDBCIPHER="https://github.com/skiptools/swift-sqlcipher.git#1.2.1" swift test

Running it without the environment variable will leave things exactly as they are. The majority of changes in this PR are simply re-ordering the import header checks for GRDBCIPHER and SWIFT_PACKAGE, since we would need the former to take precedence of the latter.

An advantage of doing it this way is that it facilitates creating a GRDB-SQLCipher fork that can follow the releases of the upstream. The only change in such a fork would be to hardwire the GRDBCIPHER setting in Package.swift to some SQLCipher source repository (either mine, or one that you maintain). When a client package wants to switch between GRDB and GRDB-SQLCipher, they would just need to swap their dependency URL.

This PR also adds in support building and testing against Android, mostly by stubbing out the unit tests that can't compile due to missing NSFileCoordinator, Combine, and the like. Android tests can be run with skip:

GRDBCIPHER="https://github.com/skiptools/swift-sqlcipher.git#1.2.1" skip android test

Most of the tests pass, but there are a few that need to be investigated. I'll look into them next.

Closes #1701

@marcprux
Copy link
Author

Addendum: out of curiosity, I ran some comparisons between the built-in SQLite and the SQLCipher build with the following script (and adding a GRDB_PERFORMANCE_TESTS check to enable GRDBPerformanceTests in Package.swift).

The results over a dozen runs are a fairly consistent slowdown of around 10% when using SQLCipher. It'd be interesting to tweak the SQLite build flags in swift-sqlcipher to see what might increase/decrease the performance.

echo "With GRDBCIPHER"; rm -r .build; GRDB_PERFORMANCE_TESTS=1 GRDBCIPHER="https://github.com/skiptools/swift-sqlcipher.git#1.2.1" swift test --filter GRDBPerformanceTests 2>&1 | grep --after-context=2 "Test Suite 'GRDBPackageTests.xctest' passed"
echo "Without GRDBCIPHER"; rm -r .build; GRDB_PERFORMANCE_TESTS=1 swift test --filter GRDBPerformanceTests 2>&1 | grep --after-context=2 "Test Suite 'GRDBPackageTests.xctest' passed"

With GRDBCIPHER
Test Suite 'GRDBPackageTests.xctest' passed at 2025-01-26 22:33:13.477.
	 Executed 23 tests, with 0 failures (0 unexpected) in 353.144 (353.146) seconds
Test Suite 'Selected tests' passed at 2025-01-26 22:33:13.477.
Without GRDBCIPHER
Test Suite 'GRDBPackageTests.xctest' passed at 2025-01-26 22:38:59.398.
	 Executed 23 tests, with 0 failures (0 unexpected) in 314.684 (314.686) seconds

@Joannis
Copy link

Joannis commented Jan 30, 2025

@marcprux the Android NDK doesn't seem to bundle sqlite3.h, whereas GRDB and your PR do rely on that being available. How are you providing that right now when building and testing GRDB?

@marcprux
Copy link
Author

@marcprux the Android NDK doesn't seem to bundle sqlite3.h, whereas GRDB and your PR do rely on that being available. How are you providing that right now when building and testing GRDB?

It is included with the SQLCipher source module: https://github.com/skiptools/swift-sqlcipher/tree/main/Sources/SQLCipher/sqlite

Copy link
Owner

@groue groue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @marcprux,

I like that very much. This pull request is a great gift 🎁! I'll shortly play with the new targets.

I made a first review and preliminary comments. I hope we agree on the broad outlines!

GRDB/Dump/Database+Dump.swift Outdated Show resolved Hide resolved
Tests/GRDBTests/DatabaseQueueTests.swift Outdated Show resolved Hide resolved
Tests/GRDBTests/FTS5TableBuilderTests.swift Outdated Show resolved Hide resolved
Tests/GRDBTests/FailureTestCase.swift Outdated Show resolved Hide resolved
Tests/GRDBTests/JSONColumnTests.swift Outdated Show resolved Hide resolved
Tests/GRDBTests/JSONExpressionsTests.swift Outdated Show resolved Hide resolved
Tests/GRDBTests/MutablePersistableRecordTests.swift Outdated Show resolved Hide resolved
Tests/GRDBTests/PersistableRecordTests.swift Outdated Show resolved Hide resolved
Tests/GRDBTests/ValueObservationRecorderTests.swift Outdated Show resolved Hide resolved
@groue
Copy link
Owner

groue commented Feb 1, 2025

The results over a dozen runs are a fairly consistent slowdown of around 10% when using SQLCipher. It'd be interesting to tweak the SQLite build flags in swift-sqlcipher to see what might increase/decrease the performance.

Isn't it just the cipher? (EDIT: I'm wrong because performance tests don't run on encrypted databases, see comment below.)

SQLCipher can also be extremely slow when it opens a connection. Some people have reported up to 0.5 seconds (😱). To the point I had to ship #1350.

@groue
Copy link
Owner

groue commented Feb 1, 2025

I ran the same performance tests, but this time in release configuration:

echo "With GRDBCIPHER"; rm -rf .build; GRDB_PERFORMANCE_TESTS=1 GRDBCIPHER="https://github.com/skiptools/swift-sqlcipher.git#1.2.1" swift test --configuration release --filter GRDBPerformanceTests 2>&1 | grep --after-context=2 "Test Suite 'GRDBPackageTests.xctest' passed"
echo "Without GRDBCIPHER"; rm -rf .build; GRDB_PERFORMANCE_TESTS=1 swift test --configuration release --filter GRDBPerformanceTests 2>&1 | grep --after-context=2 "Test Suite 'GRDBPackageTests.xctest' passed"

With GRDBCIPHER
Test Suite 'GRDBPackageTests.xctest' passed at 2025-02-01 13:53:37.700.
	 Executed 23 tests, with 0 failures (0 unexpected) in 72.258 (72.260) seconds
Test Suite 'Selected tests' passed at 2025-02-01 13:53:37.700.
Without GRDBCIPHER
Test Suite 'GRDBPackageTests.xctest' passed at 2025-02-01 13:57:26.264.
	 Executed 23 tests, with 0 failures (0 unexpected) in 80.139 (80.141) seconds
Test Suite 'Selected tests' passed at 2025-02-01 13:57:26.264.

This time, SQLCipher tests are faster.

@marcprux
Copy link
Author

marcprux commented Feb 1, 2025

SQLCipher can also be extremely slow when it opens a connection

Agreed, but the performance hit should only occur when encryption is actually enabled on the database, which shouldn't affect any of the perf tests.

This time, SQLCipher tests are faster.

Of course! Foolish of me to not do that. Otherwise, it is comparing the debug build of the sqlcipher/sqlite source build against the release build of the vendored sqlite.

It is heartening to see that superior performance can be squeezed out of a source build. I wonder how much more might be managed with strategic performance-related flags (e.g., sbooth/CSQLite#59 (comment)).

@marcprux
Copy link
Author

marcprux commented Feb 6, 2025

Apologies for the long delay in addressing the feedback (I was traveling).

I believe I've handled each of the requested changes. Let me know if I can do anything else to help get this PR over the finish line – it has grown quite large, so I'm nervous that rebasing is going to be difficult if it starts to become stale.

@Joannis
Copy link

Joannis commented Feb 7, 2025

We've been using this branch successfully.

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