chore: specialize `Encodable`/`Decodable` for bytes #4145

dpc · 2024-01-26T19:56:04Z

Due to Rust limitations, we can't have impls for both Vec<T> and Vec<u8>. Handling byte case in a generic way is potentially very inefficient, and we do use it in some places, thought it's hard to find all of them (and then prevent even more from being introduced).

The most appealing approach seems to be specializing that case using TypeId check and unsafe.

Cons:

TypeId requires 'static bound
unsafe (but small and easy to reason about)

Pros:

zero cost (at runtime)
works automatically everywhere
no loss in ergonomics

Edit:

In idle just mprocs I can't tell if there's any performance difference.

dpc · 2024-01-26T20:01:46Z

fedimint-core/src/encoding/mod.rs

+    chunk_size: usize,
+}
+
+/// Read `opts.len` bytes from reader, where `opts.len` could potentially be


This reminds me I need to port https://github.com/rust-bitcoin/rust-bitcoin/blob/e2b9555070d9357fb552e56085fb6fb3f0274560/bitcoin/src/consensus/encode.rs#L304 to Fedimint ...

codecov · 2024-01-26T20:04:34Z

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (f7463d3) 58.10% compared to head (154b804) 58.11%.

Files	Patch %	Lines
fedimint-core/src/encoding/mod.rs	95.89%	3 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #4145   +/-   ##
=======================================
  Coverage   58.10%   58.11%           
=======================================
  Files         192      192           
  Lines       42981    43054   +73     
=======================================
+ Hits        24976    25020   +44     
- Misses      18005    18034   +29

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Re fedimint#4111 Re rust-bitcoin/rust-bitcoin#2390 Due to Rust limitations, we can't have impls for both `Vec<T>` and `Vec<u8>`. Handling byte case in a generic way is potentially *very* inefficient, and we do use it in some places, thought it's hard to find all of them (and then prevent even more from being introduced). The most appealing approach seems to be specializing that case using `TypeId` check and `unsafe`. Cons: * `TypeId` requires `'static` bound * unsafe (but small and easy to reason about) Pros: * zero cost (at runtime) * works automatically everywhere * no loss in ergonomics

elsirion

In a not very scientific test this made running fedimint-dbtool dump 18% faster.

elsirion · 2024-01-27T06:17:57Z

fedimint-core/src/encoding/mod.rs

+        if TypeId::of::<T>() == TypeId::of::<u8>() {
+            // unsafe: we've just checked that T is `u8` so the transmute here is a no-op
+            return consensus_encode_bytes(unsafe { mem::transmute::<&[T], &[u8]>(self) }, writer);
+        }


This is a really cool pattern! 🤯

elsirion · 2024-01-27T06:24:58Z

fedimint-core/src/encoding/mod.rs

+    let len: usize =
+        usize::try_from(len).map_err(|_| DecodeError::from_str("size exceeds memory"))?;


This should only error on <64bit platforms, right?

