Make Addr::unchecked a const fn #869

webmaster128 · 2021-04-08T23:22:14Z

This allows you to use Addr::unchecked("foo") in all the places where you can use "foo", specially const VARIABLES. This uses the facts that:

We have a known max size and Addr can have a constant size. The memory layout can safely be copied.
In contract to String, Addr is immutable. So it does not matter if we move or copy.

yihuang · 2021-04-09T03:59:44Z

packages/std/src/addresses.rs

+        while i < input_bytes.len() {
+            data[i] = input_bytes[i];
+            i += 1;
+        }


Suggested change

while i < input_bytes.len() {

data[i] = input_bytes[i];

i += 1;

}

data[..input_bytes.len()].clone_from_slice(input.as_bytes());

I think we can do this here?

Here we are very limited in what is available because it must be const fn compatible. We can't even rewrite the while loop to a for i in 0...input_bytes.len() loop. clone_from_slice is not a const fn, so it cannot be used.

Thinking about optimizing it, I looked at the implementation of eg. https://doc.rust-lang.org/core/ptr/fn.copy_nonoverlapping.html but that just point to rust intrinsics, so I guess this is the best we can do

Wow – seems like we can use copy_nonoverlapping directly because it became const (rust-lang/rust#79684). For some reason this is not shown in the docs yet. Will try.

ethanfrey

Interesting approach. This does obviate the need for AddrRef, but at the same time creates a rather large array that must be copied every time we pass it through a function.

I am unsure of the benefits of this over using the AddrRef implementation I had, making AddrRef::unchecked(&str) a const fn and "promoting it" via Addr::from(addrRef). That would make the clone (copy data to heap) explicit and work well with the lifetimes we have in storage-plus.

We could then use AddrRef::unchecked("name") in all the test code and just name.into() when needed (no more unchecked). Can you explain the benefits of this approach?

ethanfrey · 2021-04-09T07:17:41Z

packages/std/src/addresses.rs

-#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, Eq, Hash, JsonSchema)]
-pub struct Addr(String);
+#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash, JsonSchema)]
+pub struct Addr(#[schemars(with = "String")] AddrInner);


The two level nesting here is for schemars output?

Eaxtly. If we find a better way to ensure it gets a "string" schema, we can simplify at any time because all the internals are private.

ethanfrey · 2021-04-09T07:20:48Z

packages/std/src/addresses.rs

+    where
+        E: de::Error,
+    {
+        if v.len() > ADDR_MAX_LENGTH {


Nice to assert this here.
Should we assert this in unchecked as well? Or would that violate the const fn (you could just panic there, but it would ensure nothing gets chopped off by accident)

ethanfrey · 2021-04-09T07:21:15Z

packages/std/src/addresses.rs

+        let mut data = [0u8; ADDR_MAX_LENGTH];
+        let mut i = 0;
+        while i < input_bytes.len() {
+            data[i] = input_bytes[i];


This can buffer overflow if we don't have a length assertion for input

ethanfrey · 2021-04-09T07:22:54Z

packages/std/src/addresses.rs

@@ -93,25 +166,25 @@ impl PartialEq<Addr> for String {

 impl From<Addr> for String {
    fn from(addr: Addr) -> Self {
-        addr.0
+        addr.as_str().to_owned()


Can we also call this to_string? Sometimes it is nice to have explicit names (as_str, to_string) and not just from and into

to_string is already implemented via the Display trait implementation. This implies a ToString implementation.

ethanfrey · 2021-04-09T07:24:04Z

packages/std/src/addresses.rs

    }
 }

 impl From<&Addr> for String {
    fn from(addr: &Addr) -> Self {
-        addr.0.clone()
+        addr.as_str().to_owned()
    }
 }

 impl From<Addr> for HumanAddr {


Maybe we remove these - let's not support HumanAddr with the new code (and if someone wants that can always do addr.as_str().into().

ethanfrey · 2021-04-09T07:29:47Z

packages/std/src/addresses.rs

+/// The maximum allowed size for bech32 (https://github.com/bitcoin/bips/blob/master/bip-0173.mediawiki#bech32)
+const ADDR_MAX_LENGTH: usize = 90;
+
+#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)]


I want to reflect on the performance changes of this. We should add some guidance on usage.

Passing Addr as an argument copies 94 bytes, rather than a 4 byte pointer (like .clone() of the past version, but no heap allocations).

Passing &Addr is cheap and can be restored to Addr via copy() or clone().

Addr/&Addr.as_str() is cheap

Addr.to_string() requires a heap allocation (not just consuming the original Addr).

With this in mind, I think we can recommend returning Addr and passing in &Addr to functions.

Yes, if we pass Addr references to functions (which is the recommended approach in general), and only return Addr, I think this would be OK, performance/memory-wise.

I would like to play around with this a bit next week. The size of Addr is also my major (and only) concern here. But since we do not have huge amounts of addresses in memory at the same time and we have a well known size limit, I think it is fair to store the addresses on the stack.

We can always push them to heap by using Box<Addr>. Then we get what we had before, just without the mutability of a String implementation.

What is the problem you are trying to solve?
I feel this will just make my life harder and not solve the problem that motivated AddrRef

What is the problem you are trying to solve?

Make using owned Addr easier, especially literals. Then in many places you suddenly get an Addr->&str conversion instead of the opposite direction.

I'm not claiming this solves all issues and it is well possible that it does not replace what AddrRef does.

Make using owned Addr easier, especially literals.

I didn't have any issues there. And am afraid I will have to think way too much about unneeded copies of this struct if it happens. See #872 for another solution that provide simple literals for tests

ethanfrey · 2021-04-09T07:33:11Z

packages/std/src/addresses.rs

+        while i < input_bytes.len() {
+            data[i] = input_bytes[i];
+            i += 1;
+        }


Thinking about optimizing it, I looked at the implementation of eg. https://doc.rust-lang.org/core/ptr/fn.copy_nonoverlapping.html but that just point to rust intrinsics, so I guess this is the best we can do

maurolacy

Nice work.

packages/std/src/addresses.rs

webmaster128 · 2021-04-12T12:26:54Z

We don't do that

Make Addr::unchecked a const fn

7840fa0

webmaster128 requested review from ethanfrey and maurolacy April 8, 2021 23:22

webmaster128 mentioned this pull request Apr 8, 2021

Update cosmwasm-std CosmWasm/cw-plus#260

Merged

yihuang reviewed Apr 9, 2021

View reviewed changes

ethanfrey reviewed Apr 9, 2021

View reviewed changes

maurolacy suggested changes Apr 9, 2021

View reviewed changes

packages/std/src/addresses.rs Show resolved Hide resolved

packages/std/src/addresses.rs Show resolved Hide resolved

ethanfrey mentioned this pull request Apr 9, 2021

Addr ref helper #872

Closed

webmaster128 mentioned this pull request Apr 12, 2021

Extend Addr implementation #868

Closed

webmaster128 closed this Apr 12, 2021

webmaster128 deleted the stack-addr branch April 12, 2021 12:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Addr::unchecked a const fn #869

Make Addr::unchecked a const fn #869

webmaster128 commented Apr 8, 2021

yihuang Apr 9, 2021 •

edited

Loading

webmaster128 Apr 9, 2021

ethanfrey Apr 9, 2021

webmaster128 Apr 9, 2021

ethanfrey left a comment

ethanfrey Apr 9, 2021

webmaster128 Apr 9, 2021

ethanfrey Apr 9, 2021

ethanfrey Apr 9, 2021

ethanfrey Apr 9, 2021

webmaster128 Apr 9, 2021

ethanfrey Apr 9, 2021

ethanfrey Apr 9, 2021

maurolacy Apr 9, 2021

webmaster128 Apr 9, 2021

ethanfrey Apr 9, 2021

webmaster128 Apr 9, 2021

ethanfrey Apr 9, 2021

ethanfrey Apr 9, 2021

maurolacy left a comment

webmaster128 commented Apr 12, 2021

Make Addr::unchecked a const fn #869

Make Addr::unchecked a const fn #869

Conversation

webmaster128 commented Apr 8, 2021

yihuang Apr 9, 2021 • edited Loading

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

ethanfrey 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

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

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

maurolacy left a comment

Choose a reason for hiding this comment

webmaster128 commented Apr 12, 2021

yihuang Apr 9, 2021 •

edited

Loading