[refactor] #1985: Reduce size of `Name` struct #2365

Erigara · 2022-06-16T15:36:20Z

Description of the Change

Add ConstString: immutable, inlinable string, which uses union internally to store boxed and inlined variants in the same space;
Replace String with ConstString in Name struct.

Issue

Closes #1985.

Benefits

All structs that uses Name take up less space.
We win at least one machine word for every instance of Name and up to 23 byte for strings 15 bytes long on 64-bit architecture, also strings shorter than 15 bytes are stored on the stack, which avoids allocation.

Possible Drawbacks

ConstString uses unsafe code, which increases the risk of bugs and vulnerabilities.

Usage Examples or Tests

Run unit tests:

cargo test --package iroha_data_primitives --lib -- conststr::tests --nocapture

Run unit tests with miri to find leaks and UB:

cargo +nightly-2022-04-20 miri test --package iroha_data_primitives --lib -- conststr::tests --nocapture

Alternate Designs

It possible to use enum instead of union to avoid unsafe, but it take 24 bytes instead of 16, see.

codecov · 2022-06-16T16:03:42Z

Codecov Report

Merging #2365 (a38603b) into iroha2-dev (6973f71) will increase coverage by 4.55%.
The diff coverage is 82.98%.

❗ Current head a38603b differs from pull request most recent head 4c298f3. Consider uploading reports for the commit 4c298f3 to get more accurate results

@@              Coverage Diff               @@
##           iroha2-dev    #2365      +/-   ##
==============================================
+ Coverage       65.50%   70.05%   +4.55%     
==============================================
  Files             133      142       +9     
  Lines           24697    27874    +3177     
==============================================
+ Hits            16177    19528    +3351     
+ Misses           8520     8346     -174

Impacted Files	Coverage Δ
cli/derive/src/lib.rs	`74.72% <ø> (ø)`
cli/src/torii/mod.rs	`28.88% <ø> (ø)`
client/src/http.rs	`47.82% <ø> (ø)`
config/src/lib.rs	`15.38% <0.00%> (+1.09%)`	⬆️
core/src/lib.rs	`100.00% <ø> (ø)`
core/src/smartcontracts/isi/block.rs	`100.00% <ø> (ø)`
core/src/smartcontracts/isi/domain.rs	`39.82% <ø> (ø)`
core/src/smartcontracts/isi/triggers.rs	`0.00% <0.00%> (ø)`
core/src/smartcontracts/isi/tx.rs	`25.00% <ø> (ø)`
core/src/smartcontracts/isi/world.rs	`9.52% <0.00%> (+0.43%)`	⬆️
... and 115 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 77e6429...4c298f3. Read the comment docs.

appetrosyan · 2022-06-16T17:27:20Z

data_model/primitives/src/conststr.rs

+}
+
+/// Union representing const-string variants: inlined or boxed.
+/// Distinction between variants are achieved by setting least significant bit for inlined variant.


I'm afraid that this might be fragile

Have you checked that non-ASCII characters also work?

Yes, i checked it, i will add test case for that.
In general, something like proptest would be helpful for testing.

Re: proptest, It would be good for unit tests, if a bit expensive. We plan on using hypothesis from Python downstream.

Still, there's this is an easy unit test to write and comment.

Another quick and dirty way to ensure that UTF-8 is fully supported, is to construct a boxed string when you see non-Ascii characters.

mversic · 2022-06-16T17:01:53Z