This errors if trying to decode byte array larger than total theoretical virtual memory available for the process (which we wouldn't be able to allocate anyway).

Right, but for all reasonable platforms these days u64==usize

dpc · 2024-01-27T22:36:01Z

In a not very scientific test this made running fedimint-dbtool dump 18% faster.

I'm happy to hear you were able to confirm this improves something. :D

tcharding · 2024-01-28T19:40:40Z

TypeId requires 'static bound

That is a pretty big con, right? In a real life program when would one decode static things? (Excuse my ignorance but the only 'static decodable things are static strings in tests, are they not?)

dpc · 2024-01-28T22:46:05Z

That is a pretty big con, right? In a real life program when would one decode static things? (Excuse my ignorance but the only 'static decodable things are static strings in tests, are they not?)

AFAIR T: 'static means "any references held by this type need to be of static lifetime. Doesn't seem like a big con. It would interefere with zero-copy de/serialization, I guess. I mean - con is a con. Fedimint not being a general purpose library, might be OK with it.

maan2003 · 2024-01-29T06:21:19Z

fedimint-core/src/encoding/mod.rs

@@ -336,14 +431,29 @@ where
    }
 }

+// From <https://github.com/rust-lang/rust/issues/61956>
+unsafe fn horribe_array_transmute_workaround<const N: usize, A, B>(mut arr: [A; N]) -> [B; N] {


#[inline(always)] to make array doesn't have to be copied

maan2003 · 2024-01-29T06:39:49Z

fedimint-core/src/encoding/mod.rs

 {
    fn consensus_decode<D: std::io::Read>(
        d: &mut D,
        modules: &ModuleDecoderRegistry,
    ) -> Result<Self, DecodeError> {
+        if TypeId::of::<T>() == TypeId::of::<u8>() {
+            // unsafe: we've just checked that T is `u8` so the transmute here is a no-op
+            return Ok(unsafe { mem::transmute::<Vec<u8>, Vec<T>>(consensus_decode_bytes(d)?) });


wait you can't transmute Vec you have to go through {to, from}_raw methods

see https://doc.rust-lang.org/nomicon/transmutes.html?highlight=Vec%20transmute#transmutes

actually it is correct, because T is u8

maan2003 · 2024-01-29T06:51:01Z

I think we can remove unsafe from this, using dyn Any api

maan2003 · 2024-01-29T07:28:22Z

yes, I removed a lot of unsafe in 6ee8e87

but for &[T], rust complains about 'static

maan2003 · 2024-01-29T07:42:17Z

fedimint-core/src/encoding/mod.rs

+        .map_err(DecodeError::from_err)?;
+    Ok(bytes)
+}
+
 impl_encode_decode_tuple!(T1, T2);
 impl_encode_decode_tuple!(T1, T2, T3);
 impl_encode_decode_tuple!(T1, T2, T3, T4);

 impl<T> Encodable for &[T]


we should impl for [T] instead of &[T]

but using Any still doesn't work
because you can't do .downcast_ref::<[u8]>(), because downcast_ref needs Sized

we could impl for Vec without unsafe, but not for Box<[u8]> or Arc, Rc

Kixunil · 2024-01-29T13:17:17Z

To shed more light on why 'static is required: Rust doesn't want to allow non-'static casting because it's not clear how type IDs should interact with lifetimes. Also while apparently there could be a safe downcasting function it's not expressible with current syntax (at least I think so):

fn downcast_ref<'a, T, U>(value:: &'a T) -> Option<&'a U> { /* ... */ }

There needs to be a bound requiring that T outlives U (assuming T and U are covariant) to ban this code:

fn do_stuff<T>(value: &T, s: &mut &'static str) {
    *s = *downcast_ref(value).unwrap();
}

But such bound is currently inexpressible.

This PR wold be sound even without the 'static bound because instead of a generic U it uses a compile time known type u8, which never has any references (this is not the same as 'static!) and thus always outlives itself. (So yes, having an imagined U: 'always_static bound above would work too but it'd still be more restrictive than necessary.) However there's no way to check if T is u8 because you can't obtain TypeId for non-'static T.

dpc · 2024-01-29T16:47:13Z

@Kixunil 100% matches my understanding.

BTW. If Rust had something like TypeId::get_static<T> -> Option<TypeID> then one could have a cake and eat it, no T: 'static would be required - it would just return None.

dpc · 2024-01-29T16:49:58Z

@maan2003 I don't see a reason to worry about wrapping this unsafe into anything. These are the only places where it seems worthwhile and it's already small, isolated and easy to check that it's correct.

maan2003

Agreed

maan2003 · 2024-01-30T04:00:18Z

wow github doesn't like this pr, maybe we broke something

dpc · 2024-01-30T04:33:44Z

wow github doesn't like this pr, maybe we broke something

It already passed before the CI. It must be some flakiness on master.

dpc force-pushed the 24-01-26-fec-u8-perf branch from fc45773 to 970849b Compare January 26, 2024 19:56

dpc commented Jan 26, 2024

View reviewed changes

dpc force-pushed the 24-01-26-fec-u8-perf branch from 970849b to 0233fda Compare January 26, 2024 20:33

dpc mentioned this pull request Jan 26, 2024

observation: rust-bitcoin does not impl Encodable for Vec<T> because they prefer Vec<u8> specialization for perf. reasons. #4111

Closed

dpc marked this pull request as ready for review January 26, 2024 20:38

dpc requested a review from a team as a code owner January 26, 2024 20:38

dpc force-pushed the 24-01-26-fec-u8-perf branch from 0233fda to 154b804 Compare January 26, 2024 20:46

elsirion approved these changes Jan 27, 2024

View reviewed changes

elsirion enabled auto-merge January 27, 2024 09:28

dpc mentioned this pull request Jan 27, 2024

Test the Encodable impl for Vec<T> where T: Encodable rust-bitcoin/rust-bitcoin#2390

Open

maan2003 reviewed Jan 29, 2024

View reviewed changes

maan2003 approved these changes Jan 29, 2024

View reviewed changes

elsirion added this pull request to the merge queue Jan 29, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 29, 2024

dpc added this pull request to the merge queue Jan 30, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 30, 2024

dpc added this pull request to the merge queue Jan 30, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 30, 2024

maan2003 added this pull request to the merge queue Jan 30, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 30, 2024

dpc added this pull request to the merge queue Jan 30, 2024

Merged via the queue into fedimint:master with commit 8dd5331 Jan 30, 2024
21 of 22 checks passed

dpc deleted the 24-01-26-fec-u8-perf branch January 30, 2024 04:44

bradleystachurski mentioned this pull request Feb 29, 2024

fix: make Strings in meta backwards compatible #4438

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: specialize `Encodable`/`Decodable` for bytes #4145

chore: specialize `Encodable`/`Decodable` for bytes #4145

dpc commented Jan 26, 2024 •

edited

dpc Jan 26, 2024

codecov bot commented Jan 26, 2024 •

edited

elsirion left a comment

elsirion Jan 27, 2024

elsirion Jan 27, 2024

dpc Jan 27, 2024

elsirion Jan 29, 2024

dpc commented Jan 27, 2024

tcharding commented Jan 28, 2024

dpc commented Jan 28, 2024 •

edited

maan2003 Jan 29, 2024

maan2003 Jan 29, 2024

maan2003 Jan 29, 2024

maan2003 Jan 29, 2024

maan2003 commented Jan 29, 2024

maan2003 commented Jan 29, 2024 •

edited

maan2003 Jan 29, 2024

maan2003 Jan 29, 2024

maan2003 Jan 29, 2024

Kixunil commented Jan 29, 2024

dpc commented Jan 29, 2024 •

edited

dpc commented Jan 29, 2024

maan2003 left a comment

maan2003 commented Jan 30, 2024

dpc commented Jan 30, 2024

		let len: usize =
		usize::try_from(len).map_err(\|_\| DecodeError::from_str("size exceeds memory"))?;

chore: specialize Encodable/Decodable for bytes #4145

chore: specialize Encodable/Decodable for bytes #4145

Conversation

dpc commented Jan 26, 2024 • edited

Choose a reason for hiding this comment

codecov bot commented Jan 26, 2024 • edited

Codecov Report

elsirion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpc commented Jan 27, 2024

tcharding commented Jan 28, 2024

dpc commented Jan 28, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maan2003 commented Jan 29, 2024

maan2003 commented Jan 29, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kixunil commented Jan 29, 2024

dpc commented Jan 29, 2024 • edited

dpc commented Jan 29, 2024

maan2003 left a comment

Choose a reason for hiding this comment

maan2003 commented Jan 30, 2024

dpc commented Jan 30, 2024

chore: specialize `Encodable`/`Decodable` for bytes #4145

chore: specialize `Encodable`/`Decodable` for bytes #4145

dpc commented Jan 26, 2024 •

edited

codecov bot commented Jan 26, 2024 •

edited

dpc commented Jan 28, 2024 •

edited

maan2003 commented Jan 29, 2024 •

edited

dpc commented Jan 29, 2024 •

edited