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

Bk/performance improvements #272

Merged
merged 3 commits into from
Sep 22, 2023
Merged

Bk/performance improvements #272

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
afdc0e0
Prevent DateFormatters from being initialized with CalendarViewContent
bryankeller Sep 22, 2023
5953065
Optimize layoutSubviews to prevent unnecessary work
bryankeller Sep 22, 2023
c47b89d
Lint
bryankeller Sep 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Changed `ItemView` to determine user interaction capabilities from its content view's `hitTest` / `pointInside` functions
- Updated content-change animations so that the same scroll offset is maintained throughout the animation
- Changed the Swift version needed to use HorizonCalendar to 5.8
- Optimized `layoutSubviews` to avoid doing unnecessary work updating views in some cases

## [v1.16.0](https://github.com/airbnb/HorizonCalendar/compare/v1.15.0...v1.16.0) - 2023-01-30

Expand Down
41 changes: 38 additions & 3 deletions Sources/Internal/VisibleItemsProvider.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,10 @@ final class VisibleItemsProvider {
-> LayoutItem
{
let layoutItem = LayoutItem(itemType: .monthHeader(month), frame: .zero)
var context = VisibleItemsContext(centermostLayoutItem: layoutItem, firstLayoutItem: layoutItem)
var context = VisibleItemsContext(
centermostLayoutItem: layoutItem,
firstLayoutItem: layoutItem,
lastLayoutItem: layoutItem)
let monthHeaderHeight = monthHeaderHeight(for: month, context: &context)

let monthOrigin: CGPoint
Expand Down Expand Up @@ -112,7 +115,10 @@ final class VisibleItemsProvider {
-> LayoutItem
{
let layoutItem = LayoutItem(itemType: .day(day), frame: .zero)
var context = VisibleItemsContext(centermostLayoutItem: layoutItem, firstLayoutItem: layoutItem)
var context = VisibleItemsContext(
centermostLayoutItem: layoutItem,
firstLayoutItem: layoutItem,
lastLayoutItem: layoutItem)
let month = day.month
let monthHeaderHeight = monthHeaderHeight(for: month, context: &context)

Expand Down Expand Up @@ -164,6 +170,7 @@ final class VisibleItemsProvider {
var context = VisibleItemsContext(
centermostLayoutItem: previouslyVisibleLayoutItem,
firstLayoutItem: previouslyVisibleLayoutItem,
lastLayoutItem: previouslyVisibleLayoutItem,
calendarItemModelCache: .init(
minimumCapacity: previousCalendarItemModelCache?.capacity ?? 100))

Expand Down Expand Up @@ -283,6 +290,7 @@ final class VisibleItemsProvider {
visibleItems: context.visibleItems,
centermostLayoutItem: context.centermostLayoutItem,
firstLayoutItem: context.firstLayoutItem,
lastLayoutItem: context.lastLayoutItem,
visibleDayRange: visibleDayRange,
visibleMonthRange: visibleMonthRange,
framesForVisibleMonths: context.framesForVisibleMonths,
Expand Down Expand Up @@ -340,7 +348,8 @@ final class VisibleItemsProvider {
var lastHandledLayoutItemEnumeratingForwards = previouslyVisibleLayoutItem
var context = VisibleItemsContext(
centermostLayoutItem: previouslyVisibleLayoutItem,
firstLayoutItem: previouslyVisibleLayoutItem)
firstLayoutItem: previouslyVisibleLayoutItem,
lastLayoutItem: previouslyVisibleLayoutItem)

layoutItemTypeEnumerator.enumerateItemTypes(
startingAt: previouslyVisibleLayoutItem.itemType,
Expand Down Expand Up @@ -417,6 +426,22 @@ final class VisibleItemsProvider {
return itemOrigin < otherItemOrigin ? item : otherItem
}

// Returns the layout item closest to the bottom/trailing edge of `bounds`.
private func lastLayoutItem(comparing item: LayoutItem, to otherItem: LayoutItem) -> LayoutItem {
let itemOrigin: CGFloat
let otherItemOrigin: CGFloat
switch content.monthsLayout {
case .vertical:
itemOrigin = item.frame.maxY
otherItemOrigin = otherItem.frame.maxY
case .horizontal:
itemOrigin = item.frame.maxX
otherItemOrigin = otherItem.frame.maxX
}

return itemOrigin > otherItemOrigin ? item : otherItem
}

private func boundsForAnimatedUpdatePass(atOffset offset: CGPoint) -> CGRect {
// Use a larger bounds (3x the viewport size) if we're in an animated update pass, reducing the
// likelihood of an item popping in / out.
Expand Down Expand Up @@ -866,6 +891,9 @@ final class VisibleItemsProvider {
context.firstLayoutItem = firstLayoutItem(
comparing: layoutItem,
to: context.firstLayoutItem)
context.lastLayoutItem = lastLayoutItem(
comparing: layoutItem,
to: context.lastLayoutItem)
}
} else {
shouldStop = true
Expand Down Expand Up @@ -1234,6 +1262,7 @@ final class VisibleItemsProvider {
private struct VisibleItemsContext {
var centermostLayoutItem: LayoutItem
var firstLayoutItem: LayoutItem
var lastLayoutItem: LayoutItem
var firstVisibleDay: Day?
var lastVisibleDay: Day?
var firstVisibleMonth: Month?
Expand All @@ -1258,6 +1287,7 @@ struct VisibleItemsDetails {
let visibleItems: Set<VisibleItem>
let centermostLayoutItem: LayoutItem
let firstLayoutItem: LayoutItem?
let lastLayoutItem: LayoutItem?
let visibleDayRange: DayRange?
let visibleMonthRange: MonthRange?
let framesForVisibleMonths: [Month: CGRect]
Expand All @@ -1271,6 +1301,11 @@ struct VisibleItemsDetails {
maxMonthHeight + heightOfPinnedContent
}

var layoutRegion: ClosedRange<LayoutItem.ItemType>? {
guard let firstLayoutItem, let lastLayoutItem else { return nil }
return firstLayoutItem.itemType...lastLayoutItem.itemType
}

}

// MARK: - _DayRangeLayoutContext
Expand Down
24 changes: 13 additions & 11 deletions Sources/Public/CalendarView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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


self.content = content
setNeedsLayout()

Expand Down Expand Up @@ -496,6 +500,7 @@ public final class CalendarView: UIView {
private var anchorLayoutItem: LayoutItem?
private var _visibleItemsProvider: VisibleItemsProvider?
private var visibleItemsDetails: VisibleItemsDetails?
private var layoutRegion: ClosedRange<LayoutItem.ItemType>?
private var visibleViewsForVisibleItems = [VisibleItem: ItemView]()

private var isAnimatedUpdatePass = false
Expand Down Expand Up @@ -734,11 +739,14 @@ public final class CalendarView: UIView {
isAnimatedUpdatePass: isAnimatedUpdatePass)
self.anchorLayoutItem = currentVisibleItemsDetails.centermostLayoutItem

updateVisibleViews(
withVisibleItems: currentVisibleItemsDetails.visibleItems,
previouslyVisibleItems: visibleItemsDetails?.visibleItems ?? [])
// If our first / last layout item hasn't changed, then we haven't scrolled enough to trigger
// an update of visible views. This short-circuit greatly improves scroll performance.
if currentVisibleItemsDetails.layoutRegion != layoutRegion {
updateVisibleViews(withVisibleItems: currentVisibleItemsDetails.visibleItems)
}

visibleItemsDetails = currentVisibleItemsDetails
layoutRegion = currentVisibleItemsDetails.layoutRegion
Comment on lines +744 to +749
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)


let minimumScrollOffset = visibleItemsDetails?.contentStartBoundary.map {
($0 - firstLayoutMarginValue).alignedToPixel(forScreenWithScale: scale)
Expand All @@ -760,10 +768,7 @@ public final class CalendarView: UIView {
}
}

private func updateVisibleViews(
withVisibleItems visibleItems: Set<VisibleItem>,
previouslyVisibleItems _: Set<VisibleItem>)
{
private func updateVisibleViews(withVisibleItems visibleItems: Set<VisibleItem>) {
var viewsToHideForVisibleItems = visibleViewsForVisibleItems
visibleViewsForVisibleItems.removeAll(keepingCapacity: true)

Expand Down Expand Up @@ -1158,10 +1163,7 @@ extension CalendarView {
guard cachedAccessibilityElements == nil else {
return cachedAccessibilityElements
}
guard
let visibleItemsDetails,
let visibleMonthRange
else {
guard let visibleItemsDetails, let visibleMonthRange else {
return nil
}

Expand Down
47 changes: 15 additions & 32 deletions Sources/Public/CalendarViewContent.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,12 +188,7 @@ public final class CalendarViewContent {
-> CalendarViewContent
{
self.monthHeaderItemProvider = { [defaultMonthHeaderItemProvider] month in
guard let itemModel = monthHeaderItemProvider(month) else {
// If the caller returned nil, fall back to the default item provider
return defaultMonthHeaderItemProvider(month)
}

return itemModel
monthHeaderItemProvider(month) ?? defaultMonthHeaderItemProvider(month)
}

return self
Expand Down Expand Up @@ -223,12 +218,7 @@ public final class CalendarViewContent {
-> CalendarViewContent
{
self.dayOfWeekItemProvider = { [defaultDayOfWeekItemProvider] month, weekdayIndex in
guard let itemModel = dayOfWeekItemProvider(month, weekdayIndex) else {
// If the caller returned nil, fall back to the default item provider
return defaultDayOfWeekItemProvider(month, weekdayIndex)
}

return itemModel
dayOfWeekItemProvider(month, weekdayIndex) ?? defaultDayOfWeekItemProvider(month, weekdayIndex)
}

return self
Expand All @@ -254,12 +244,7 @@ public final class CalendarViewContent {
-> CalendarViewContent
{
self.dayItemProvider = { [defaultDayItemProvider] day in
guard let itemModel = dayItemProvider(day) else {
// If the caller returned nil, fall back to the default item provider
return defaultDayItemProvider(day)
}

return itemModel
dayItemProvider(day) ?? defaultDayItemProvider(day)
}

return self
Expand Down Expand Up @@ -407,22 +392,20 @@ public final class CalendarViewContent {

/// The default `monthHeaderItemProvider` if no provider has been configured,
/// or if the existing provider returns nil.
private lazy var defaultMonthHeaderItemProvider: (Month) -> AnyCalendarItemModel = { [
calendar,
monthHeaderDateFormatter
] month in
let firstDateInMonth = calendar.firstDate(of: month)
let monthText = monthHeaderDateFormatter.string(from: firstDateInMonth)
let itemModel = MonthHeaderView.calendarItemModel(
invariantViewProperties: .base,
content: .init(monthText: monthText, accessibilityLabel: monthText))
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 •
edited
Loading

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)

let firstDateInMonth = calendar.firstDate(of: month)
let monthText = monthHeaderDateFormatter.string(from: firstDateInMonth)
let itemModel = MonthHeaderView.calendarItemModel(
invariantViewProperties: .base,
content: .init(monthText: monthText, accessibilityLabel: monthText))
return itemModel
}

/// The default `dayHeaderItemProvider` if no provider has been configured,
/// or if the existing provider returns nil.
private lazy var defaultDayOfWeekItemProvider: (Month?, Int)
-> AnyCalendarItemModel = { [monthHeaderDateFormatter] _, weekdayIndex in
private lazy var defaultDayOfWeekItemProvider: (Month?, Int) -> AnyCalendarItemModel =
{ [unowned self] _, weekdayIndex in
let dayOfWeekText = monthHeaderDateFormatter.veryShortStandaloneWeekdaySymbols[weekdayIndex]
let itemModel = DayOfWeekView.calendarItemModel(
invariantViewProperties: .base,
Expand All @@ -432,7 +415,7 @@ public final class CalendarViewContent {

/// The default `dayItemProvider` if no provider has been configured,
/// or if the existing provider returns nil.
private lazy var defaultDayItemProvider: (Day) -> AnyCalendarItemModel = { [calendar, dayDateFormatter] day in
private lazy var defaultDayItemProvider: (Day) -> AnyCalendarItemModel = { [unowned self] day in
let date = calendar.startDate(of: day)
let itemModel = DayView.calendarItemModel(
invariantViewProperties: .baseNonInteractive,
Expand Down