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

fix: Don't render LoadingSpinner after it has faded out #5164

Merged
merged 1 commit into from Jul 7, 2021
Merged

fix: Don't render LoadingSpinner after it has faded out #5164

merged 1 commit into from Jul 7, 2021

Conversation

jviide
Copy link
Contributor

@jviide jviide commented Jul 6, 2021

This pull request attempts to reduce Wire's CPU usage when idle.

In the discussions related to issues #2507 and #2239 (and maybe elsewhere) @charlag et al. tracked down a culprit for some surpsingly high constant CPU load. The LoadingSpinner component is always rendered on top of the rest of the UI with 0 opacity after it has initially faded out, continuously using resources.

This pull request addresses the issue by hooking to the onTransitionEnd and onTransitionCanceled events and set a flag marking the fade out completed giving the fade out transition a fixed amount of time to finish. After this the component returns null, effectively removing the LoadingSpinner contents from the DOM tree.

Personally, I've noticed constant ~15% load when Wire was idling on my M1 MacBook Air. Launching Wire with devtools enabled and removing the spinner from the DOM drops the idle CPU usage to near zero. I couldn't get the build infra work 100% on my machine, so @ikisusi and @esatormi volunteered to test this patch on their Intel Macs, both getting similar results. @ikisusi's CPU activity monitor looked like this before applying the patch (and after the spinned had faded to 0 opacity):

before

And like this after applying the patch:

after

@CLAassistant
Copy link

CLAassistant commented Jul 6, 2021

CLA assistant check
All committers have signed the CLA.

@jviide jviide changed the title Don't render LoadingSpinner after it has faded out fix: Don't render LoadingSpinner after it has faded out Jul 6, 2021
@ffflorian
Copy link
Contributor

Hi @jviide, thanks for the PR! I'll review it tomorrow.

@jviide
Copy link
Contributor Author

jviide commented Jul 7, 2021

I modified the approach. Instead of observing onTransitionEnd and onTransitionCancel events, the transition is given a fixed amount of time to finish after the isLoading flag flips from true to false. This happens regardless of whether the visible flag is on or off.

The length of this "transition grace period" is 500 ms by default to accommodate the current 250 ms transition.

I made this change after realizing that there may be situations when the transition events never get dispatched. For example, the user might switch accounts just right, making visible a webview that has already finished loading in the background. Then the transition from opacity 1 to 0 never happens.

@ffflorian
Copy link
Contributor

Tested manually. Looks good!

@ffflorian ffflorian added this to the Version 3.27 milestone Jul 7, 2021
@ffflorian ffflorian merged commit 60f542c into wireapp:dev Jul 7, 2021
@ffflorian
Copy link
Contributor

ffflorian commented Jul 7, 2021

Again thank you very much @jviide! I'll notify you as soon as the release including this PR is published.

@jviide jviide deleted the hide-loading-spinner branch July 7, 2021 22:35
@jviide
Copy link
Contributor Author

jviide commented Aug 26, 2021

Is there anything I could do to help get this change into production? The ability to squeeze a bit more battery life out of my laptop would be lovely :)

@bennycode
Copy link
Contributor

Hi @jviide, your fix will go live with v3.27. We plan to ship this release early next week.

@marvinhagemeister
Copy link

marvinhagemeister commented Oct 13, 2021

@bennycode @ffflorian @Yserz I'm running into the same issue as @jviide and would greatly appreciate a new release. In its current state wireapp drains battery like crazy.

@bennycode
Copy link
Contributor

Hi @marvinhagemeister, unfortunately our release got pushed back so it comes later than I anticipated. I am also not working at Wire anymore so my successor will take care of it.

@fluffypony
Copy link

Still waiting for this to go in, and for M1 support (Electron has had M1 support for well over a year).

@JMoVS
Copy link

JMoVS commented Dec 13, 2021

@Yserz Is there any news when this might finally land on desktop on Macs? Desktop is still stuck at 3.26

@fluffypony
Copy link

This is crazy that it's taking months to go in - is this not something that can be part of a Wire Web update? I was under the impression those happen weekly, yet here we are nearly half a year later.

@embeddedt
Copy link

Can confirm that this is still an issue in January 2022 - after using the uBlock-based workaround suggested here, my CPU usage at idle is a lot less.

@jviide
Copy link
Contributor Author

jviide commented Apr 11, 2022

Seems that a version containing this PR has now been released to Mac App Store.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants