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(ShadowContainer): prevent unnecessary repaint #830

Merged
merged 4 commits into from
Oct 6, 2023
Merged

fix(ShadowContainer): prevent unnecessary repaint #830

merged 4 commits into from
Oct 6, 2023

Conversation

roubachof
Copy link
Contributor

@roubachof roubachof commented Sep 7, 2023 •
edited
Loading

Multiple shadows repaint were made because of comparison of records through an interface.
The base interface was replaced by an abstract record.
Another repaint was also sometimes made due to the fact that we didn't take into account the uninitialized pixel ratio.

@roubachof roubachof requested review from kazo0 and Xiaoy312 and removed request for kazo0 September 7, 2023 17:32
@github-actions
Copy link

github-actions bot commented Sep 7, 2023 •
edited by unfurl-links bot
Loading

Azure Static Web Apps: Your stage site is ready! Visit it here: https://polite-field-01aa64f0f-830.eastus2.azurestaticapps.net

@jeromelaban
Copy link
Member

On the wasm sample, the shadow does not appear anymore (it flickers and goes away):

image

@github-actions
Copy link

github-actions bot commented Sep 7, 2023 •
edited by unfurl-links bot
Loading

Azure Static Web Apps: Your stage site is ready! Visit it here: https://polite-field-01aa64f0f-830.eastus2.azurestaticapps.net

@roubachof roubachof mentioned this pull request Sep 13, 2023
Open
2 tasks
@roubachof
Copy link
Contributor Author

last commit should fix the uncovered wasm bug

@github-actions
Copy link

github-actions bot commented Sep 13, 2023 •
edited by unfurl-links bot
Loading

Azure Static Web Apps: Your stage site is ready! Visit it here: https://polite-field-01aa64f0f-830.eastus2.azurestaticapps.net

@rafael-rosa-knowcode
Copy link
Contributor

When tested in wasm we still have delays and changes seem slower.

shadow-repaint-fix.mp4

@roubachof
Copy link
Contributor Author

roubachof commented Sep 13, 2023 •
edited
Loading

Well I guess because when resizing the shadows the canvas must be invalidated at each size...
Without this PR be aware that the rendering was working because of the combination of 2 bugs.

@roubachof
Copy link
Contributor Author

I will try to squash some invalidation and do some perf logs to improve shadow rendering when elements/canvas are resizing.

@roubachof roubachof mentioned this pull request Sep 14, 2023
Closed
18 tasks
@roubachof roubachof self-assigned this Sep 14, 2023
@github-actions
Copy link

github-actions bot commented Sep 14, 2023 •
edited by unfurl-links bot
Loading

Azure Static Web Apps: Your stage site is ready! Visit it here: https://polite-field-01aa64f0f-830.eastus2.azurestaticapps.net

@roubachof
Copy link
Contributor Author

roubachof commented Sep 19, 2023 •
edited
Loading

Beware of impressions, when tested in wasm with profiling:

	  <WasmShellMonoRuntimeExecutionMode>InterpreterAndAOT</WasmShellMonoRuntimeExecutionMode>
	  <WasmShellEnableEmccProfiling>true</WasmShellEnableEmccProfiling>

We got the following results:

Without the PR:

profile_without_PR

WITH the PR:

profile_WITH_PR

Roughly 40% increased performance.
json trace files:

json_traces.zip

This is confirmed by logs made in Debug:

Without PR:

WINDOWS

[ShadowContainer] 00:00:03.3284090 InvalidationCount: 104, PaintAvoidedCount: 0, PaintCacheCount: 51, PaintShadowsCount: 52
With only one log: [ShadowContainer] 00:00:00.1427170 InvalidationCount: 104, PaintAvoidedCount: 0, PaintCacheCount: 51, PaintShadowsCount: 52

ANDROID

[ShadowContainer] 00:00:01.1261408 InvalidationCount: 94, PaintAvoidedCount: 0, PaintCacheCount: 46, PaintShadowsCount: 47
With only one log: [ShadowContainer] 00:00:00.8916601 InvalidationCount: 94, PaintAvoidedCount: 0, PaintCacheCount: 46, PaintShadowsCount: 47

