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

fix: restore performance of macOS window resizing #40577

Merged
merged 1 commit into from Nov 22, 2023
Merged

fix: restore performance of macOS window resizing #40577

merged 1 commit into from Nov 22, 2023

Conversation

MarshallOfSound
Copy link
Member

@MarshallOfSound MarshallOfSound commented Nov 21, 2023 •
edited

As in title, technically an upstream of a discord downstream patch --> discord@a712b66

Fixes an ongoing perf issue with window resizing on macOS by reverting an android-only fix in Chromium

Notes: Fixed resizing performance issue on macOS

@MarshallOfSound MarshallOfSound requested a review from a team as a code owner November 21, 2023 23:08
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Nov 21, 2023
@MarshallOfSound MarshallOfSound added target/26-x-y PR should also be added to the "26-x-y" branch. target/27-x-y PR should also be added to the "27-x-y" branch. target/28-x-y PR should also be added to the "28-x-y" branch. and removed new-pr 🌱 PR opened in the last 24 hours labels Nov 21, 2023
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Nov 21, 2023
@MarshallOfSound MarshallOfSound added semver/patch backwards-compatible bug fixes and removed new-pr 🌱 PR opened in the last 24 hours labels Nov 21, 2023
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Nov 21, 2023
@nornagon
Copy link
Member

has this been fixed upstream too?

@MarshallOfSound
Copy link
Member Author

has this been fixed upstream too?

No, it doesn't impact upstream, but I'm figuring out if I can make this change behind an desktop vs mobile check or something and upstream that. For now this is a lightweight patch that fixes a rather annoying bug on our side with low risk (because it's reverting an android only fix)

@zcbenz
Copy link
Member

zcbenz commented Nov 22, 2023

Do you have a screencast that shows the difference? It should be possible to have Chromium revert the change for at least desktop if you can show them the downgraded performance.

@bpasero
Copy link
Contributor

bpasero commented Nov 22, 2023

One thing I notice with Electron apps is how slow the content responds to resize events when quickly done rapidly:

Recording 2023-11-22 at 07 19 19

Curious if this change would have an impact 🤔

@MarshallOfSound
Copy link
Member Author

Curious if this change would have an impact 🤔

This is exactly what this fixes

@MarshallOfSound
Copy link
Member Author

Do you have a screencast that shows the difference? It should be possible to have Chromium revert the change for at least desktop if you can show them the downgraded performance.

I'll include a screencast when I bug this upstream 👍

@MarshallOfSound MarshallOfSound merged commit 1574cbf into main Nov 22, 2023
25 checks passed
@MarshallOfSound MarshallOfSound deleted the fix-window-resize branch November 22, 2023 07:58
@release-clerk Release Clerk
Copy link

release-clerk bot commented Nov 22, 2023

Release Notes Persisted

Fixed resizing performance issue on macOS

This was referenced Nov 22, 2023
Merged
Merged
@trop
Copy link
Contributor

trop bot commented Nov 22, 2023

I have automatically backported this PR to "26-x-y", please check out #40584

@trop trop bot mentioned this pull request Nov 22, 2023
Merged
@trop trop bot removed the target/26-x-y PR should also be added to the "26-x-y" branch. label Nov 22, 2023
@trop
Copy link
Contributor

trop bot commented Nov 22, 2023

I have automatically backported this PR to "27-x-y", please check out #40585

@trop
Copy link
Contributor

trop bot commented Nov 22, 2023

I have automatically backported this PR to "28-x-y", please check out #40586

@trop trop bot added in-flight/28-x-y and removed target/27-x-y PR should also be added to the "27-x-y" branch. target/28-x-y PR should also be added to the "28-x-y" branch. labels Nov 22, 2023
@deepak1556
Copy link
Member

No, it doesn't impact upstream,

@MarshallOfSound curious why this issue does not impact desktop upstream ? Is there anything unique to the code path in question that has behavior differences between electron and chrome ?

@MarshallOfSound
Copy link
Member Author

curious why this issue does not impact desktop upstream ? Is there anything unique to the code path in question that has behavior differences between electron and chrome ?

I was interested in this too but couldn't figure it out. There's definitely something different here (which would be good to figure out separately). Will dig in deeper tomorrow.

@trop trop bot added merged/26-x-y PR was merged to the "26-x-y" branch. merged/28-x-y PR was merged to the "28-x-y" branch. merged/27-x-y PR was merged to the "27-x-y" branch. and removed in-flight/26-x-y in-flight/28-x-y in-flight/27-x-y labels Nov 22, 2023
nornagon
nornagon reviewed Nov 22, 2023
View reviewed changes
patches/chromium/fix_restore_original_resize_performance_on_macos.patch
Comment on lines +6 to +8
Reverts https://chromium-review.googlesource.com/c/chromium/src/+/3118293 which
fixed a android only regression but in the process regressed electron window
resize perf wildly on macOS.
Copy link
Member

Choose a reason for hiding this comment

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

Per our patch policy, all patches are required to include in their description either (a) conditions for the patch's removal, or (b) a reason why the patch can't be removed. Please update this description 🙇🏻

@bpasero
Copy link
Contributor

bpasero commented Nov 23, 2023

I just played around with the newly released Electron 27.1.2 and unfortunately do not see a difference:

Recording 2023-11-23 at 09 10 01

Filed #40603, because I cannot reproduce in web only.

@markh-discord markh-discord mentioned this pull request Nov 27, 2023
Open
@dtzxporter
Copy link

I would also like to mention that the issue we commented on in #36280 is also not resolved by this PR using 27.1.2:
image

MrHuangJser pushed a commit to MrHuangJser/electron that referenced this pull request Dec 11, 2023
@MarshallOfSound
fix: restore performance of macOS window resizing (electron#40577)
8a97cf6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nornagon nornagon nornagon left review comments

@VerteDinde VerteDinde VerteDinde approved these changes

Assignees
No one assigned
Labels
merged/26-x-y PR was merged to the "26-x-y" branch. merged/27-x-y PR was merged to the "27-x-y" branch. merged/28-x-y PR was merged to the "28-x-y" branch. new-pr 🌱 PR opened in the last 24 hours semver/patch backwards-compatible bug fixes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@MarshallOfSound @nornagon @zcbenz @bpasero @deepak1556 @dtzxporter @VerteDinde