Skip to content

Commit

Permalink
Merge pull request #179 from wayfair/revert-178-performance
Browse files Browse the repository at this point in the history 
Revert "Fixed performance of height recalculation"
  • Loading branch information
@jay18001
jay18001 authored Aug 24, 2017
2 parents 197f799 + c79a952 commit 208035f
Show file tree
Hide file tree
Showing 20 changed files with 89 additions and 268 deletions.
12 changes: 0 additions & 12 deletions BrickKit.xcodeproj/project.pbxproj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -142,10 +142,6 @@
9333538F1E36A37F003CEC85 /* GenericBrickTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 9333538D1E36A37F003CEC85 /* GenericBrickTests.swift */; };
933FF95E1DC1207900E0B80E /* Swizzle.swift in Sources */ = {isa = PBXBuildFile; fileRef = 933FF95D1DC1207900E0B80E /* Swizzle.swift */; };
933FF95F1DC1207900E0B80E /* Swizzle.swift in Sources */ = {isa = PBXBuildFile; fileRef = 933FF95D1DC1207900E0B80E /* Swizzle.swift */; };
9340B6B21F4DBC2A005C28F0 /* BrickLogger.swift in Sources */ = {isa = PBXBuildFile; fileRef = 9340B6B11F4DBC2A005C28F0 /* BrickLogger.swift */; };
9340B6B31F4DBC2A005C28F0 /* BrickLogger.swift in Sources */ = {isa = PBXBuildFile; fileRef = 9340B6B11F4DBC2A005C28F0 /* BrickLogger.swift */; };
9340B6B51F4DE8F4005C28F0 /* BrickLoggerTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 9340B6B41F4DE8F4005C28F0 /* BrickLoggerTests.swift */; };
9340B6B61F4DE8F4005C28F0 /* BrickLoggerTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 9340B6B41F4DE8F4005C28F0 /* BrickLoggerTests.swift */; };
934ADAA31E53B4B200C36587 /* ButtonBrick.xib in Resources */ = {isa = PBXBuildFile; fileRef = 934ADA971E53B48200C36587 /* ButtonBrick.xib */; };
934ADAA41E53B4B200C36587 /* ImageBrick.xib in Resources */ = {isa = PBXBuildFile; fileRef = 934ADA9C1E53B49300C36587 /* ImageBrick.xib */; };
934ADAA51E53B4B200C36587 /* LabelBrick.xib in Resources */ = {isa = PBXBuildFile; fileRef = 934ADA991E53B48C00C36587 /* LabelBrick.xib */; };
Expand Down Expand Up @@ -315,8 +311,6 @@
933353891E368903003CEC85 /* GenericBrick.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; name = GenericBrick.swift; path = Generic/GenericBrick.swift; sourceTree = "<group>"; };
9333538D1E36A37F003CEC85 /* GenericBrickTests.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = GenericBrickTests.swift; sourceTree = "<group>"; };
933FF95D1DC1207900E0B80E /* Swizzle.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = Swizzle.swift; sourceTree = "<group>"; };
9340B6B11F4DBC2A005C28F0 /* BrickLogger.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = BrickLogger.swift; sourceTree = "<group>"; };
9340B6B41F4DE8F4005C28F0 /* BrickLoggerTests.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = BrickLoggerTests.swift; sourceTree = "<group>"; };
934ADA971E53B48200C36587 /* ButtonBrick.xib */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = file.xib; path = ButtonBrick.xib; sourceTree = "<group>"; };
934ADA991E53B48C00C36587 /* LabelBrick.xib */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = file.xib; path = LabelBrick.xib; sourceTree = "<group>"; };
934ADA9C1E53B49300C36587 /* ImageBrick.xib */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = file.xib; path = ImageBrick.xib; sourceTree = "<group>"; };
Expand Down Expand Up @@ -774,7 +768,6 @@
93D9EBE11DA4057000D8C87A /* BrickExtensions.swift */,
93D9EBE21DA4057000D8C87A /* BrickUtils.swift */,
93D9EBE31DA4057000D8C87A /* FatalError.swift */,
9340B6B11F4DBC2A005C28F0 /* BrickLogger.swift */,
);
path = Utils;
sourceTree = "<group>";
Expand Down Expand Up @@ -908,7 +901,6 @@
9372C5061DA40735007D7EA1 /* FatalErrorTests.swift */,
93D9EC4E1DA4057900D8C87A /* XCTests.swift */,
933FF95D1DC1207900E0B80E /* Swizzle.swift */,
9340B6B41F4DE8F4005C28F0 /* BrickLoggerTests.swift */,
);
path = Utils;
sourceTree = "<group>";
Expand Down Expand Up @@ -1165,7 +1157,6 @@
4E9A253B1DABEB9300D7EA99 /* BrickExtensions.swift in Sources */,
4E387C561DAEAB350087820E /* BrickAppearBehavior.swift in Sources */,
4E9A25331DABEB8800D7EA99 /* BrickLayoutSection.swift in Sources */,
9340B6B31F4DBC2A005C28F0 /* BrickLogger.swift in Sources */,
4E9A25311DABEB8800D7EA99 /* BrickLayout.swift in Sources */,
4E9A25211DABEB6000D7EA99 /* MinimumStickyLayoutBehavior.swift in Sources */,
4E9A25301DABEB8800D7EA99 /* BrickFlowLayout.swift in Sources */,
Expand Down Expand Up @@ -1245,7 +1236,6 @@
4E9A254C1DABECDF00D7EA99 /* StickyFooterLayoutBehaviorTests.swift in Sources */,
4E9A256B1DABED0600D7EA99 /* TestBrickViewController.swift in Sources */,
4E9A255C1DABECF500D7EA99 /* BrickSelfsizingFlowLayoutTests.swift in Sources */,
9340B6B61F4DE8F4005C28F0 /* BrickLoggerTests.swift in Sources */,
4E9A25501DABECE300D7EA99 /* ImageBrickTests.swift in Sources */,
932365731DF4FE1F00BE5183 /* BrickAlignmentTests.swift in Sources */,
4E387C571DAEAB380087820E /* BrickAppearBehaviorTests.swift in Sources */,
Expand Down Expand Up @@ -1282,7 +1272,6 @@
93D9EC0A1DA4057000D8C87A /* BrickLayoutInvalidationContext.swift in Sources */,
93F9B4D21DAD686E00927BE6 /* BrickAppearBehavior.swift in Sources */,
93D9EBF01DA4057000D8C87A /* OnScrollDownStickyLayoutBehavior.swift in Sources */,
9340B6B21F4DBC2A005C28F0 /* BrickLogger.swift in Sources */,
93D9EC151DA4057000D8C87A /* BrickCollectionView+BrickLayoutDataSource.swift in Sources */,
93D9EC121DA4057000D8C87A /* BrickExtensions.swift in Sources */,
93D9EBE91DA4057000D8C87A /* CardLayoutBehavior.swift in Sources */,
Expand Down Expand Up @@ -1329,7 +1318,6 @@
93D9EC821DA4057900D8C87A /* DataSources.swift in Sources */,
93D9EC7D1DA4057900D8C87A /* BrickSectionDataSourceTests.swift in Sources */,
93D9EC5D1DA4057900D8C87A /* SpotlightLayoutBehaviorTests.swift in Sources */,
9340B6B51F4DE8F4005C28F0 /* BrickLoggerTests.swift in Sources */,
9361309B1DB83CB7008FFFEF /* CustomBrickCollectionView.swift in Sources */,
93D9EC691DA4057900D8C87A /* DummyBrickWithoutNib.swift in Sources */,
93D9EC791DA4057900D8C87A /* BrickSelfsizingFlowLayoutTests.swift in Sources */,
Expand Down
22 changes: 7 additions & 15 deletions Source/Cells/BrickCell.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ open class BaseBrickCell: UICollectionViewCell {
}

// This value stores the expected width, so we can identify when this is met
private var requestedSize: CGSize = .zero
private var requestedWidth: CGFloat = 0

open func setContent(_ brick: Brick, index: Int, collectionIndex: Int, collectionIdentifier: String?) {
self._brick = brick
Expand Down Expand Up @@ -100,19 +100,23 @@ open class BaseBrickCell: UICollectionViewCell {
open override func apply(_ layoutAttributes: UICollectionViewLayoutAttributes) {
super.apply(layoutAttributes)

self.requestedSize = layoutAttributes.frame.size
self.requestedWidth = layoutAttributes.frame.width

// Setting zPosition instead of relaying on
// UICollectionView zIndex management 'fixes' the issue
// http://stackoverflow.com/questions/12659301/uicollectionview-setlayoutanimated-not-preserving-zindex
self.layer.zPosition = CGFloat(layoutAttributes.zIndex)

if self is AsynchronousResizableCell {
self.layoutIfNeeded()
}
}

open override func layoutSubviews() {
super.layoutSubviews()
brickBackgroundView?.frame = self.bounds

if _brick != nil && frame.width == requestedSize.width {
if _brick != nil && frame.width == requestedWidth {
self.layoutIfNeeded() // This layoutIfNeeded is added to make sure that the subviews are laid out correctly
self.framesDidLayout()
}
Expand Down Expand Up @@ -177,17 +181,12 @@ open class BrickCell: BaseBrickCell {
return UIEdgeInsetsMake(defaultTopConstraintConstant, defaultLeftConstraintConstant, defaultBottomConstraintConstant, defaultRightConstraintConstant)
}

private var didUpdateEdgeInsets: Bool = false
@objc open dynamic var edgeInsets: UIEdgeInsets = UIEdgeInsets.zero {
didSet {
if edgeInsets == oldValue {
return
}
self.topSpaceConstraint?.constant = edgeInsets.top
self.bottomSpaceConstraint?.constant = edgeInsets.bottom
self.leftSpaceConstraint?.constant = edgeInsets.left
self.rightSpaceConstraint?.constant = edgeInsets.right
didUpdateEdgeInsets = true
}
}

Expand Down Expand Up @@ -228,13 +227,6 @@ open class BrickCell: BaseBrickCell {
return layoutAttributes
}

if !didUpdateEdgeInsets {
guard let brickAttributes = layoutAttributes as? BrickLayoutAttributes, brickAttributes.isEstimateSize else {
return layoutAttributes
}
}
didUpdateEdgeInsets = false

let preferred = layoutAttributes

// We're inverting the frame because the given frame is already transformed
Expand Down
51 changes: 2 additions & 49 deletions Source/Layout/BrickFlowLayout.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,20 +32,6 @@ fileprivate func >= <T : Comparable>(lhs: T?, rhs: T?) -> Bool {
/// BrickFlowLayoutiis a UICollectionViewLayout that can handle behaviors
open class BrickFlowLayout: UICollectionViewLayout, BrickLayout {

var isDirty: Bool {
get {
return !dirtyMap.isEmpty
}
set {
if !newValue {
dirtyMap.removeAll()
dirtyIndexPaths.removeAll()
}
}
}
var dirtyMap: [Int: Int] = [:]
var dirtyIndexPaths: [IndexPath] = []

// Mark: - Public members

open override var description: String {
Expand Down Expand Up @@ -298,25 +284,7 @@ extension BrickFlowLayout {
return contentSize
}

func updateDirtyBricks(updatedAttributes: @escaping OnAttributesUpdatedHandler) {
for (section, item) in dirtyMap {
let layoutSection = sections![section]!
layoutSection.createOrUpdateCells(from: item, invalidate: false, updatedAttributes: updatedAttributes)
if let sectionAttributes = layoutSection.sectionAttributes {
invalidateHeight(for: sectionAttributes.indexPath, updatedAttributes: updatedAttributes)
}
}

recalculateContentSize()
dirtyIndexPaths.forEach { indexPath in
delegate?.brickLayout(self, didUpdateHeightForItemAtIndexPath: indexPath)
}

isDirty = false
}

open override func invalidateLayout(with context: UICollectionViewLayoutInvalidationContext) {
BrickLogger.logVerbose(message: "Invalidate layout with context \(context)")
guard sections != nil else { // No need to invalidate if there are no sections
super.invalidateLayout(with: context)
return
Expand Down Expand Up @@ -384,21 +352,7 @@ extension BrickFlowLayout {
}
let shouldInvalidate = preferredAttributes.frame.height != brickAttribute.originalFrame.height
brickAttribute.isEstimateSize = false

// return shouldInvalidate
if shouldInvalidate {
let indexPath = preferredAttributes.indexPath
sections![indexPath.section]?.update(height: preferredAttributes.frame.height, at: indexPath.item, continueCalculation: false, updatedAttributes: nil)
if let current = dirtyMap[indexPath.section] {
if indexPath.item < current {
dirtyMap[indexPath.section] = indexPath.item
}
} else {
dirtyMap[indexPath.section] = indexPath.item
}
dirtyIndexPaths.append(indexPath)
}
return false
return shouldInvalidate
}

open override func invalidationContext(forPreferredLayoutAttributes preferredAttributes: UICollectionViewLayoutAttributes, withOriginalAttributes originalAttributes: UICollectionViewLayoutAttributes) -> UICollectionViewLayoutInvalidationContext {
Expand Down Expand Up @@ -533,7 +487,7 @@ extension BrickFlowLayout: BrickLayoutInvalidationProvider {
let contentOffsetAdjustment = shouldAdjustContentOffset ? height - firstAttributes.frame.height : 0

updateSection(section, updatedAttributes: updatedAttributes) {
section.update(height: height, at: indexPath.item, continueCalculation: true, updatedAttributes: { attributes, oldFrame in
section.update(height: height, at: indexPath.item, updatedAttributes: { attributes, oldFrame in
updatedAttributes(attributes, oldFrame)
self.attributesWereUpdated(attributes, oldFrame: oldFrame, fromBehaviors: false, updatedAttributes: updatedAttributes)
})
Expand Down Expand Up @@ -590,7 +544,6 @@ extension BrickFlowLayout: BrickLayoutInvalidationProvider {
self.contentSize = contentSize
}

@discardableResult
func recalculateContentSize() -> CGSize {
let oldContentSize = self.contentSize
contentSize = sections?[0]?.frame.size ?? CGSize.zero
Expand Down
21 changes: 7 additions & 14 deletions Source/Layout/BrickLayoutInvalidationContext.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ enum BrickLayoutInvalidationContextType {
case behaviorsChanged
case invalidate
case updateVisibility
case updateDirtyBricks

/**
Flag that indicates if all attributes should be invalidated.
Expand All @@ -26,7 +25,7 @@ enum BrickLayoutInvalidationContextType {
*/
var shouldInvalidateAllAttributes: Bool {
switch self {
case .rotation, .invalidate, .creation, .updateVisibility, .updateDirtyBricks /*, .updateHeight(_) */: return true
case .rotation, .invalidate, .creation, .updateVisibility, .updateHeight(_): return true
default: return false
}
}
Expand All @@ -45,7 +44,6 @@ protocol BrickLayoutInvalidationProvider: class {
func registerUpdatedAttributes(_ attributes: BrickLayoutAttributes, oldFrame: CGRect?, fromBehaviors: Bool, updatedAttributes: @escaping OnAttributesUpdatedHandler)
func applyHideBehavior(updatedAttributes: @escaping OnAttributesUpdatedHandler)
func updateContentSize(_ contentSize: CGSize)
func updateDirtyBricks(updatedAttributes: @escaping OnAttributesUpdatedHandler)
}

extension BrickLayoutInvalidationContext {
Expand Down Expand Up @@ -107,8 +105,6 @@ class BrickLayoutInvalidationContext: UICollectionViewLayoutInvalidationContext
self.invalidateSections(provider, layout: layout)
case .updateVisibility:
self.applyHideBehaviors(provider, updatedAttributes: updateAttributes)
case .updateDirtyBricks:
provider.updateDirtyBricks(updatedAttributes: updateAttributes)
default: break
}

Expand Down Expand Up @@ -160,17 +156,14 @@ class BrickLayoutInvalidationContext: UICollectionViewLayoutInvalidationContext
}

func handleAttributes(_ attributes: BrickLayoutAttributes, oldFrame: CGRect?, provider: BrickLayoutInvalidationProvider, layout: UICollectionViewLayout, fromBehaviors: Bool) {
if !type.shouldInvalidateAllAttributes {
if !updatedAttributes.contains(attributes) {
updatedAttributes.append(attributes)
}
}

if !updatedAttributes.contains(attributes) {
updatedAttributes.append(attributes)
}

provider.registerUpdatedAttributes(attributes, oldFrame: oldFrame, fromBehaviors: fromBehaviors, updatedAttributes: { attributes, oldFrame in
if !self.type.shouldInvalidateAllAttributes {
if !self.updatedAttributes.contains(attributes) {
self.updatedAttributes.append(attributes)
}
if !self.updatedAttributes.contains(attributes) {
self.updatedAttributes.append(attributes)
}
})
}
Expand Down
49 changes: 23 additions & 26 deletions Source/Layout/BrickLayoutSection.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -239,20 +239,18 @@ internal class BrickLayoutSection {
attributes.frame.size.width = 0
}

func update(height: CGFloat, at index: Int, continueCalculation: Bool, updatedAttributes: OnAttributesUpdatedHandler?) {
func update(height: CGFloat, at index: Int, updatedAttributes: OnAttributesUpdatedHandler?) {
guard let brickAttributes = attributes[index] else {
return
}
brickAttributes.isEstimateSize = false

// guard brickAttributes.originalFrame.height != height else {
// return
// }
guard brickAttributes.originalFrame.height != height else {
return
}

brickAttributes.originalFrame.size.height = height
if continueCalculation {
createOrUpdateCells(from: index, invalidate: false, updatedAttributes: updatedAttributes)
}
createOrUpdateCells(from: index, invalidate: false, updatedAttributes: updatedAttributes)
}

func invalidate(at index: Int, updatedAttributes: OnAttributesUpdatedHandler?) {
Expand Down Expand Up @@ -309,7 +307,7 @@ internal class BrickLayoutSection {
/// - firstIndex: The index the calculation needs to start from (the main reason is to just calculate the next cells
/// - invalidate: Identifies if the attributes need to be invalidated (reset height etc)
/// - updatedAttributes: Callback for the attributes that have been updated
public func createOrUpdateCells(from firstIndex: Int, invalidate: Bool, updatedAttributes: OnAttributesUpdatedHandler?) {
fileprivate func createOrUpdateCells(from firstIndex: Int, invalidate: Bool, updatedAttributes: OnAttributesUpdatedHandler?) {

guard let dataSource = dataSource else {
return
Expand Down Expand Up @@ -403,10 +401,10 @@ internal class BrickLayoutSection {
frame.size.width = x + edgeInsets.right
}

// if attributes.count < 100 {
// // Prevent that the "Huge" test aren't taking forever to complete
// BrickLogger.logVerbose(message: self.printAttributes())
// }

if brickDebug {
printAttributes()
}
}

/// To continue calculating, it needs to start from a certain origin. To make sure that the rows are
Expand Down Expand Up @@ -574,21 +572,19 @@ internal class BrickLayoutSection {
return rowAttributes
}

func printAttributes() -> String {
var debugString = ""
debugString += "\n"
debugString += "Attributes for section \(sectionIndex) in \(dataSource!)"
debugString += "\n"
debugString += "Number of attributes: \(attributes.count) in frameOfInterest \(_dataSource.frameOfInterest)"
debugString += "\n"
debugString += "Total Frame: \(self.frame)"
debugString += "\n"
func printAttributes() {
guard attributes.count < 100 else {
// Prevent that the "Huge" test aren't taking forever to complete
return
}
BrickUtils.print("\n")
BrickUtils.print("Attributes for section \(sectionIndex) in \(String(describing: dataSource))")
BrickUtils.print("Number of attributes: \(attributes.count) in \(_dataSource.frameOfInterest)")
BrickUtils.print("Frame: \(self.frame)")
let keys = attributes.keys.sorted(by: <)
for key in keys {
debugString += "\(key): \(attributes[key]!)"
debugString += "\n"
BrickUtils.print("\(key): \(attributes[key]!)")
}
return debugString
}

/// Create or update 1 cell
Expand Down Expand Up @@ -765,10 +761,11 @@ extension BrickLayoutSection {
attributes.append(brickAttributes)
}
return true
} else {
// if the frame is not the same as the originalFrame, continue checking because the attribute could be offscreen
return brickAttributes.frame != brickAttributes.originalFrame
}

// if the frame is not the same as the originalFrame, continue checking because the attribute could be offscreen
return brickAttributes.frame != brickAttributes.originalFrame
}

// Go back to see if previous attributes are not closer
Expand Down
Loading

0 comments on commit 208035f

Please sign in to comment.