WASM

[ShadowContainer] 00:00:02.0430000 InvalidationCount: 235, PaintAvoidedCount: 0, PaintCacheCount: 187, PaintShadowsCount: 47
With only one log: [ShadowContainer] 00:00:02.2280000 InvalidationCount: 235, PaintAvoidedCount: 0, PaintCacheCount: 187, PaintShadowsCount:

With PR:

WINDOWS

[ShadowContainer] 00:00:03.3811963 InvalidationCount: 104, PaintAvoidedCount: 48, PaintCacheCount: 19, PaintShadowsCount: 36
With only one log: [ShadowContainer] 00:00:00.1131885 InvalidationCount: 104, PaintAvoidedCount: 48, PaintCacheCount: 19, PaintShadowsCount: 36

ANDROID

[ShadowContainer] 00:00:01.1177631 InvalidationCount: 94, PaintAvoidedCount: 46, PaintCacheCount: 17, PaintShadowsCount: 30
With only one log: [ShadowContainer] 00:00:00.9411894 InvalidationCount: 94, PaintAvoidedCount: 46, PaintCacheCount: 17, PaintShadowsCount: 30

WASM

[ShadowContainer] 00:00:00.8540000 InvalidationCount: 235, PaintAvoidedCount: 187, PaintCacheCount: 16, PaintShadowsCount: 31
With only one log: [ShadowContainer] 00:00:00.8320000 InvalidationCount: 235, PaintAvoidedCount: 187, PaintCacheCount: 16, PaintShadowsCount: 31

@github-actions
Copy link

github-actions bot commented Sep 25, 2023 •
edited by unfurl-links bot
Loading

Azure Static Web Apps: Your stage site is ready! Visit it here: https://polite-field-01aa64f0f-830.eastus2.azurestaticapps.net

carldebilly
carldebilly approved these changes Sep 26, 2023
View reviewed changes
Copy link
Member

@carldebilly carldebilly left a comment

Choose a reason for hiding this comment

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

LGTM

@agneszitte
Copy link
Contributor

@Mergifyio backport legacy/3x release/stable/4.2

@mergify
Copy link
Contributor

mergify bot commented Oct 6, 2023 •
edited
Loading

backport legacy/3x release/stable/4.2

✅ Backports have been created

@agneszitte agneszitte merged commit fcf4422 into main Oct 6, 2023
23 checks passed
@agneszitte agneszitte deleted the Dev/jmaf/shadow-repaint-fix branch October 6, 2023 15:42
@unoplatform unoplatform deleted a comment from welcome bot Oct 6, 2023
mergify bot pushed a commit that referenced this pull request Oct 6, 2023
@roubachof @mergify
fix(ShadowContainer): prevent unnecessary repaint (#830)
69cd99a 
(cherry picked from commit fcf4422)
mergify bot pushed a commit that referenced this pull request Oct 6, 2023
@roubachof @mergify
fix(ShadowContainer): prevent unnecessary repaint (#830)
9051f23 
(cherry picked from commit fcf4422)
This was referenced Oct 6, 2023
Merged
Merged
agneszitte pushed a commit that referenced this pull request Oct 6, 2023
@mergify @roubachof
fix(ShadowContainer): prevent unnecessary repaint (backport #830) (#863)
0f2413f 
Co-authored-by: Jean-Marie Alfonsi <jm.alfonsi@gmail.com>
agneszitte pushed a commit that referenced this pull request Oct 6, 2023
@mergify @roubachof
fix(ShadowContainer): prevent unnecessary repaint (backport #830) (#862)
0b4ccac 
Co-authored-by: Jean-Marie Alfonsi <jm.alfonsi@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carldebilly carldebilly carldebilly approved these changes

@Xiaoy312 Xiaoy312 Xiaoy312 approved these changes

@kazo0 kazo0 Awaiting requested review from kazo0

Assignees

@roubachof roubachof

Labels
control/shadows
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@roubachof @jeromelaban @rafael-rosa-knowcode @agneszitte @Xiaoy312 @carldebilly @kazo0