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

Unnecessary internal use of World::read_change_ticks #6812

Closed
james7132 opened this issue Dec 1, 2022 · 4 comments
Closed

Unnecessary internal use of World::read_change_ticks #6812

james7132 opened this issue Dec 1, 2022 · 4 comments
Labels
A-ECS Entities, components, systems, and events C-Performance A change motivated by improving speed, memory usage or compile times D-Trivial Nice and easy! A great choice to get started with Bevy S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it!

Comments

@james7132
Copy link
Member

We're currently using World::read_change_ticks in a variety of locations in bevy_ecs where we have a &mut World. This adds unnecessary atomic reads in locations where we can use normal loads. These usages can be replaced with World::change_tick instead.

@james7132 james7132 added D-Trivial Nice and easy! A great choice to get started with Bevy A-ECS Entities, components, systems, and events C-Performance A change motivated by improving speed, memory usage or compile times S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! labels Dec 1, 2022
@mnmaita
Copy link
Member

mnmaita commented Dec 1, 2022

Most places where I checked that were using read_change_ticks were also borrowing World as an immutable reference, so using change_tick makes it to be also borrowed as mutable.

e.g.

    /// Returns an [`Iterator`] over the query results for the given [`World`].
    #[inline]
    pub fn iter_mut<'w, 's>(&'s mut self, world: &'w mut World) -> QueryIter<'w, 's, Q, F> {
        // SAFETY: query has unique world access
        unsafe {
            self.update_archetypes(world);
            self.iter_unchecked_manual(world, world.last_change_tick(), world.change_tick())  // world is borrowed both as immutable and mutable here
        }
    }

I did find two places where we can just replace the method but do you have any suggestions on these places where we can't?

@james7132
Copy link
Member Author

The change tick does not increment before those function calls, so it should still be correct to use World::change_ticks to read it, cache a value, and then pass that in place of the atomic read.

@mnmaita
Copy link
Member

mnmaita commented Dec 1, 2022

Oh! I see, thanks for the clarification.😅

I can take care of this task @james7132.

@mnmaita
Copy link
Member

mnmaita commented Dec 1, 2022

I'll push a PR that addresses this soon. One more question though @james7132:

    #[inline]
    pub unsafe fn get_unchecked_mut<T: Component>(&self) -> Option<Mut<'_, T>> {
        get_component_and_ticks_with_type(self.world, TypeId::of::<T>(), self.entity, self.location)
            .map(|(value, ticks)| Mut {
                value: value.assert_unique().deref_mut::<T>(),
                ticks: Ticks::from_tick_cells(
                    ticks,
                    self.world.last_change_tick(),
                    self.world.read_change_tick(),
                ),
            })
    }

In this case, world field is mut but self is not. I'm unsure if I should change this method signature to be able to access world mutably.

@bors bors bot closed this as completed in eff632d Dec 5, 2022
ickshonpe pushed a commit to ickshonpe/bevy that referenced this issue Dec 6, 2022
…`bevy_ecs` crate (bevyengine#6816)

# Objective

- Fixes bevyengine#6812.

## Solution

- Replaced `World::read_change_ticks` with `World::change_ticks` within `bevy_ecs` crate in places where `World` references were mutable.

---
alradish pushed a commit to alradish/bevy that referenced this issue Jan 22, 2023
…`bevy_ecs` crate (bevyengine#6816)

# Objective

- Fixes bevyengine#6812.

## Solution

- Replaced `World::read_change_ticks` with `World::change_ticks` within `bevy_ecs` crate in places where `World` references were mutable.

---
ItsDoot pushed a commit to ItsDoot/bevy that referenced this issue Feb 1, 2023
…`bevy_ecs` crate (bevyengine#6816)

# Objective

- Fixes bevyengine#6812.

## Solution

- Replaced `World::read_change_ticks` with `World::change_ticks` within `bevy_ecs` crate in places where `World` references were mutable.

---
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Performance A change motivated by improving speed, memory usage or compile times D-Trivial Nice and easy! A great choice to get started with Bevy S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it!
Projects
None yet
2 participants