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

to_string causes crazy memory usage problems #16415

Closed
thestinger opened this issue Aug 11, 2014 · 13 comments
Closed

to_string causes crazy memory usage problems #16415

thestinger opened this issue Aug 11, 2014 · 13 comments
Labels
I-compiletime Issue: Problems and improvements with respect to compile times. I-slow Issue: Problems and improvements with respect to performance of generated code.

Comments

@thestinger
Copy link
Contributor

5u32.to_string().byte_capacity() -> 128
"foo".to_string().byte_capacity() -> 128

I think the right solution to this is adding size hints to the underlying formatting system.

@liigo
Copy link
Contributor

liigo commented Aug 11, 2014

String is a buffer, later appends will benefits from it. If don't need a
buffer, why not use &str?

@huonw
Copy link
Member

huonw commented Aug 11, 2014

@liigo it needs to be dynamic constructed (i.e. &str is completely inappropriate for this), and 128 bytes is way way way too much when there is only 1 or 3 bytes actually used (like in the two examples).

@pnkfelix
Copy link
Member

Just so it does not get lost in the shuffle, here is my current half-baked idea to address this:

  • a separate trait ToStringSupport : fmt::Show { fn size_hint(&self) -> Option<uint> { None } }, and then,
  • change the generic impl to impl<T: ToStringSupport> ToString for T (and then use write! into a buffer of appropriate capacity instead of format!)

see related bit of discussion here: https://botbot.me/mozilla/rust-internals/2014-08-11/?msg=19511520&page=2

It would put a minor road-block before you could call to_string() on an arbitrary type implementing Show, but it seems like a pretty simple way to get most of what is desired here.

@eddyb
Copy link
Member

eddyb commented Aug 12, 2014

@pnkfelix adding the hint method to Show as a default method would break no existing code, I would prefer that.

@pczarn
Copy link
Contributor

pczarn commented Aug 15, 2014

Good ideas. Should we use the upper bound for all integers? What about different bases? I think that integers shouldn't implement size_hint, because leading_zeros (Int trait) and size_of are sufficient for determining the number of digits in all radixes.

@huonw
Copy link
Member

huonw commented Aug 15, 2014

The size_hint could take a &fmt::Formatter as well as self to be able to answer the question more precisely.

I think that integers shouldn't implement size_hint, because leading_zeros (Int trait) and size_of are sufficient for determining the number of digits in all radixes.

How will they convey the computed number of digits if they don't implement that trait and method?

@pczarn
Copy link
Contributor

pczarn commented Aug 15, 2014

I've just found out that Show is implemented for RadixFmt<T, Radix>, not directly on integers. This resolves my doubts.

How will they convey the computed number of digits if they don't implement that trait and method?

By using either size_hint or leading_zeros. Looks like it was a bad idea. Or by implementing size_hint for T: Show + Int.

@huonw
Copy link
Member

huonw commented Aug 15, 2014

not directly on integers

It is.

@pczarn
Copy link
Contributor

pczarn commented Aug 15, 2014

Then it's as simple as extending these macros. I can do that.

@barosl
Copy link
Contributor

barosl commented Dec 2, 2014

It seems that this issue can be closed now.

5u32.to_string().capacity() -> 1
"foo".to_string().capacity() -> 4
"hello".to_string().capacity() -> 8

Now the buffer size is incremented to fit the minimum 2^n.

@thestinger
Copy link
Contributor Author

That makes it seem like it's just pushing them without using with_capacity at all...

@barosl
Copy link
Contributor

barosl commented Dec 2, 2014

@thestinger Uhm, yeah, you're right. Now I'm worried that not using with_capacity() could impact the performance...

@thestinger
Copy link
Contributor Author

Rust was just picking 128 as an arbitrary amount to reserve up front before. Not using a size hint will have a severe impact on performance, but clamping to 128 was worse.

lnicola pushed a commit to lnicola/rust that referenced this issue Jan 28, 2024
…Veykril

internal: Make TryToNav trait public

Currently there is no proper way to get a target FileRange for a given Definition.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-compiletime Issue: Problems and improvements with respect to compile times. I-slow Issue: Problems and improvements with respect to performance of generated code.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants