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

Bk/performance improvements #272

Merged
merged 3 commits into from
Sep 22, 2023
Merged

Conversation

bryankeller
Copy link
Contributor

Details

This PR makes 2 performance improvements:

  • Prevent a few DateFormatters from getting recreated on every CalendarViewContent creation
  • Prevent updateVisibleViews(...) from getting called if the calendar hasn't scrolled far enough for the visible set of views to have changed

Related Issue

N/A

Motivation and Context

Optimizing host calendar in the Airbnb app

How Has This Been Tested

Example app, profiling.

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.

return itemModel
}
private lazy var defaultMonthHeaderItemProvider: (Month) -> AnyCalendarItemModel =
{ [unowned self] month in
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be impossible to hit cc @calda

Copy link
Member

Choose a reason for hiding this comment

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

What do you mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@calda I mean the unowned should be "safe" / there's no way to crash here.

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps -- although not sure I understand the motivation to change from capturing individual properties (guaranteed to be safe) to using unowned instead (potentially unsafe).

You could feasibly construct a case that crashes by doing something like this:

let content = CalendarViewContent(...)

let monthHeaderItemProvider = content.monthHeaderItemProvider

/// ... later, in a different scope, so content itself has been deallocated

monthBackgroundItemProvider(month) // 💥 crash because content no longer exists but there was an unowned reference to it

so the safety depends on the usage of the content.monthHeaderItemProvider property

Copy link
Contributor Author

@bryankeller bryankeller Sep 22, 2023

Choose a reason for hiding this comment

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

That would only be possible internally though, since CalendarViewContent doesn't expose its item provider closures publicly.

Capturing the individual properties here causes them to be created immediately (which is expensive for date formatters).

lazy var expensiveProperty = ExpensiveObject()

let foo = { [expensiveProperty] in 
  expensiveProperty.doSomething()
}

// later...
foo()

In this example, expensiveProperty is initialized when foo is initialized, not when foo is called later on.

Copy link
Member

Choose a reason for hiding this comment

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

That makes sense -- I guess in a lot of typical usage we'd be initializing the monthHeaderDateFormatter for the defaultMonthHeaderItemProvider but then never even calling the default provider (because the consumer immediately provides a custom provider)

@@ -258,6 +258,10 @@ public final class CalendarView: UIView {
invalidateIntrinsicContentSize()
}

// Clear this so that we don't return early in our next layout pass even though our layout
// region might not have changed.
layoutRegion = nil
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we clear this in setContent, since we want updateVisibleViews(...) to run on the next layout pass even if our layout region hasn't changed

Comment on lines +744 to +749
if currentVisibleItemsDetails.layoutRegion != layoutRegion {
updateVisibleViews(withVisibleItems: currentVisibleItemsDetails.visibleItems)
}

visibleItemsDetails = currentVisibleItemsDetails
layoutRegion = currentVisibleItemsDetails.layoutRegion
Copy link
Contributor Author

Choose a reason for hiding this comment

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

here's the big optimization - when we're scrolling, we only need to updateVisibleViews(...) if we've scrolled far enough to change our "layout region" (month headers, days, etc. enter / exit the viewport)

@bryankeller bryankeller added the enhancement New feature or request label Sep 22, 2023
@bryankeller bryankeller merged commit 343cd82 into master Sep 22, 2023
@bryankeller bryankeller deleted the bk/performance-improvements branch September 22, 2023 23:53
bryankeller added a commit that referenced this pull request Sep 27, 2023
bryankeller added a commit that referenced this pull request Sep 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants