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

SQLiteJournal: performance improvements #34

Closed
wants to merge 1 commit into from

Conversation

@sallyx
Copy link
Contributor

sallyx commented Oct 24, 2015

SQLiteJournal: performance improvements

@sallyx sallyx force-pushed the sallyx:optimize-sqlitejournal branch from e32571a to 4e8833f Oct 24, 2015
@@ -32,7 +32,8 @@ public function __construct($path)
$this->pdo = new \PDO('sqlite:' . $path);
$this->pdo->setAttribute(\PDO::ATTR_ERRMODE, \PDO::ERRMODE_EXCEPTION);
$this->pdo->exec('
PRAGMA foreign_keys = ON;
PRAGMA foreign_keys = OFF;

This comment has been minimized.

Copy link
@milo

milo Oct 24, 2015

Member

Good point when there are no foreign keys.

@@ -41,10 +42,9 @@ public function __construct($path)
key BLOB NOT NULL,
priority INT NOT NULL
);
CREATE INDEX IF NOT EXISTS idx_tags_key ON tags(key);

This comment has been minimized.

Copy link
@milo

milo Oct 24, 2015

Member

This will slow down writing. Or not?

This comment has been minimized.

Copy link
@sallyx

sallyx Oct 24, 2015

Author Contributor

No, removing this index speed up writting.

This comment has been minimized.

Copy link
@milo

This comment has been minimized.

Copy link
@sallyx

sallyx Oct 24, 2015

Author Contributor

SQLite can use idx_tags_key_tag index for searching in key column. Therefore idx_tags_key is needless.

This comment has been minimized.

Copy link
@milo

milo Oct 24, 2015

Member

Oh, that's true.

CREATE INDEX IF NOT EXISTS idx_tags_tag ON tags(tag);
CREATE UNIQUE INDEX IF NOT EXISTS idx_tags_key_tag ON tags(key, tag);
CREATE INDEX IF NOT EXISTS idx_priorities_key ON priorities(key);
CREATE UNIQUE INDEX IF NOT EXISTS idx_priorities_key ON priorities(key);

This comment has been minimized.

Copy link
@milo

milo Oct 24, 2015

Member

👍

@@ -32,7 +32,8 @@ public function __construct($path)
$this->pdo = new \PDO('sqlite:' . $path);
$this->pdo->setAttribute(\PDO::ATTR_ERRMODE, \PDO::ERRMODE_EXCEPTION);
$this->pdo->exec('
PRAGMA foreign_keys = ON;
PRAGMA foreign_keys = OFF;
PRAGMA journal_mode = WAL;

This comment has been minimized.

Copy link
@milo

milo Oct 24, 2015

Member

Good idea. SQLite 3.7 should be there since PHP 5.4. Personally, I would add PRAGMA synchronous = OFF; too.

This comment has been minimized.

Copy link
@sallyx

sallyx Oct 28, 2015

Author Contributor

I'd rather see some configuration option for synchronous = OFF/NORMAL/FULL;

@milo

This comment has been minimized.

Copy link
Member

milo commented Oct 24, 2015

Could you change commit message to be consistent and add test to priority bugfix?

@sallyx

This comment has been minimized.

Copy link
Contributor Author

sallyx commented Oct 24, 2015

I have updated the test file already.

@Majkl578

This comment has been minimized.

Copy link
Contributor

Majkl578 commented Oct 25, 2015

fixed bug - keys with multiple priority

This should be in separate PR (or commit at least).

disabled foreign keys check
removed unnecessary indices
enabled journal_mode WAL
@sallyx sallyx force-pushed the sallyx:optimize-sqlitejournal branch from 4e8833f to cd0583a Oct 28, 2015
@sallyx

This comment has been minimized.

Copy link
Contributor Author

sallyx commented Oct 28, 2015

I have created a new PR #35 for the bug fix.

@milo

This comment has been minimized.

Copy link
Member

milo commented Oct 30, 2015

With following synthetic benchmark:

use Nette\Caching\Cache;
use Nette\Caching\Storages\SQLiteJournal;

require __DIR__ . '/../caching/vendor/autoload.php';

$keys = $tags = $dels = [];
foreach (range(1, 200) as $i) {
    $keys[] = "key-$i";
    $tags[] = "tag-$i";
    if (($i % 4) === 0) $dels[] = "tag-$i";
}

$journal = new SQLiteJournal(__DIR__ . '/bench.s3db');

foreach (range(1, 10) as $loop) {
    # One key, multiple tags
    $journal->write('key', [Cache::TAGS => $tags]);

    # n-keys, one tag
    foreach ($keys as $key) {
        $journal->write($key, [Cache::TAGS => ['tag']]);
    }

    # Delete some of tags
    $journal->clean([Cache::TAGS => $dels]);
}

Apache sequential bench:

$ rm -f bench.s3db
$ ab -c 1 -n 10 http://localhost/dev/nette/caching.tests/bench.php

# Before patch
Connect:        0    0   0.0      0       0
Processing:  3906 5036 836.1   5288    6365
Waiting:     3906 5036 836.1   5288    6365
Total:       3906 5036 836.1   5288    6365

# With patch
Connect:        0    0   0.0      0       0
Processing:   686  783 149.3    704    1145
Waiting:      686  783 149.3    704    1145
Total:        686  783 149.3    704    1145

Apache parallel bench:

$ rm -f bench.s3db
$ ab -c 8 -n 10 http://localhost/dev/nette/caching.tests/bench.php

# Before patch
Connect:        0    0   0.1      0       0
Processing:  6567 36465 16157.5  45939   51092
Waiting:     6567 36465 16157.5  45939   51092
Total:       6567 36465 16157.6  45939   51092

# With patch
Connect:        0    0   0.1      0       0
Processing:  2739 4437 1425.3   4443    6845
Waiting:     2739 4437 1425.3   4443    6845
Total:       2740 4437 1425.3   4444    6845

So 👍 for merge.

@milo

This comment has been minimized.

Copy link
Member

milo commented Oct 30, 2015

@sallyx Could you change the commit message to something like: SQLiteJournal: performance improvements

@sallyx sallyx changed the title Optimized SQLiteJournal SQLiteJournal: performance improvements Oct 30, 2015
@dg

This comment has been minimized.

Copy link
Member

dg commented Nov 4, 2015

@sallyx not title but commit message

@dg dg closed this in 96e1d58 Nov 29, 2015
dg added a commit that referenced this pull request Nov 29, 2015
disabled foreign keys check
removed unnecessary indices
enabled journal_mode WAL
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.