Future API changes to consider

[cberner]

• replace drain and drain_filter with extract_if to match BtreeMap. Waiting on the std API to stabilize, so we can match it. Tracking Issue for {BTreeMap,BTreeSet}::extract_if rust-lang/rust#70530
• constant time API to get the length of a MultimapValue
• optimize Table::len() by storing the number of entries in a counter. See len is slow #710
• Introduce a common trait for all tables, including untyped ones, that has the stats() and len() methods
• Implement len() for untyped tables
• Rename RedbKey to Key and RedbValue to Value
• Remove lifetimes from as many public structs as possible

[Gawdl3y]

I'm eager to see a lot of the lifetimes being removed from public structs, as I believe the lifetimes are making downstream use of vincent-herlemont/native_db kludgier than it could be. See vincent-herlemont/native_db#69 and vincent-herlemont/native_db#71. The main struct causing the troubles there is redb::TableDefinition.

[Ralith]

The lifetime relationships of ReadTransaction and ReadOnlyTable forced me to expose two layers of guard objects in https://github.com/Ralith/hypermine/blob/master/save/src/lib.rs, which is a mildly annoying wart in an otherwise ergonomic encapsulation of redb.

[cberner]

@Ralith can you give master a try and see if the new 'static lifetimes resolves your issue?

[Ralith]

That avoids the double-guard for reads, but it looks like the problem still exists for writes. Not sure how solvable that is without making things more dynamic, though.

[cberner]

I plan to remove those too. Just haven't gotten to it yet

…

Sent from my phone

On Sat, Jan 27, 2024, 11:12 PM Benjamin Saunders ***@***.***> wrote: That avoids the double-guard for reads, but it looks like the problem still exists for writes. Not sure how solvable that is without making things more dynamic, though. — Reply to this email directly, view it on GitHub <#677 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGNXQFNVHC4UQLEV5O5LB3YQX25ZAVCNFSM6AAAAAA4KWIGKCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMJTGQ4DEMBQGI> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[eserilev]

Recently I was trying to do something like

 let iter = {
            let read_txn = self.db.begin_read().unwrap();
            let table = read_txn.open_table(table_definition).unwrap();
            table.range::<&[u8]>(from..).unwrap().map(|res| {
                let (k, v) = res.unwrap();
                Ok((
                    K::from_bytes(k.value()).unwrap(),
                    v.value().to_vec(),
                ))
            })
        }

because of lifetime issues I couldn't find a way to return iter cleanly. The static lifetime changes currently on master are a huge game changer for us. Really excited for this to land!

[cberner]

master now has much friendlier lifetimes

[Evrey]

For future changes i’d like to also suggest this one:

• Replace all occurrences of String with beef::Cow<'static, str> or core::borrow::Cow<'static, str> where possible.

This would obviously be a breaking change, hence »future change«.

I’m suggesting this, because currently there are a bunch of completely useless String allocations being performed in my code base. I only use const table definitions with &'static str table names, so all the error paths can take the name from my table definition directly, resulting in e.g. TableError::TableDoesNotExist(Cow<'static, str>). To make it worse, usually i throw these String names away immediately, because when i am handling errors, i know which table i just opened and don’t need a copy of the name.

[cberner]

Unfortunately, I don't think that would work. The call site can have a non 'static lifetime:

redb/src/transactions.rs

Line 1212 in 222a37f

.ok_or_else(|| TableError::TableDoesNotExist(definition.name().to_string()))?;

[Evrey]

Well, yes. I’m suggesting to borrow the name when it’s 'static, and only allocate it when it’s not. Alternatively, have TableErrors bound to the table definition’s lifetime (which i’d say makes sense to do), storing borrowed &'a strs, and have an impl<'a> Into<TableError<'static>> from TableError<'a> that allocates all Cow<'_, str> names, erasing the original lifetime.

[cberner]

I’m suggesting to borrow the name when it’s 'static, and only allocate it when it’s not.

Do you have an example of the code for that? I'm not sure how to make it conditional on whether the lifetime is 'static or not

[Evrey]

Oh right. I’ve been surfing on too much nightly Rust and forgot you can’t specialise over lifetimes.

[Ralith]

I plan to remove [write transaction lifetimes] too. Just haven't gotten to it yet

It looks like a double-guard is still needed for write transactions: WriteTransaction::open_table borrows from the transaction, which means that an encapsulating API cannot store both the transaction and the tables in the same object.

[cberner]

Yes, that's correct. If you need it to be lifetime-less, I would recommend opening the tables on-demand rather than retaining them.

[Ralith]

That would work, but I hesitated since the cost of open_table isn't clear. What's a good mental model here?

[cberner]

It's log N in the number of tables in the database, and for repeat opens of the same table should end up being about as expensive as a few HashMap lookups

[Evrey]

Those are good numbers to document in APIs.