Skip to content
This repository has been archived by the owner on Aug 11, 2024. It is now read-only.

Rendering performance improvements #1124

Merged
merged 19 commits into from
Mar 18, 2024
Merged

Conversation

bfollington
Copy link
Collaborator

@bfollington bfollington commented Feb 22, 2024

  • Fix obvious bottlenecks
  • Prevent thrashing / loading multiple times in rapid succession
  • Composite background separately to content in all contexts

Before

image

After (No Hangs!)

image

// We have already parsed the whole document as Subtext but
// as part of rendering we will re-parse each block again.

// This has a significant cost in list views.
Self.renderer.render(renderable.block.description)
Copy link
Collaborator Author

@bfollington bfollington Feb 22, 2024

Choose a reason for hiding this comment

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

Need to rethink this rendering approach at some point.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@bfollington bfollington marked this pull request as ready for review February 27, 2024 06:05
colorScheme == .dark ? DeckTheme.darkBg : DeckTheme.lightBg
)
var body: some View {
(colorScheme == .dark ? DeckTheme.darkBg : DeckTheme.lightBg)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It doesn't seem like much, but directly rendering a LinearGradient as a view vs. using it as a background on a frame has a massively reduced rendering cost.

@bfollington bfollington force-pushed the 2024-02-22-rendering-performance branch from 9359c71 to ab96c0d Compare March 15, 2024 00:31
Copy link
Collaborator

@gordonbrander gordonbrander left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -87,7 +87,6 @@ struct AppView: View {
.onChange(of: self.scenePhase) { phase in
store.send(.scenePhaseChange(phase))
}
.sentryTrace("AppView")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Too expensive?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Too expensive and not actually useful without forking out more money to sentry.

.onAppear {
selectedTab = store.state.selectedAppTab
}
.onChange(of: selectedTab) { v in
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note: on iOS 17 we'll need to update this to

.onChange(of: selectedTab) { _, v in
  // ...
}

// We have already parsed the whole document as Subtext but
// as part of rendering we will re-parse each block again.

// This has a significant cost in list views.
Self.renderer.render(renderable.block.description)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@bfollington bfollington force-pushed the 2024-02-22-rendering-performance branch from ee0a142 to 6b72afe Compare March 18, 2024 06:01
@bfollington bfollington merged commit 40e8e39 into main Mar 18, 2024
2 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants