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

Mark ASRunLoopQueue as drained if it contains only NULLs #558

Merged
merged 7 commits into from
Sep 11, 2017
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
4 changes: 4 additions & 0 deletions AsyncDisplayKit.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@
3917EBD41E9C2FC400D04A01 /* _ASCollectionReusableView.h in Headers */ = {isa = PBXBuildFile; fileRef = 3917EBD21E9C2FC400D04A01 /* _ASCollectionReusableView.h */; settings = {ATTRIBUTES = (Private, ); }; };
3917EBD51E9C2FC400D04A01 /* _ASCollectionReusableView.m in Sources */ = {isa = PBXBuildFile; fileRef = 3917EBD31E9C2FC400D04A01 /* _ASCollectionReusableView.m */; };
3C9C128519E616EF00E942A0 /* ASTableViewTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = 3C9C128419E616EF00E942A0 /* ASTableViewTests.mm */; };
4E9127691F64157600499623 /* ASRunLoopQueueTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 4E9127681F64157600499623 /* ASRunLoopQueueTests.m */; };
509E68601B3AED8E009B9150 /* ASScrollDirection.m in Sources */ = {isa = PBXBuildFile; fileRef = 205F0E111B371BD7007741D0 /* ASScrollDirection.m */; };
509E68611B3AEDA0009B9150 /* ASAbstractLayoutController.h in Headers */ = {isa = PBXBuildFile; fileRef = 205F0E171B37339C007741D0 /* ASAbstractLayoutController.h */; };
509E68621B3AEDA5009B9150 /* ASAbstractLayoutController.mm in Sources */ = {isa = PBXBuildFile; fileRef = 205F0E181B37339C007741D0 /* ASAbstractLayoutController.mm */; };
Expand Down Expand Up @@ -624,6 +625,7 @@
4640521B1A3F83C40061C0BA /* ASTableLayoutController.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = ASTableLayoutController.h; sourceTree = "<group>"; };
4640521C1A3F83C40061C0BA /* ASTableLayoutController.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = ASTableLayoutController.m; sourceTree = "<group>"; };
4640521D1A3F83C40061C0BA /* ASLayoutController.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = ASLayoutController.h; sourceTree = "<group>"; };
4E9127681F64157600499623 /* ASRunLoopQueueTests.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = ASRunLoopQueueTests.m; sourceTree = "<group>"; };
68355B2E1CB5799E001D4E68 /* ASImageNode+AnimatedImage.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = "ASImageNode+AnimatedImage.mm"; sourceTree = "<group>"; };
68355B361CB57A5A001D4E68 /* ASPINRemoteImageDownloader.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = ASPINRemoteImageDownloader.m; sourceTree = "<group>"; };
68355B371CB57A5A001D4E68 /* ASImageContainerProtocolCategories.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = ASImageContainerProtocolCategories.h; sourceTree = "<group>"; };
Expand Down Expand Up @@ -1219,6 +1221,7 @@
69FEE53C1D95A9AF0086F066 /* ASLayoutElementStyleTests.m */,
695BE2541DC1245C008E6EA5 /* ASWrapperSpecSnapshotTests.mm */,
699B83501E3C1BA500433FA4 /* ASLayoutSpecTests.m */,
4E9127681F64157600499623 /* ASRunLoopQueueTests.m */,
);
path = Tests;
sourceTree = "<group>";
Expand Down Expand Up @@ -2140,6 +2143,7 @@
696FCB311D6E46050093471E /* ASBackgroundLayoutSpecSnapshotTests.mm in Sources */,
CC583AD81EF9BDC300134156 /* OCMockObject+ASAdditions.m in Sources */,
69FEE53D1D95A9AF0086F066 /* ASLayoutElementStyleTests.m in Sources */,
4E9127691F64157600499623 /* ASRunLoopQueueTests.m in Sources */,
CC4981B31D1A02BE004E13CC /* ASTableViewThrashTests.m in Sources */,
CC54A81E1D7008B300296A24 /* ASDispatchTests.m in Sources */,
CCE4F9B31F0D60AC00062E4E /* ASIntegerMapTests.m in Sources */,
Expand Down
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
- Fixed a memory corruption issue in the ASImageNode display system. [Adlai Holler](https://github.com/Adlai-Holler) [#555](https://github.com/TextureGroup/Texture/pull/555)
- [Breaking] Rename ASCollectionGalleryLayoutSizeProviding to ASCollectionGalleryLayoutPropertiesProviding. Besides a fixed item size, it now can provide interitem and line spacings, as well as section inset [Huy Nguyen](https://github.com/nguyenhuy) [#496](https://github.com/TextureGroup/Texture/pull/496) [#533](https://github.com/TextureGroup/Texture/pull/533)
- Deprecate `-[ASDisplayNode displayWillStart]` in favor of `-displayWillStartAsynchronously:` [Huy Nguyen](https://github.com/nguyenhuy) [536](https://github.com/TextureGroup/Texture/pull/536)
- Mark ASRunLoopQueue as drained if it contains only NULLs [Cesar Estebanez](https://github.com/cesteban)
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Please add the PR's URL.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, sorry!


##2.4
- Fix an issue where inserting/deleting sections could lead to inconsistent supplementary element behavior. [Adlai Holler](https://github.com/Adlai-Holler)
Expand Down
2 changes: 2 additions & 0 deletions Source/ASRunLoopQueue.h
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ AS_SUBCLASSING_RESTRICTED

- (void)enqueue:(ObjectType)object;

@property (nonatomic, readonly) BOOL isEmpty;

@property (nonatomic, assign) NSUInteger batchSize; // Default == 1.
@property (nonatomic, assign) BOOL ensureExclusiveMembership; // Default == YES. Set-like behavior.

Expand Down
24 changes: 16 additions & 8 deletions Source/ASRunLoopQueue.mm
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,7 @@ @interface ASRunLoopQueue () {
CFRunLoopSourceRef _runLoopSource;
CFRunLoopObserverRef _runLoopObserver;
NSPointerArray *_internalQueue; // Use NSPointerArray so we can decide __strong or __weak per-instance.
BOOL _isQueueDrained;
ASDN::RecursiveMutex _internalQueueLock;

// In order to not pollute the top-level activities, each queue has 1 root activity.
Expand Down Expand Up @@ -277,6 +278,7 @@ - (instancetype)initWithRunLoop:(CFRunLoopRef)runloop retainObjects:(BOOL)retain
_queueConsumer = handlerBlock;
_batchSize = 1;
_ensureExclusiveMembership = YES;
_isQueueDrained = YES;

// We don't want to pollute the top-level app activities with run loop batches, so we create one top-level
// activity per queue, and each batch activity joins that one instead.
Expand Down Expand Up @@ -344,19 +346,18 @@ - (void)processQueue
// This is to avoid needlessly retaining/releasing the objects if we don't have a block.
std::vector<id> itemsToProcess;

BOOL isQueueDrained = NO;
{
ASDN::MutexLocker l(_internalQueueLock);

// Early-exit if the queue is empty.
NSInteger internalQueueCount = _internalQueue.count;
if (internalQueueCount == 0) {
if (_isQueueDrained) {
return;
}

ASSignpostStart(ASSignpostRunLoopQueueBatch);

// Snatch the next batch of items.
NSInteger internalQueueCount = _internalQueue.count;
NSInteger maxCountToProcess = MIN(internalQueueCount, self.batchSize);

/**
Expand All @@ -383,8 +384,8 @@ - (void)processQueue
}

[_internalQueue compact];
if (_internalQueue.count == 0) {
isQueueDrained = YES;
if (_internalQueue.count == 0 || foundItemCount == 0) {
_isQueueDrained = YES;
}
}

Expand All @@ -395,7 +396,7 @@ - (void)processQueue
auto itemsEnd = itemsToProcess.cend();
for (auto iterator = itemsToProcess.begin(); iterator < itemsEnd; iterator++) {
__unsafe_unretained id value = *iterator;
_queueConsumer(value, isQueueDrained && iterator == itemsEnd - 1);
_queueConsumer(value, _isQueueDrained && iterator == itemsEnd - 1);
as_log_verbose(ASDisplayLog(), "processed %@", value);
}
if (count > 1) {
Expand All @@ -404,7 +405,7 @@ - (void)processQueue
}

// If the queue is not fully drained yet force another run loop to process next batch of items
if (!isQueueDrained) {
if (!_isQueueDrained) {
CFRunLoopSourceSignal(_runLoopSource);
CFRunLoopWakeUp(_runLoop);
}
Expand Down Expand Up @@ -434,10 +435,17 @@ - (void)enqueue:(id)object

if (!foundObject) {
[_internalQueue addPointer:(__bridge void *)object];
_isQueueDrained = NO;

CFRunLoopSourceSignal(_runLoopSource);
CFRunLoopWakeUp(_runLoop);
}
}

- (BOOL)isEmpty
{
ASDN::MutexLocker l(_internalQueueLock);
return _isQueueDrained;
}

@end
165 changes: 165 additions & 0 deletions Tests/ASRunLoopQueueTests.m
Original file line number Diff line number Diff line change
@@ -0,0 +1,165 @@
//
// ASRunLoopQueueTests.m
// Texture
//
// Copyright (c) 2014-present, Facebook, Inc. All rights reserved.
// This source code is licensed under the BSD-style license found in the
// LICENSE file in the /ASDK-Licenses directory of this source tree. An additional
// grant of patent rights can be found in the PATENTS file in the same directory.
//
// Modifications to this file made after 4/13/2017 are: Copyright (c) 2017-present,
// Pinterest, Inc. Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//

#import <XCTest/XCTest.h>
#import <AsyncDisplayKit/ASRunLoopQueue.h>

static NSTimeInterval const kRunLoopRunTime = 0.001; // Allow the RunLoop to run for one millisecond each time.

@interface ASRunLoopQueueTests : XCTestCase

@end

@implementation ASRunLoopQueueTests

#pragma mark enqueue tests

- (void)testEnqueueNilObjectsToQueue
{
ASRunLoopQueue *queue = [[ASRunLoopQueue alloc] initWithRunLoop:CFRunLoopGetMain() retainObjects:YES handler:nil];
id object = nil;
[queue enqueue:object];
XCTAssertTrue(queue.isEmpty);
}

- (void)testEnqueueSameObjectTwiceToDefaultQueue
{
id object = [[NSObject alloc] init];
__unsafe_unretained id weakObject = object;
__block NSUInteger dequeuedCount = 0;
ASRunLoopQueue *queue = [[ASRunLoopQueue alloc] initWithRunLoop:CFRunLoopGetMain() retainObjects:YES handler:^(id _Nonnull dequeuedItem, BOOL isQueueDrained) {
if (dequeuedItem == weakObject) {
dequeuedCount++;
}
}];
[queue enqueue:object];
[queue enqueue:object];
[[NSRunLoop mainRunLoop] runUntilDate:[NSDate dateWithTimeIntervalSinceNow:kRunLoopRunTime]];
XCTAssert(dequeuedCount == 1);
}

- (void)testEnqueueSameObjectTwiceToNonExclusiveMembershipQueue
{
id object = [[NSObject alloc] init];
__unsafe_unretained id weakObject = object;
__block NSUInteger dequeuedCount = 0;
ASRunLoopQueue *queue = [[ASRunLoopQueue alloc] initWithRunLoop:CFRunLoopGetMain() retainObjects:YES handler:^(id _Nonnull dequeuedItem, BOOL isQueueDrained) {
if (dequeuedItem == weakObject) {
dequeuedCount++;
}
}];
queue.ensureExclusiveMembership = NO;
[queue enqueue:object];
[queue enqueue:object];
[[NSRunLoop mainRunLoop] runUntilDate:[NSDate dateWithTimeIntervalSinceNow:kRunLoopRunTime]];
XCTAssert(dequeuedCount == 2);
}

#pragma mark processQueue tests

- (void)testDefaultQueueProcessObjectsOneAtATime
{
ASRunLoopQueue *queue = [[ASRunLoopQueue alloc] initWithRunLoop:CFRunLoopGetMain() retainObjects:YES handler:^(id _Nonnull dequeuedItem, BOOL isQueueDrained) {
[NSThread sleepForTimeInterval:kRunLoopRunTime * 2]; // So each element takes more time than the available
}];
[queue enqueue:[[NSObject alloc] init]];
[queue enqueue:[[NSObject alloc] init]];
[[NSRunLoop mainRunLoop] runUntilDate:[NSDate dateWithTimeIntervalSinceNow:kRunLoopRunTime]];
XCTAssertFalse(queue.isEmpty);
}

- (void)testQueueProcessObjectsInBatchesOfSpecifiedSize
{
ASRunLoopQueue *queue = [[ASRunLoopQueue alloc] initWithRunLoop:CFRunLoopGetMain() retainObjects:YES handler:^(id _Nonnull dequeuedItem, BOOL isQueueDrained) {
[NSThread sleepForTimeInterval:kRunLoopRunTime * 2]; // So each element takes more time than the available
}];
queue.batchSize = 2;
[queue enqueue:[[NSObject alloc] init]];
[queue enqueue:[[NSObject alloc] init]];
[[NSRunLoop mainRunLoop] runUntilDate:[NSDate dateWithTimeIntervalSinceNow:kRunLoopRunTime]];
XCTAssertTrue(queue.isEmpty);
}

- (void)testQueueOnlySendsIsDrainedForLastObjectInBatch
{
id objectA = [[NSObject alloc] init];
id objectB = [[NSObject alloc] init];
__unsafe_unretained id weakObjectA = objectA;
__unsafe_unretained id weakObjectB = objectB;
__block BOOL isQueueDrainedWhenProcessingA = NO;
__block BOOL isQueueDrainedWhenProcessingB = NO;
ASRunLoopQueue *queue = [[ASRunLoopQueue alloc] initWithRunLoop:CFRunLoopGetMain() retainObjects:YES handler:^(id _Nonnull dequeuedItem, BOOL isQueueDrained) {
if (dequeuedItem == weakObjectA) {
isQueueDrainedWhenProcessingA = isQueueDrained;
} else if (dequeuedItem == weakObjectB) {
isQueueDrainedWhenProcessingB = isQueueDrained;
}
}];
queue.batchSize = 2;
[queue enqueue:objectA];
[queue enqueue:objectB];
[[NSRunLoop mainRunLoop] runUntilDate:[NSDate dateWithTimeIntervalSinceNow:kRunLoopRunTime]];
XCTAssertFalse(isQueueDrainedWhenProcessingA);
XCTAssertTrue(isQueueDrainedWhenProcessingB);
}

#pragma mark strong/weak tests

- (void)testStrongQueueRetainsObjects
{
id object = [[NSObject alloc] init];
__unsafe_unretained id weakObject = object;
__block BOOL didProcessObject = NO;
ASRunLoopQueue *queue = [[ASRunLoopQueue alloc] initWithRunLoop:CFRunLoopGetMain() retainObjects:YES handler:^(id _Nonnull dequeuedItem, BOOL isQueueDrained) {
if (dequeuedItem == weakObject) {
didProcessObject = YES;
}
}];
[queue enqueue:object];
object = nil;
[[NSRunLoop mainRunLoop] runUntilDate:[NSDate dateWithTimeIntervalSinceNow:kRunLoopRunTime]];
XCTAssertTrue(didProcessObject);
}

- (void)testWeakQueueDoesNotRetainsObjects
{
id object = [[NSObject alloc] init];
__unsafe_unretained id weakObject = object;
__block BOOL didProcessObject = NO;
ASRunLoopQueue *queue = [[ASRunLoopQueue alloc] initWithRunLoop:CFRunLoopGetMain() retainObjects:NO handler:^(id _Nonnull dequeuedItem, BOOL isQueueDrained) {
if (dequeuedItem == weakObject) {
didProcessObject = YES;
}
}];
[queue enqueue:object];
object = nil;
[[NSRunLoop mainRunLoop] runUntilDate:[NSDate dateWithTimeIntervalSinceNow:kRunLoopRunTime]];
XCTAssertFalse(didProcessObject);
}

- (void)testWeakQueueWithAllDeallocatedObjectsIsDrained
{
ASRunLoopQueue *queue = [[ASRunLoopQueue alloc] initWithRunLoop:CFRunLoopGetMain() retainObjects:NO handler:nil];
id object = [[NSObject alloc] init];
[queue enqueue:object];
object = nil;
XCTAssertFalse(queue.isEmpty);
[[NSRunLoop mainRunLoop] runUntilDate:[NSDate dateWithTimeIntervalSinceNow:kRunLoopRunTime]];
XCTAssertTrue(queue.isEmpty);
}

@end