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

Avoid allocating strings when formatting allocation byte-sizes #244

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nical
Copy link
Contributor

@nical nical commented Jul 18, 2024

It's not particularly important but while I was adding integration in wgpu for the allocator reports and duplicating a bit of the formatting code in the process, it was rubbing me the wrong way to allocate strings in fmt_bytes when a Display impl could do the same without allocating, so I made the change there. Now I wrote the slightly better version, might as well contribute it back to its original spot!

Copy link
Member

@MarijnS95 MarijnS95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, that's how it's supposed to be implemented

src/allocator/mod.rs Outdated Show resolved Hide resolved
src/allocator/mod.rs Outdated Show resolved Hide resolved
@nical nical force-pushed the fmtbytes branch 2 times, most recently from 3d6b3a2 to d68764b Compare July 18, 2024 20:22
src/allocator/mod.rs Outdated Show resolved Hide resolved
src/allocator/mod.rs Outdated Show resolved Hide resolved
@MarijnS95
Copy link
Member

Seems quite coincidental that that Rust release notes suggest a nice way of writing this loop too:

https://blog.rust-lang.org/2024/07/25/Rust-1.80.0.html#exclusive-ranges-in-patterns

@nical
Copy link
Contributor Author

nical commented Jul 29, 2024

Seems quite coincidental that that Rust release notes suggest a nice way of writing this loop too:

Nice. Is it worth the msrv bump, though? wgpu has to lag a few rustc versions behind, so I'd appreciate if we shelf the nicer version for later.

@MarijnS95
Copy link
Member

Nice. Is it worth the msrv bump, though? wgpu has to lag a few rustc versions behind, so I'd appreciate if we shelf the nicer version for later.

Wasn't intending to bump just to use .. (as the article suggests, ..= is already possible with a separate constant), just that it was so well timed :)

@MarijnS95 MarijnS95 enabled auto-merge (squash) October 15, 2024 09:53
@MarijnS95 MarijnS95 changed the title Avoid allocating strings when formatting bytes Avoid allocating strings when formatting allocation byte-sizes Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants