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

Reduce size of Name struct #1985

Closed
appetrosyan opened this issue Mar 21, 2022 · 4 comments
Closed

Reduce size of Name struct #1985

appetrosyan opened this issue Mar 21, 2022 · 4 comments
Assignees
Labels
good first issue Good for newcomers iroha2-dev The re-implementation of a BFT hyperledger in RUST Optimization Something isn't working as well as it should

Comments

@appetrosyan
Copy link
Contributor

Name is a repr(transparent) struct wrapping std::string::String, which has a pointer, a length and a capacity.

However, in most cases a name isn't modified, but instead, a new name is created. So in theory we could get rid of capacity, as we don't intend for the Name struct to be mutable (there isn't a single mutable borrow or mutable Name in the code-base).

So we should investigate either using std::str::str or creating a custom structure for storing Names

@Arjentix
Copy link
Contributor

I think we can do something like ConstantString and implement impl From<ConstantString> for String to give possibility to act with it as basic a String.

Also right now Name::new() accepts &str but allocates memory for String inside it. AFAIK Rust convention is to let user decide when to allocate memory. So I think a new constructor should be: pub fn new(name: ConstantString).
If we really want to create Name from &str, then we can use FromStr::from_str() or create Name::from() as it is done for std::str::String

@appetrosyan
Copy link
Contributor Author

I largely agree, but I have a few comments:

I think we can do something like ConstantString and implement impl From<ConstantString> for String to give possibility to act with it as basic a String.

Only if necessary. The way I see it, a ConstantString should only implement Deref to a string slice. Most functions we're interested in take a slice anyway.

Also right now Name::new() accepts &str but allocates memory for String inside it…

We could try that. IMO it should really be FromStr::parse, because we raise a ParseError.

@mversic
Copy link
Contributor

mversic commented Mar 31, 2022

We could try that. IMO it should really be FromStr::parse, because we raise a ParseError.

Name::new is out the window with #1984

If we'll consider interning of Names as in #2008, which would save us a lot more memory space compared to removing capacity usize, this should become a much more low priority issue

@mversic
Copy link
Contributor

mversic commented Mar 31, 2022

This seems like an interesting and highly maintained crate

@appetrosyan appetrosyan added the iroha2-dev The re-implementation of a BFT hyperledger in RUST label May 16, 2022
@appetrosyan appetrosyan self-assigned this May 19, 2022
@appetrosyan appetrosyan added good first issue Good for newcomers Optimization Something isn't working as well as it should labels May 19, 2022
@appetrosyan appetrosyan removed their assignment Jun 1, 2022
@Erigara Erigara self-assigned this Jun 9, 2022
Erigara added a commit to Erigara/iroha that referenced this issue Jun 16, 2022
Signed-off-by: Shanin Roman <shanin1000@yandex.ru>
Erigara added a commit to Erigara/iroha that referenced this issue Jun 16, 2022
…Name`

Signed-off-by: Shanin Roman <shanin1000@yandex.ru>
Erigara added a commit to Erigara/iroha that referenced this issue Jun 16, 2022
…Name`

Signed-off-by: Shanin Roman <shanin1000@yandex.ru>
Erigara added a commit to Erigara/iroha that referenced this issue Jun 20, 2022
…Name`

Signed-off-by: Shanin Roman <shanin1000@yandex.ru>
Erigara added a commit to Erigara/iroha that referenced this issue Jun 21, 2022
Signed-off-by: Shanin Roman <shanin1000@yandex.ru>
Erigara added a commit to Erigara/iroha that referenced this issue Jun 21, 2022
…Name`

Signed-off-by: Shanin Roman <shanin1000@yandex.ru>
Erigara added a commit to Erigara/iroha that referenced this issue Jun 21, 2022
Signed-off-by: Shanin Roman <shanin1000@yandex.ru>
Erigara added a commit to Erigara/iroha that referenced this issue Jun 21, 2022
…Name`

Signed-off-by: Shanin Roman <shanin1000@yandex.ru>
Erigara added a commit to Erigara/iroha that referenced this issue Jun 22, 2022
Signed-off-by: Shanin Roman <shanin1000@yandex.ru>
Erigara added a commit to Erigara/iroha that referenced this issue Jun 22, 2022
…Name`

Signed-off-by: Shanin Roman <shanin1000@yandex.ru>
Erigara added a commit to Erigara/iroha that referenced this issue Jun 22, 2022
…Name`

Signed-off-by: Shanin Roman <shanin1000@yandex.ru>
appetrosyan pushed a commit to Erigara/iroha that referenced this issue Jun 22, 2022
Signed-off-by: Shanin Roman <shanin1000@yandex.ru>
appetrosyan pushed a commit to Erigara/iroha that referenced this issue Jun 22, 2022
…Name`

Signed-off-by: Shanin Roman <shanin1000@yandex.ru>
appetrosyan pushed a commit that referenced this issue Jun 22, 2022
Signed-off-by: Shanin Roman <shanin1000@yandex.ru>
BAStos525 pushed a commit to BAStos525/soramitsu-iroha that referenced this issue Jul 8, 2022
…r#2365)

Signed-off-by: Shanin Roman <shanin1000@yandex.ru>
Signed-off-by: BAStos525 <jungle.vas@yandex.ru>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers iroha2-dev The re-implementation of a BFT hyperledger in RUST Optimization Something isn't working as well as it should
Projects
None yet
Development

No branches or pull requests

4 participants