data_model/src/lib.rs


 impl Name {
    pub(crate) const fn empty() -> Self {
-        Self(String::new())
+        Self(ConstString::new())


if Name is immutable, and I think it is?, this can be a shared const value

I don't think about this! Thank you for suggestion.
Do we need empty Id in the first place?

Yes, we have to support empty Name/Ids at at least for now. There is an issue tracking the discussion on whether we'll allow or disallow empty identifiers

So the approach has to be one of two. Either we publish this as a separate crate that could be shared with other projects, in which case we need to allow empty values, or we tailor it directly to what our Name struct needs (in which case we could equally just merge the two and get rid of another layer of indirection.

I don't mind either way, but I think that a ConstString could be useful in Sora.

data_model/primitives/src/conststr.rs

appetrosyan · 2022-06-17T07:49:27Z

data_model/primitives/src/conststr.rs

+impl PartialOrd for ConstString {
+    #[inline]
+    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+        PartialOrd::partial_cmp(&**self, &**other)


Check if you need to impl or if you can derive, @Erigara

data_model/primitives/src/conststr.rs

appetrosyan · 2022-06-17T07:51:24Z

data_model/primitives/src/conststr.rs

+}
+
+/// Union representing const-string variants: inlined or boxed.
+/// Distinction between variants are achieved by setting least significant bit for inlined variant.


Another quick and dirty way to ensure that UTF-8 is fully supported, is to construct a boxed string when you see non-Ascii characters.

data_model/primitives/src/conststr.rs

appetrosyan · 2022-06-17T07:56:10Z

data_model/primitives/src/conststr.rs

+    }
+
+    #[test]
+    fn const_string_display() {


This is a wholly unnecessary test. All you need to do for coverage is to println! inside of other tests, which IMO would also help if someone is modifying the code.

appetrosyan · 2022-06-17T07:57:55Z

data_model/primitives/src/conststr.rs

+    }
+
+    #[test]
+    fn const_string_layout() {


This test is important. I'd put it at the top.

appetrosyan · 2022-06-17T07:58:44Z

data_model/primitives/src/conststr.rs

+    }
+
+    #[test]
+    fn const_string_new() {


This test needs refactoring. You don't test a constructor, you need to test a ConstString's lifecycle.

appetrosyan · 2022-06-17T08:00:31Z

data_model/primitives/src/conststr.rs

+
+    use super::*;
+
+    fn run_with_strings(f: impl Fn(String)) {


Like this function, since you can now add more strings to the iterator. I would suggest adding strings that contain UTF-8 characters here and seeing if the tests keep working. Consider the cases of 4-byte characters. many 2-bytes, ASCII all the way except last, odd number of UTF-8, wonky alignment…

data_model/primitives/src/conststr.rs

appetrosyan

Good job!

data_model/primitives/src/conststr.rs

mversic · 2022-06-21T10:53:58Z

data_model/primitives/src/conststr.rs

+/// Can't be derived.
+impl Eq for ConstString {}
+
+impl Serialize for ConstString {


how come there is no deserialze implementation?

Name struct doesn't have Deserialize and Decode, so i decided not to implement them for now.
I can implement then if needed.

It has them implemented manually, no?

oh, i missed them...
I will add Decode and Deserialize for ConstString!

data_model/primitives/src/conststr.rs

data_model/src/lib.rs

data_model/primitives/src/conststr.rs

data_model/src/lib.rs

data_model/primitives/src/conststr.rs

Signed-off-by: Shanin Roman <shanin1000@yandex.ru>

…Name` Signed-off-by: Shanin Roman <shanin1000@yandex.ru>

Arjentix · 2022-06-22T11:13:30Z

data_model/primitives/src/conststr.rs

+/// Distinction between variants are achieved by tagging most significant bit of field `len`:
+/// - for inlined variant MSB of `len` is always equal to 1, it's enforced by `InlinedString` constructor;
+/// - for boxed variant MSB of `len` is always equal to 0, it's enforced by the fact
+/// that `Box` and `Vec` never allocate more than`isize::MAX bytes`.


Suggested change

/// that `Box` and `Vec` never allocate more than`isize::MAX bytes`.

/// that `Box` and `Vec` never allocate more than `isize::MAX bytes`.

Arjentix · 2022-06-22T11:16:53Z

data_model/primitives/src/conststr.rs

+/// - for inlined variant MSB of `len` is always equal to 1, it's enforced by `InlinedString` constructor;
+/// - for boxed variant MSB of `len` is always equal to 0, it's enforced by the fact
+/// that `Box` and `Vec` never allocate more than`isize::MAX bytes`.
+/// For little-endian 64bit architecture memory layout of `ConstStringData` is following:


I can't see any other mentions about ConstStringData. Looks like it was deprecated

data_model/primitives/src/conststr.rs

Arjentix · 2022-06-22T11:27:24Z

data_model/primitives/src/conststr.rs

+impl Ord for ConstString {
+    #[inline]
+    fn cmp(&self, other: &Self) -> Ordering {
+        Ord::cmp(&**self, &**other)


What's the reason of dereferencing and taking reference after that? Why not just *self?

Arjentix · 2022-06-22T11:30:00Z

data_model/primitives/src/conststr.rs

+    ($($ty:ty,)*) => {
+        impl_eq!($($ty),*);
+    };
+    ($($ty:ty),*) => {$(


Suggested change

($($ty:ty,)*) => {

impl_eq!($($ty),*);

};

($($ty:ty),*) => {$(

($($ty:ty),* $(,)?) => {$(

The result is the same but it's easier to write and read

Not really because with such approach impl_eq!(,) would be valid macro invocation.

Arjentix · 2022-06-22T11:35:13Z

data_model/primitives/src/conststr.rs

+impl<T> From<T> for ConstString
+where
+    T: TryInto<InlinedString>,
+    <T as TryInto<InlinedString>>::Error: Into<BoxedString>,


Does anybody would use this blanked implementation? For me it seems like it'se easier to implement From<MyType> for ConstString manualy than implementing TryFrom<MyType> for InlinedString where Error...

interesting to note that you can have private InlinedString as a bound on a public trait

Arjentix · 2022-06-22T12:59:06Z

data_model/primitives/src/conststr.rs

+/// `BoxedString` is `Send` because the data they
+/// reference is unaliased. Aliasing invariant is enforced by


Suggested change

/// `BoxedString` is `Send` because the data they

/// reference is unaliased. Aliasing invariant is enforced by

/// [`BoxedString`] is [`Send`] because the data it

/// references is unaliased. Aliasing invariant is enforced by

Arjentix · 2022-06-22T13:00:54Z

data_model/primitives/src/conststr.rs

+/// `BoxedString` is `Sync` because the data they
+/// reference is unaliased. Aliasing invariant is enforced by


Suggested change

/// `BoxedString` is `Sync` because the data they

/// reference is unaliased. Aliasing invariant is enforced by

/// [`BoxedString`] is [`Sync`] because the data it

/// references is unaliased. Aliasing invariant is enforced by

…r#2365) Signed-off-by: Shanin Roman <shanin1000@yandex.ru> Signed-off-by: BAStos525 <jungle.vas@yandex.ru>

Erigara requested review from s8sato, appetrosyan, mversic, Arjentix, SamHSmith, QuentinI, outoftardis, ilchu and pesterev as code owners June 16, 2022 15:36

github-actions bot added the iroha2-dev The re-implementation of a BFT hyperledger in RUST label Jun 16, 2022

Erigara force-pushed the reduce_name_size branch from 837595e to 4f79413 Compare June 16, 2022 16:29

appetrosyan reviewed Jun 16, 2022

View reviewed changes

mversic requested changes Jun 16, 2022

View reviewed changes

outoftardis reviewed Jun 17, 2022

View reviewed changes

data_model/primitives/src/conststr.rs Outdated Show resolved Hide resolved

data_model/primitives/src/conststr.rs Outdated Show resolved Hide resolved

appetrosyan assigned appetrosyan and mversic Jun 17, 2022

appetrosyan reviewed Jun 17, 2022

View reviewed changes

Erigara force-pushed the reduce_name_size branch 2 times, most recently from 34dc508 to 1d237dd Compare June 21, 2022 08:31

mversic reviewed Jun 21, 2022

View reviewed changes

data_model/primitives/src/conststr.rs Outdated Show resolved Hide resolved

appetrosyan previously approved these changes Jun 21, 2022

View reviewed changes

data_model/primitives/src/conststr.rs Outdated Show resolved Hide resolved

data_model/primitives/src/conststr.rs Show resolved Hide resolved

mversic reviewed Jun 21, 2022

View reviewed changes

mversic requested changes Jun 21, 2022

View reviewed changes

data_model/primitives/src/conststr.rs Outdated Show resolved Hide resolved

data_model/primitives/src/conststr.rs Show resolved Hide resolved

data_model/primitives/src/conststr.rs Show resolved Hide resolved

mversic reviewed Jun 21, 2022

View reviewed changes

data_model/primitives/src/conststr.rs Outdated Show resolved Hide resolved

mversic reviewed Jun 21, 2022

View reviewed changes

data_model/primitives/src/conststr.rs Outdated Show resolved Hide resolved

appetrosyan requested review from appetrosyan and outoftardis June 21, 2022 11:58

mversic reviewed Jun 21, 2022

View reviewed changes

data_model/primitives/src/conststr.rs Outdated Show resolved Hide resolved

Erigara dismissed appetrosyan’s stale review via 4bd3416 June 21, 2022 19:51

Erigara force-pushed the reduce_name_size branch from 1d237dd to 4bd3416 Compare June 21, 2022 19:51

mversic reviewed Jun 22, 2022

View reviewed changes

data_model/primitives/src/conststr.rs Outdated Show resolved Hide resolved

mversic reviewed Jun 22, 2022

View reviewed changes

data_model/primitives/src/conststr.rs Outdated Show resolved Hide resolved

mversic reviewed Jun 22, 2022

View reviewed changes

data_model/src/lib.rs Outdated Show resolved Hide resolved

mversic reviewed Jun 22, 2022

View reviewed changes

data_model/primitives/src/conststr.rs Show resolved Hide resolved

mversic reviewed Jun 22, 2022

View reviewed changes

data_model/primitives/src/conststr.rs Show resolved Hide resolved

mversic reviewed Jun 22, 2022

View reviewed changes

data_model/primitives/src/conststr.rs Show resolved Hide resolved

mversic reviewed Jun 22, 2022

View reviewed changes

data_model/primitives/src/conststr.rs Outdated Show resolved Hide resolved

mversic reviewed Jun 22, 2022

View reviewed changes

data_model/primitives/src/conststr.rs Show resolved Hide resolved

mversic reviewed Jun 22, 2022

View reviewed changes

data_model/src/lib.rs Outdated Show resolved Hide resolved

appetrosyan reviewed Jun 22, 2022

View reviewed changes

data_model/src/lib.rs Outdated Show resolved Hide resolved

data_model/primitives/src/conststr.rs Outdated Show resolved Hide resolved

data_model/primitives/src/conststr.rs Outdated Show resolved Hide resolved

data_model/primitives/src/conststr.rs Outdated Show resolved Hide resolved

appetrosyan added Optimization Something isn't working as well as it should Unsafe labels Jun 22, 2022

safinsaf deleted the branch hyperledger:iroha2-dev June 22, 2022 11:56

safinsaf closed this Jun 22, 2022

safinsaf reopened this Jun 22, 2022

Erigara force-pushed the reduce_name_size branch 2 times, most recently from eea0e7c to a38603b Compare June 22, 2022 12:54

mversic approved these changes Jun 22, 2022

View reviewed changes

appetrosyan approved these changes Jun 22, 2022

View reviewed changes

Erigara added 2 commits June 22, 2022 17:08

[feature] hyperledger#1985: Implement ConstString

84bbfeb

Signed-off-by: Shanin Roman <shanin1000@yandex.ru>

[refactor] hyperledger#1985: Replace String with ConstString in `…

4c298f3

…Name` Signed-off-by: Shanin Roman <shanin1000@yandex.ru>

appetrosyan force-pushed the reduce_name_size branch from a38603b to 4c298f3 Compare June 22, 2022 13:08

appetrosyan merged commit eeb19b2 into hyperledger:iroha2-dev Jun 22, 2022

Arjentix reviewed Jun 22, 2022

View reviewed changes

Erigara deleted the reduce_name_size branch June 22, 2022 14:50

BAStos525 pushed a commit to BAStos525/soramitsu-iroha that referenced this pull request Jul 8, 2022

[refactor] hyperledger#1985: Reduce size of Name struct (hyperledge…

0f8f488

…r#2365) Signed-off-by: Shanin Roman <shanin1000@yandex.ru> Signed-off-by: BAStos525 <jungle.vas@yandex.ru>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[refactor] #1985: Reduce size of `Name` struct #2365

[refactor] #1985: Reduce size of `Name` struct #2365

Erigara commented Jun 16, 2022 •

edited

codecov bot commented Jun 16, 2022 •

edited

appetrosyan Jun 16, 2022 •

edited

Erigara Jun 17, 2022

appetrosyan Jun 17, 2022

appetrosyan Jun 17, 2022

mversic Jun 16, 2022

Erigara Jun 17, 2022 •

edited

mversic Jun 17, 2022 •

edited

appetrosyan Jun 20, 2022

appetrosyan Jun 17, 2022

appetrosyan Jun 17, 2022

appetrosyan Jun 17, 2022

appetrosyan Jun 17, 2022

appetrosyan Jun 17, 2022

appetrosyan Jun 17, 2022

appetrosyan left a comment

mversic Jun 21, 2022

Erigara Jun 21, 2022

mversic Jun 21, 2022

Erigara Jun 21, 2022

Arjentix Jun 22, 2022

Arjentix Jun 22, 2022

Arjentix Jun 22, 2022

Arjentix Jun 22, 2022

Arjentix Jun 22, 2022

Erigara Jun 22, 2022 •

edited

Arjentix Jun 22, 2022

mversic Jun 22, 2022 •

edited

Arjentix Jun 22, 2022

Arjentix Jun 22, 2022

	/// that `Box` and `Vec` never allocate more than`isize::MAX bytes`.
	/// that `Box` and `Vec` never allocate more than `isize::MAX bytes`.

		/// `BoxedString` is `Send` because the data they
		/// reference is unaliased. Aliasing invariant is enforced by

		/// `BoxedString` is `Sync` because the data they
		/// reference is unaliased. Aliasing invariant is enforced by

[refactor] #1985: Reduce size of Name struct #2365

[refactor] #1985: Reduce size of Name struct #2365

Conversation

Erigara commented Jun 16, 2022 • edited

Description of the Change

Issue

Benefits

Possible Drawbacks

Usage Examples or Tests

Alternate Designs

codecov bot commented Jun 16, 2022 • edited

Codecov Report

appetrosyan Jun 16, 2022 • 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

Choose a reason for hiding this comment

Erigara Jun 17, 2022 • edited

Choose a reason for hiding this comment

mversic Jun 17, 2022 • 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

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

appetrosyan 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

Erigara Jun 22, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mversic Jun 22, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[refactor] #1985: Reduce size of `Name` struct #2365

[refactor] #1985: Reduce size of `Name` struct #2365

Erigara commented Jun 16, 2022 •

edited

codecov bot commented Jun 16, 2022 •

edited

appetrosyan Jun 16, 2022 •

edited

Erigara Jun 17, 2022 •

edited

mversic Jun 17, 2022 •

edited

Erigara Jun 22, 2022 •

edited

mversic Jun 22, 2022 •

edited