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.

Sign up for GitHub

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

Jump to bottom

Use immutable key for HashMap and HashSet #12086

Merged
merged 21 commits into from
Feb 26, 2024
Merged

Use immutable key for HashMap and HashSet #12086

merged 21 commits into from
Feb 26, 2024

Conversation

tguichaoua
Copy link
Contributor

@tguichaoua tguichaoua commented Feb 24, 2024 •
edited
Loading

Objective

Memory usage optimisation

Solution

HashMap and HashSet's keys are immutable. So using mutable types like String, Vec<T>, or PathBuf as a key is a waste of memory: they have an extra usize for their capacity and may have spare capacity.
This PR replaces these types by their immutable equivalents Box<str>, Box<[T]>, and Box<Path>.

For more context, I recommend watching the Use Arc Instead of Vec video.

@james7132 james7132 added A-ECS Entities, components, systems, and events A-Assets Load files from disk to use for things like images, models, and sounds C-Performance A change motivated by improving speed, memory usage or compile times labels Feb 24, 2024
nicopap
nicopap reviewed Feb 24, 2024
View reviewed changes
Copy link
Contributor

@nicopap nicopap left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I'm for it. I think it's a direct improvement.

Could you replace all the Box::from(x) and .into_boxed_string() with x.into() for the Box<str> and Box<[T]>?

It should work, practically all the relevant conversions are implemented https://doc.rust-lang.org/stable/alloc/boxed/struct.Box.html#impl-From%3CCow%3C'_,+str%3E%3E-for-Box%3Cstr%3E

crates/bevy_asset/src/io/memory.rs Outdated Show resolved Hide resolved
crates/bevy_asset/src/io/memory.rs Outdated Show resolved Hide resolved
crates/bevy_asset/src/io/gated.rs Outdated Show resolved Hide resolved
crates/bevy_asset/src/io/gated.rs Outdated Show resolved Hide resolved
james7132
james7132 reviewed Feb 24, 2024
View reviewed changes
Copy link
Member

@james7132 james7132 left a comment

Choose a reason for hiding this comment

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

Note that this does require us to more proactively use the entry APIs to avoid allocating while using these maps, which I'm not opposed to, but it is a potential ergonomics hazard.

I'm similarly concerned about needing to double allocate for many of these, since the conversion from String/Vec to Box does require a shrink_to_fit reallocation.

crates/bevy_asset/src/io/memory.rs Outdated Show resolved Hide resolved
nicopap
nicopap approved these changes Feb 25, 2024
View reviewed changes
Copy link
Contributor

@nicopap nicopap left a comment

Choose a reason for hiding this comment

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

I don't see anywhere where we do allocations when we didn't before, so it looks good to me.

james7132
james7132 requested changes Feb 25, 2024
View reviewed changes
Copy link
Member

@james7132 james7132 left a comment

Choose a reason for hiding this comment

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

I don't see anywhere where we do allocations when we didn't before

We may be able to eliminate many of tthe allocations entirely by using entry_ref where possible.

I made a few suggestions, but in reality, we can use this since the parameter to get and entry_ref take any reference that hash to the same result.

crates/bevy_asset/src/io/gated.rs Outdated Show resolved Hide resolved
crates/bevy_asset/src/io/gated.rs Outdated Show resolved Hide resolved
tools/build-templated-pages/src/examples.rs Outdated Show resolved Hide resolved
crates/bevy_render/src/render_resource/pipeline_cache.rs Outdated Show resolved Hide resolved
tguichaoua and others added 5 commits February 26, 2024 10:10
@tguichaoua @james7132
use entry_ref
2b3f08b 
Co-authored-by: James Liu <contact@jamessliu.com>
@tguichaoua @james7132
use entry_ref
f965ae3 
Co-authored-by: James Liu <contact@jamessliu.com>
@tguichaoua @james7132
use entry_ref
76327b4 
Co-authored-by: James Liu <contact@jamessliu.com>
@tguichaoua @james7132
use entry_ref
456a85f 
Co-authored-by: James Liu <contact@jamessliu.com>
@tguichaoua
Merge branch 'main' into hash_map_set_immutable_key
6ffba85
@github-actions GitHub Actions
Copy link
Contributor

You added a new example but didn't add metadata for it. Please update the root Cargo.toml file.

1 similar comment
@github-actions GitHub Actions
Copy link
Contributor

You added a new example but didn't add metadata for it. Please update the root Cargo.toml file.

@github-actions GitHub Actions
Copy link
Contributor

You added a new example but didn't add metadata for it. Please update the root Cargo.toml file.

1 similar comment
@github-actions GitHub Actions
Copy link
Contributor

You added a new example but didn't add metadata for it. Please update the root Cargo.toml file.

james7132
james7132 approved these changes Feb 26, 2024
View reviewed changes
Copy link
Member

@james7132 james7132 left a comment

Choose a reason for hiding this comment

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

Thanks for addressing those changes as a part of this PR.

@james7132 james7132 added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Feb 26, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Feb 26, 2024
Merged via the queue into bevyengine:main with commit 1cded6a Feb 26, 2024
28 of 29 checks passed
@tguichaoua tguichaoua deleted the hash_map_set_immutable_key branch February 26, 2024 16:49
msvbg pushed a commit to msvbg/bevy that referenced this pull request Feb 26, 2024
@tguichaoua @james7132 @msvbg
Use immutable key for HashMap and HashSet (bevyengine#12086)
dc0e02a 
# Objective

Memory usage optimisation

## Solution

`HashMap` and `HashSet`'s keys are immutable. So using mutable types
like `String`, `Vec<T>`, or `PathBuf` as a key is a waste of memory:
they have an extra `usize` for their capacity and may have spare
capacity.
This PR replaces these types by their immutable equivalents `Box<str>`,
`Box<[T]>`, and `Box<Path>`.

For more context, I recommend watching the [Use Arc Instead of
Vec](https://www.youtube.com/watch?v=A4cKi7PTJSs) video.

---------

Co-authored-by: James Liu <contact@jamessliu.com>
msvbg pushed a commit to msvbg/bevy that referenced this pull request Feb 26, 2024
@tguichaoua @james7132 @msvbg
Use immutable key for HashMap and HashSet (bevyengine#12086)
09599f1 
# Objective

Memory usage optimisation

## Solution

`HashMap` and `HashSet`'s keys are immutable. So using mutable types
like `String`, `Vec<T>`, or `PathBuf` as a key is a waste of memory:
they have an extra `usize` for their capacity and may have spare
capacity.
This PR replaces these types by their immutable equivalents `Box<str>`,
`Box<[T]>`, and `Box<Path>`.

For more context, I recommend watching the [Use Arc Instead of
Vec](https://www.youtube.com/watch?v=A4cKi7PTJSs) video.

---------

Co-authored-by: James Liu <contact@jamessliu.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@james7132 james7132 james7132 approved these changes

@nicopap nicopap nicopap approved these changes

Assignees
No one assigned
Labels
A-Assets Load files from disk to use for things like images, models, and sounds A-ECS Entities, components, systems, and events C-Performance A change motivated by improving speed, memory usage or compile times S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tguichaoua @james7132 @nicopap @alice-i-cecile