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

iOS mobile windowing-related performance regression and log spam #16541

Open
extrawurst opened this issue Nov 28, 2024 · 9 comments
Open

iOS mobile windowing-related performance regression and log spam #16541

extrawurst opened this issue Nov 28, 2024 · 9 comments
Labels
A-Rendering Drawing game state to the screen A-Windowing Platform-agnostic interface layer to run your app in C-Bug An unexpected or incorrect behavior C-Performance A change motivated by improving speed, memory usage or compile times O-iOS Specific to the iOS mobile operating system S-Needs-Design This issue requires design work to think about how it would best be accomplished

Comments

@extrawurst
Copy link
Contributor

extrawurst commented Nov 28, 2024

Bevy version

3d72e08

[Optional] Relevant system information

  • cargo 1.82.0 (8f40fc59f 2024-08-21)
  • xcode 16.1 (16B40)

What you did

Just running the bevy_mobile_example.xcodeproj

What went wrong

Console spamming of:

Monitor removed 14v3908#16784732192782
processing non `RedrawRequested` event after the main event loop: AboutToWait
Monitor removed 4v3083#13241384173572
processing non `RedrawRequested` event after the main event loop: AboutToWait
Monitor removed 14v3909#16789027160078
processing non `RedrawRequested` event after the main event loop: AboutToWait
Monitor removed 4v3084#13245679140868
processing non `RedrawRequested` event after the main event loop: AboutToWait
Monitor removed 14v3910#16793322127374
processing non `RedrawRequested` event after the main event loop: AboutToWait
Monitor removed 4v3085#13249974108164
processing non `RedrawRequested` event after the main event loop: AboutToWait
Monitor removed 14v3911#16797617094670
processing non `RedrawRequested` event after the main event loop: AboutToWait
Monitor removed 4v3086#13254269075460
processing non `RedrawRequested` event after the main event loop: AboutToWait
Monitor removed 14v3912#16801912061966
processing non `RedrawRequested` event after the main event loop: AboutToWait
Monitor removed 4v3087#13258564042756
processing non `RedrawRequested` event after the main event loop: AboutToWait
Monitor removed 14v3913#16806207029262
processing non `RedrawRequested` event after the main event loop: AboutToWait
Monitor removed 4v3088#13262859010052

Otherwise visually the example seems to run fine in the simulator.

here is the log of the same example on bevy 14.2: https://gist.github.com/extrawurst/7cbbb5b1918b5876a93705b92d65e196

here is the entire log of the above example in 3d72e08:
https://gist.github.com/extrawurst/b91117297242d412091862db171e29ab

If you compare this to the 14.2 behaviour there are a few things very different:

  1. we don't have timestamps anymore
  2. instead of just the old friend processing non RedrawRequested event after the main event loop: AboutToWait we now seem to get this second log every frame: Monitor removed 4v3088#13262859010052
  3. the startup produces an absolutely massive amount of additional logs if you compare both full logs

I wonder if this is all intended? The repeating Monotitor removed at the very least looks very wrong.

Additional Information

This additional activity seems to mirror also the huge additional CPU load:

Image
Image

Average CPU usage in the example goes up from ~90% to >200%. This is a regression in my books.

@extrawurst extrawurst added C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled labels Nov 28, 2024
@BenjaminBrienen BenjaminBrienen added O-iOS Specific to the iOS mobile operating system S-Needs-Design This issue requires design work to think about how it would best be accomplished A-Rendering Drawing game state to the screen C-Performance A change motivated by improving speed, memory usage or compile times and removed S-Needs-Triage This issue needs to be labelled labels Nov 28, 2024
@alice-i-cecile alice-i-cecile added the A-Windowing Platform-agnostic interface layer to run your app in label Nov 28, 2024
@alice-i-cecile alice-i-cecile added this to the 0.16 milestone Nov 28, 2024
@alice-i-cecile
Copy link
Member

This is important to me, but I don't see an imminent easy fix at this stage. We'll do our best to backport anything that's done though.

@extrawurst
Copy link
Contributor Author

ok instrumentation reveals that we spent 15% in bevy_winit::system::create_monitors:

Image

The reason for this is because we detect a new unseen monitor each frame and spawn it:

let entity = commands

just to delete the old one from last frame:

info!("Monitor removed {:?}", entity);

which triggers exactly the logging line that we see spamming: Monitor removed 4v3086#13254269075460

I am not sure yet why this is. Its almost as if we can't seem to rely on the MonitorHandle::eq check. @tychedelia did you run into this when adding this via 3360b45 ?

@tychedelia
Copy link
Contributor

I am not sure yet why this is. Its almost as if we can't seem to rely on the MonitorHandle::eq check. @tychedelia did you run into this when adding this via 3360b45 ?

No, this is not the behavior that we see on desktop, and I don't have a way to test mobile. But this is a good call out, we should investigate on the winit side as the id for the monitor should be stable, it's possible this is a regression there.

@tychedelia
Copy link
Contributor

I also imagine that there's a lot of cost to the info! log while it's spamming. Not that this is a "good" solution but compiling out that log may at least help the perf if we want a stop gap solution.

@extrawurst
Copy link
Contributor Author

I also imagine that there's a lot of cost to the info! log while it's spamming. Not that this is a "good" solution but compiling out that log may at least help the perf if we want a stop gap solution.

60% of this is the span of info and 40% is the hotpath Vec::retain

@tychedelia
Copy link
Contributor

UiKit:

impl PartialEq for MonitorHandle {
    fn eq(&self, other: &Self) -> bool {
        ptr::eq(self, other)
    }
}

AppKit:

impl PartialEq for MonitorHandle {
    fn eq(&self, other: &Self) -> bool {
        unsafe {
            ffi::CGDisplayCreateUUIDFromDisplayID(self.0)
                == ffi::CGDisplayCreateUUIDFromDisplayID(other.0)
        }
    }
}

@extrawurst
Copy link
Contributor Author

@madsmtm would be great to have your opinion

@madsmtm
Copy link

madsmtm commented Nov 28, 2024

Indeed, that's wrong, Winit is comparing the pointer address of MonitorHandle itself (not the pointer address of the inner UIScreen). I've put up rust-windowing/winit#4013, though don't have the time rn to test if that's actually enough.

@alice-i-cecile alice-i-cecile modified the milestones: 0.16, 0.15 Nov 28, 2024
@alice-i-cecile alice-i-cecile changed the title iOS mobile example regressions? iOS mobile windowing-related performance regression and log spam Nov 29, 2024
@mockersf
Copy link
Member

mockersf commented Nov 29, 2024

The monitor log spam actually makes the real issue visible: we're running about_to_wait as fast as possible. create_monitors eats 15% cpu because we're running it 5000 times per second on my simulator.

Fixing it is a good thing, but we should look again at how we handle the winit loop

@mockersf mockersf removed this from the 0.15 milestone Nov 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen A-Windowing Platform-agnostic interface layer to run your app in C-Bug An unexpected or incorrect behavior C-Performance A change motivated by improving speed, memory usage or compile times O-iOS Specific to the iOS mobile operating system S-Needs-Design This issue requires design work to think about how it would best be accomplished
Projects
None yet
Development

No branches or pull requests

6 participants