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

Upgrade redb to 0.12.0 #1329

Merged
merged 9 commits into from
Feb 1, 2023
Merged

Upgrade redb to 0.12.0 #1329

merged 9 commits into from
Feb 1, 2023

Conversation

casey
Copy link
Collaborator

@casey casey commented Jan 22, 2023

@cberner, just took a stab at upgrading to redb 0.12.0:

  • Some additional type hints were needed. I don't fully understand the tradeoffs here, and we might have discussed this before, but could get/insert/etc be changed to take the the final borrowed form, e.g. &[u8] instead of impl Borrow<[u8]>, in which case you could pass fewer things in, but the typing would be less confusing?

  • I couldn't get inscriptions_on_output working, which previously returned a borrowed iterator over a table, but now since the range start and end are references, it has to return a reference. I don't think this is critical for ord in this particular instance, but could be a problem in other cases where it isn't practical to collect into a vec.

  • I tried to switch to the non-mmap backend (btw did you ever wind up benchmarking it?), but after switching, the new/old_schema_gives_correct_error tests fail with a panic while panicking error, which abort the thread, and sadly don't give a good backtrace. These tests might be the only ones where we create a database, write to it, close it, and then open it again. Strangely though they only fail locally, so it could be an M1 mac/16kb page thing.

@cberner
Copy link
Contributor

cberner commented Jan 22, 2023

  1. it could, although you were the one that asked for passing both refs and owned values ;) 0.10.0 Feedback cberner/redb#448

  2. hmm, I'll take a look. I thought I had worked out the lifetimes such that the range bounds' lifetime didn't matter

  3. uh oh. That's quite odd because 4k vs 16k page size should no longer be a thing. Maybe I have a bug where it can still default to the native page size though:

@cberner
Copy link
Contributor

cberner commented Jan 22, 2023

Ah yes, (3) is this bug: cberner/redb#501 it happens in those tests and also if you reopen a database on MacOS with open() (doesn't happen with create())

@cberner
Copy link
Contributor

cberner commented Jan 22, 2023

I think this will fix (2): cberner/redb#502

@cberner
Copy link
Contributor

cberner commented Jan 22, 2023

Oh yes, I ran benchmarks. mmap is something like 20-50% faster for writes and 1.2x to 3x faster for reads depending on the number of concurrent threads. The biggest benefit is on highly concurrent read workloads, where mmap avoid lock contention by instead relying on unsafe

@casey casey marked this pull request as ready for review February 1, 2023 23:03
@casey casey enabled auto-merge (squash) February 1, 2023 23:05
@casey casey merged commit 11d2d2c into master Feb 1, 2023
@casey casey deleted the upgrade-redb branch February 1, 2023 23:13
@utxo-detective
Copy link

is there a specific way to upgrade the db or should we just index it again from block 1?

@casey
Copy link
Collaborator Author

casey commented Feb 6, 2023

is there a specific way to upgrade the db or should we just index it again from block 1?

Yup, just delete the db and re-run ord.

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