Skip to content

wrap SqliteCodebase.syncInternal in an immediate transaction #1966

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

Merged
merged 3 commits into from
May 17, 2021

Conversation

aryairani
Copy link
Contributor

I was thinking that sqlite could keep more work in RAM if it knows that it's expected to be an atomic operation, and it seems it can. Syncing base was 39% faster with this.

Loose ends

I wondered if PRAGMA read_only would help too, and I stop trying when I got an error about not being able to "prepare" the statement PRAGMA read_only = ? for some reason. You could just have constant statements for = TRUE" and = FALSE"` but I stopped trying. One could also just ask the sqlite mailing list if this would speed things up, they seem to be very knowledgeable.

Timing data

Using this transcript, where base_v2 is a local clone of unisonweb/base@de5f04d and not in the ucm git cache, and repo is an empty repo.

```ucm:hide
.> builtins.merge
.> builtins.mergeio
.> pull /root/base_v2:.trunk base
.> fork base newbase
.> merge builtin newbase
.> cd newbase
.newbase> push /root/unison-slim/repo
.newbase> delete.term Either.mapRight#jruvg9gh6q
.newbase> delete.term List.map#j1ejquc7so
.newbase> push /root/unison-slim/repo
```

Pre-PR (avg of 3 runs)

stage time
pull 14.05 s
push 1 14.37 s
push 2 0.85 s

With transaction on dest DB (avg of 3)

stage time
pull 8.99 s (-36%)
push 1 9.27 s (-36%)
push 2 0.85 s

With transaction on src and dest DBs (avg of 7)

stage time
pull 8.53 s (-39%)
push 1 8.90 s (-38%)
push 2 0.68 s (-20%)

@aryairani
Copy link
Contributor Author

aryairani commented May 16, 2021

CI is failing; I'll look into it later.

Copy link
Member

@pchiusano pchiusano left a comment

Choose a reason for hiding this comment

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

Sweet!

@pchiusano pchiusano closed this May 17, 2021
@pchiusano pchiusano reopened this May 17, 2021
@pchiusano pchiusano added the ready-to-merge Apply this to a PR and it will get merged automatically once CI passes and 1 reviewer has approved label May 17, 2021
@mergify mergify bot merged commit 878a873 into trunk May 17, 2021
@mergify mergify bot deleted the topic/sync-in-transaction branch May 17, 2021 01:11
@mergify mergify bot removed the ready-to-merge Apply this to a PR and it will get merged automatically once CI passes and 1 reviewer has approved label May 17, 2021
@pchiusano pchiusano mentioned this pull request Jun 4, 2021
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.

2 participants