From 13792964839f6f29515d2e1314491f5477410e9d Mon Sep 17 00:00:00 2001 From: Jeff Tung <100387939+jtung-apple@users.noreply.github.com> Date: Fri, 2 Jun 2023 22:15:08 -0700 Subject: [PATCH] [Darwin] MTRDevice should coalesce reads and avoid duplicates (#26999) * [Darwin] MTRDevice should coalesce reads and avoid duplicates * Properly hook up the duplicate check, and add logging * Update src/darwin/Framework/CHIP/MTRAsyncCallbackWorkQueue.h Co-authored-by: Boris Zbarsky * Update src/darwin/Framework/CHIP/MTRAsyncCallbackWorkQueue.h Co-authored-by: Boris Zbarsky * Update src/darwin/Framework/CHIP/MTRAsyncCallbackWorkQueue.h Co-authored-by: Boris Zbarsky * Update src/darwin/Framework/CHIP/MTRAsyncCallbackWorkQueue.h Co-authored-by: Boris Zbarsky * Update src/darwin/Framework/CHIP/MTRAsyncCallbackWorkQueue.h Co-authored-by: Boris Zbarsky * Moved MTRAsyncCallbackQueueTests additional queuing into readyHandler to avoid potential race * Moved batching logging for clarity * Address the last review comments * Update src/darwin/Framework/CHIP/MTRAsyncCallbackWorkQueue.mm Co-authored-by: Boris Zbarsky * Addressed review commends and updated logic to handle writes/commands * Update src/darwin/Framework/CHIP/MTRAsyncCallbackWorkQueue_Internal.h Co-authored-by: Boris Zbarsky * Update src/darwin/Framework/CHIP/MTRAsyncCallbackWorkQueue_Internal.h Co-authored-by: Boris Zbarsky * Apply suggestions from code review Co-authored-by: Boris Zbarsky * Moved comment doc to better position --------- Co-authored-by: Boris Zbarsky --- .../CHIP/MTRAsyncCallbackWorkQueue.h | 2 - .../CHIP/MTRAsyncCallbackWorkQueue.mm | 63 ++++- .../CHIP/MTRAsyncCallbackWorkQueue_Internal.h | 71 ++++++ src/darwin/Framework/CHIP/MTRDevice.mm | 166 ++++++++++++-- .../CHIPTests/MTRAsyncCallbackQueueTests.m | 217 +++++++++++++++++- .../Framework/CHIPTests/MTRDeviceTests.m | 26 +++ 6 files changed, 516 insertions(+), 29 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRAsyncCallbackWorkQueue.h b/src/darwin/Framework/CHIP/MTRAsyncCallbackWorkQueue.h index acb6da8053327c..346af1edfd7fcf 100644 --- a/src/darwin/Framework/CHIP/MTRAsyncCallbackWorkQueue.h +++ b/src/darwin/Framework/CHIP/MTRAsyncCallbackWorkQueue.h @@ -56,8 +56,6 @@ typedef void (^MTRAsyncCallbackReadyHandler)(id context, NSUInteger retryCount); // Work items may be enqueued from any queue or thread // Note: Once a work item is enqueued, its handlers cannot be modified - (void)enqueueWorkItem:(MTRAsyncCallbackQueueWorkItem *)item; - -// TODO: Add a "set concurrency width" method to allow for more than 1 work item at a time @end // An item in the work queue diff --git a/src/darwin/Framework/CHIP/MTRAsyncCallbackWorkQueue.mm b/src/darwin/Framework/CHIP/MTRAsyncCallbackWorkQueue.mm index 3d8b04eaabebc6..193a4ce7a038d2 100644 --- a/src/darwin/Framework/CHIP/MTRAsyncCallbackWorkQueue.mm +++ b/src/darwin/Framework/CHIP/MTRAsyncCallbackWorkQueue.mm @@ -18,7 +18,7 @@ #import #import -#import "MTRAsyncCallbackWorkQueue.h" +#import "MTRAsyncCallbackWorkQueue_Internal.h" #import "MTRLogging_Internal.h" #pragma mark - Class extensions @@ -169,9 +169,50 @@ - (void)_callNextReadyWorkItem self.runningWorkItemCount = 1; MTRAsyncCallbackQueueWorkItem * workItem = self.items.firstObject; + + // Check if batching is possible or needed. Only ask work item to batch once for simplicity + if (workItem.batchable && workItem.batchingHandler && (workItem.retryCount == 0)) { + while (self.items.count >= 2) { + MTRAsyncCallbackQueueWorkItem * nextWorkItem = self.items[1]; + if (!nextWorkItem.batchable || (nextWorkItem.batchingID != workItem.batchingID)) { + // next item is not eligible to merge with this one + break; + } + + BOOL fullyMerged = NO; + workItem.batchingHandler(workItem.batchableData, nextWorkItem.batchableData, &fullyMerged); + if (!fullyMerged) { + // We can't remove the next work item, so we can't merge anything else into this one. + break; + } + + [self.items removeObjectAtIndex:1]; + } + } + [workItem callReadyHandlerWithContext:self.context]; } } + +- (BOOL)isDuplicateForTypeID:(NSUInteger)opaqueDuplicateTypeID workItemData:(id)opaqueWorkItemData +{ + os_unfair_lock_lock(&_lock); + // Start from the last item + for (NSUInteger i = self.items.count; i > 0; i--) { + MTRAsyncCallbackQueueWorkItem * item = self.items[i - 1]; + BOOL isDuplicate = NO; + BOOL stop = NO; + if (item.supportsDuplicateCheck && (item.duplicateTypeID == opaqueDuplicateTypeID) && item.duplicateCheckHandler) { + item.duplicateCheckHandler(opaqueWorkItemData, &isDuplicate, &stop); + if (stop) { + os_unfair_lock_unlock(&_lock); + return isDuplicate; + } + } + } + os_unfair_lock_unlock(&_lock); + return NO; +} @end @implementation MTRAsyncCallbackQueueWorkItem @@ -277,4 +318,24 @@ - (void)cancel }); } } + +- (void)setBatchingID:(NSUInteger)opaqueBatchingID + data:(id)opaqueBatchableData + handler:(MTRAsyncCallbackBatchingHandler)batchingHandler +{ + os_unfair_lock_lock(&self->_lock); + _batchable = YES; + _batchingID = opaqueBatchingID; + _batchableData = opaqueBatchableData; + _batchingHandler = batchingHandler; + os_unfair_lock_unlock(&self->_lock); +} + +- (void)setDuplicateTypeID:(NSUInteger)opaqueDuplicateTypeID handler:(MTRAsyncCallbackDuplicateCheckHandler)duplicateCheckHandler +{ + _supportsDuplicateCheck = YES; + _duplicateTypeID = opaqueDuplicateTypeID; + _duplicateCheckHandler = duplicateCheckHandler; +} + @end diff --git a/src/darwin/Framework/CHIP/MTRAsyncCallbackWorkQueue_Internal.h b/src/darwin/Framework/CHIP/MTRAsyncCallbackWorkQueue_Internal.h index 49e6343355a6d5..7abbb9cf10a83c 100644 --- a/src/darwin/Framework/CHIP/MTRAsyncCallbackWorkQueue_Internal.h +++ b/src/darwin/Framework/CHIP/MTRAsyncCallbackWorkQueue_Internal.h @@ -23,9 +23,80 @@ NS_ASSUME_NONNULL_BEGIN @class MTRDevice; +// Optional feature: Work Item Batching +// When a work item is dequeued to run, if it is of a type that can be combined with similar work items in a batch, this facility +// gives the client of this API an opportunity to coalesce and merge work items. +// - The "batching ID" is used for grouping mergeable work items with unique merging strategies. The ID value is opaque to this +// API, and the API client is responsible for assigning them. +// - Each work item will only be asked to batch before it's first dequeued to run readyHandler. +// See the MTRAsyncCallbackBatchingHandler definition for more details. + +// The batching handler is called by the work queue when all of the following are true: +// +// 1) A work item that is batchable is about to be dequeued and executed for the first time. +// 2) The next work item in the queue is also batchable. +// 3) The two work items have matching batching ids. +// +// The handler will be passed the opaque data of the two work items: opaqueDataCurrent is the data of the +// item about to be executed and opaqueDataNext is the data for the next item. +// +// The handler is expected to mutate the data as needed to achieve batching. +// +// If after the data mutations opaqueDataNext no longer requires any work, the handler should set *fullyMerged to YES to indicate +// that the next item can be dropped from the queue. Otherwise the handler should set *fullyMerged to NO. +// +// If *fullyMerged is set to YES, this handler may be called again to possibly also batch the work item +// after the one that was dropped. +typedef void (^MTRAsyncCallbackBatchingHandler)(id opaqueDataCurrent, id opaqueDataNext, BOOL * fullyMerged); + +// Optional feature: Duplicate Filtering +// This is a facility that enables the API client to check if a potential work item has already been enqueued. By providing a +// handler that can answer if a work item's relevant data is a duplicate, it can avoid redundant queuing of requests. +// - The "duplicate type ID" is used for grouping different types of work items for duplicate checking. The ID value is opaque +// to this API, and the API client is responsible for assigning them. +// See the MTRAsyncCallbackDuplicateCheckHandler definition and the WorkQueue's -isDuplicateForTypeID:workItemData: method +// descriptions for more details. + +// The duplicate check handler is called by the work queue when the client wishes to check whether a work item is a duplicate of an +// existing one, so that the client may decide to not enqueue a duplicate work item. +// +// The handler will be passed the opaque data of a potential duplicate work item. +// +// If the handler determines the data is indeed duplicate work, it should set *stop to YES, and set *isDuplicate to YES. +// +// If the handler determines the data is not duplicate work, it should set *stop to YES, and set *isDuplicate to NO. +// +// If the handler is unable to determine if the data is duplicate work, it should set *stop to NO. +// In this case, the value of *isDuplicate is not examined. +typedef void (^MTRAsyncCallbackDuplicateCheckHandler)(id opaqueItemData, BOOL * isDuplicate, BOOL * stop); + @interface MTRAsyncCallbackWorkQueue () // The MTRDevice object is only held and passed back as a reference and is opaque to the queue - (instancetype)initWithContext:(id _Nullable)context queue:(dispatch_queue_t)queue; + +// Before creating a work item, a client may call this method to check with existing work items that the new potential work item +// data is not a duplicate request. +// - This method will call the duplicate check handler for all work items matching the duplicate type ID, starting from the last +// item in the queue, and if a handler sets *stop to YES, this method will return the value the handler sets for *isDuplicate +// - If no duplicate check handlers set *stop to YES, this method will return NO. +- (BOOL)isDuplicateForTypeID:(NSUInteger)opaqueDuplicateTypeID workItemData:(id)opaqueWorkItemData; +@end + +@interface MTRAsyncCallbackQueueWorkItem () +// Batching +@property (nonatomic, readonly) BOOL batchable; +@property (nonatomic, readonly) NSUInteger batchingID; +@property (nonatomic, readonly) id batchableData; +@property (nonatomic, readonly) MTRAsyncCallbackBatchingHandler batchingHandler; +- (void)setBatchingID:(NSUInteger)opaqueBatchingID + data:(id)opaqueBatchableData + handler:(MTRAsyncCallbackBatchingHandler)batchingHandler; + +// Duplicate check +@property (nonatomic, readonly) BOOL supportsDuplicateCheck; +@property (nonatomic, readonly) NSUInteger duplicateTypeID; +@property (nonatomic, readonly) MTRAsyncCallbackDuplicateCheckHandler duplicateCheckHandler; +- (void)setDuplicateTypeID:(NSUInteger)opaqueDuplicateTypeID handler:(MTRAsyncCallbackDuplicateCheckHandler)duplicateCheckHandler; @end NS_ASSUME_NONNULL_END diff --git a/src/darwin/Framework/CHIP/MTRDevice.mm b/src/darwin/Framework/CHIP/MTRDevice.mm index 821114fcb178e8..164f395b146fdf 100644 --- a/src/darwin/Framework/CHIP/MTRDevice.mm +++ b/src/darwin/Framework/CHIP/MTRDevice.mm @@ -17,7 +17,7 @@ #import -#import "MTRAsyncCallbackWorkQueue.h" +#import "MTRAsyncCallbackWorkQueue_Internal.h" #import "MTRAttributeSpecifiedCheck.h" #import "MTRBaseDevice_Internal.h" #import "MTRBaseSubscriptionCallback.h" @@ -117,6 +117,19 @@ typedef NS_ENUM(NSUInteger, MTRDeviceExpectedValueFieldIndex) { MTRDeviceExpectedValueFieldIDIndex = 2 }; +typedef NS_ENUM(NSUInteger, MTRDeviceReadRequestFieldIndex) { + MTRDeviceReadRequestFieldPathIndex = 0, + MTRDeviceReadRequestFieldParamsIndex = 1 +}; + +typedef NS_ENUM(NSUInteger, MTRDeviceWorkItemBatchingID) { + MTRDeviceWorkItemBatchingReadID = 1, +}; + +typedef NS_ENUM(NSUInteger, MTRDeviceWorkItemDuplicateTypeID) { + MTRDeviceWorkItemDuplicateReadTypeID = 1, +}; + @interface MTRDevice () @property (nonatomic, readonly) os_unfair_lock lock; // protects the caches and device state @property (nonatomic) chip::FabricIndex fabricIndex; @@ -775,37 +788,126 @@ static BOOL AttributeHasChangesOmittedQuality(MTRAttributePath * attributePath) // 4. Cache has no entry // TODO: add option for BaseSubscriptionCallback to report during priming, to reduce when case 4 is hit if (!attributeIsSpecified || ![self _subscriptionAbleToReport] || hasChangesOmittedQuality || !attributeValueToReturn) { + // Read requests container will be a mutable array of items, each being an array containing: + // [attribute request path, params] + // Batching handler should only coalesce when params are equal. + + // For this single read API there's only 1 array item. Use NSNull to stand in for nil params for easy comparison. + MTRAttributeRequestPath * readRequestPath = [MTRAttributeRequestPath requestPathWithEndpointID:endpointID + clusterID:clusterID + attributeID:attributeID]; + NSArray * readRequestData = @[ readRequestPath, params ?: [NSNull null] ]; + + // But first, check if a duplicate read request is already queued and return + if ([_asyncCallbackWorkQueue isDuplicateForTypeID:MTRDeviceWorkItemDuplicateReadTypeID workItemData:readRequestData]) { + return attributeValueToReturn; + } + + NSMutableArray * readRequests = [NSMutableArray arrayWithObject:readRequestData]; + // Create work item, set ready handler to perform task, then enqueue the work MTRAsyncCallbackQueueWorkItem * workItem = [[MTRAsyncCallbackQueueWorkItem alloc] initWithQueue:self.queue]; + MTRAsyncCallbackBatchingHandler batchingHandler = ^(id opaqueDataCurrent, id opaqueDataNext, BOOL * fullyMerged) { + NSMutableArray * readRequestsCurrent = opaqueDataCurrent; + NSMutableArray * readRequestsNext = opaqueDataNext; + + *fullyMerged = NO; + + // Can only read up to 9 paths at a time, per spec + if (readRequestsCurrent.count >= 9) { + MTR_LOG_DEFAULT("%@ batching cannot add more", logPrefix); + return; + } + + while (readRequestsNext.count) { + // if params don't match then they cannot be merged + if (![readRequestsNext[0][MTRDeviceReadRequestFieldParamsIndex] + isEqual:readRequestsCurrent[0][MTRDeviceReadRequestFieldParamsIndex]]) { + MTR_LOG_DEFAULT("%@ batching merged all possible items", logPrefix); + return; + } + + // merge the next item's first request into the current item's list + [readRequestsCurrent addObject:readRequestsNext[0]]; + MTR_LOG_INFO("%@ batching merging %@ => %lu total", logPrefix, readRequestsNext[0], + (unsigned long) readRequestsCurrent.count); + [readRequestsNext removeObjectAtIndex:0]; + + // Can only read up to 9 paths at a time, per spec + if (readRequestsCurrent.count == 9) { + MTR_LOG_DEFAULT("%@ batching to max paths allowed", logPrefix); + break; + } + } + + if (readRequestsNext.count == 0) { + MTR_LOG_DEFAULT("%@ batching - fully merged next item", logPrefix); + *fullyMerged = YES; + } + }; + MTRAsyncCallbackDuplicateCheckHandler duplicateCheckHandler = ^(id opaqueItemData, BOOL * isDuplicate, BOOL * stop) { + for (NSArray * readItem in readRequests) { + if ([readItem isEqual:opaqueItemData]) { + MTR_LOG_DEFAULT("%@ duplicate check found %@ - report duplicate", logPrefix, readItem); + *isDuplicate = YES; + *stop = YES; + return; + } + } + *stop = NO; + }; MTRAsyncCallbackReadyHandler readyHandler = ^(MTRDevice * device, NSUInteger retryCount) { MTR_LOG_DEFAULT("%@ dequeueWorkItem %@", logPrefix, self->_asyncCallbackWorkQueue); + + // Sanity check + if (readRequests.count == 0) { + MTR_LOG_ERROR("%@ dequeueWorkItem no read requests", logPrefix); + [workItem endWork]; + return; + } + + // Build the attribute paths from the read requests + NSMutableArray * attributePaths = [NSMutableArray array]; + for (NSArray * readItem in readRequests) { + // Sanity check + if (readItem.count < 2) { + MTR_LOG_ERROR("%@ dequeueWorkItem read item missing info %@", logPrefix, readItem); + [workItem endWork]; + return; + } + [attributePaths addObject:readItem[MTRDeviceReadRequestFieldPathIndex]]; + } + // If param is the NSNull stand-in, then just use nil + id readParamObject = readRequests[0][MTRDeviceReadRequestFieldParamsIndex]; + MTRReadParams * readParams = (![readParamObject isEqual:[NSNull null]]) ? readParamObject : nil; + MTRBaseDevice * baseDevice = [self newBaseDevice]; - [baseDevice readAttributesWithEndpointID:endpointID - clusterID:clusterID - attributeID:attributeID - params:params - queue:self.queue - completion:^(NSArray *> * _Nullable values, - NSError * _Nullable error) { - if (values) { - // Since the format is the same data-value dictionary, this looks like an - // attribute report - MTR_LOG_INFO("%@ completion values %@", logPrefix, values); - [self _handleAttributeReport:values]; - } - - // TODO: better retry logic - if (error && (retryCount < 2)) { - MTR_LOG_ERROR("%@ completion error %@ retryWork %lu", logPrefix, error, - (unsigned long) retryCount); - [workItem retryWork]; - } else { - MTR_LOG_DEFAULT("%@ completion error %@ endWork", logPrefix, error); - [workItem endWork]; - } - }]; + [baseDevice + readAttributePaths:attributePaths + eventPaths:nil + params:readParams + queue:self.queue + completion:^(NSArray *> * _Nullable values, NSError * _Nullable error) { + if (values) { + // Since the format is the same data-value dictionary, this looks like an + // attribute report + MTR_LOG_INFO("%@ completion values %@", logPrefix, values); + [self _handleAttributeReport:values]; + } + + // TODO: better retry logic + if (error && (retryCount < 2)) { + MTR_LOG_ERROR("%@ completion error %@ retryWork %lu", logPrefix, error, (unsigned long) retryCount); + [workItem retryWork]; + } else { + MTR_LOG_DEFAULT("%@ completion error %@ endWork", logPrefix, error); + [workItem endWork]; + } + }]; }; workItem.readyHandler = readyHandler; + [workItem setBatchingID:MTRDeviceWorkItemBatchingReadID data:readRequests handler:batchingHandler]; + [workItem setDuplicateTypeID:MTRDeviceWorkItemDuplicateReadTypeID handler:duplicateCheckHandler]; MTR_LOG_DEFAULT("%@ enqueueWorkItem %@", logPrefix, _asyncCallbackWorkQueue); [_asyncCallbackWorkQueue enqueueWorkItem:workItem]; } @@ -836,6 +938,12 @@ - (void)writeAttributeWithEndpointID:(NSNumber *)endpointID expectedValueID:&expectedValueID]; MTRAsyncCallbackQueueWorkItem * workItem = [[MTRAsyncCallbackQueueWorkItem alloc] initWithQueue:self.queue]; + // The write operation will install a duplicate check handler, to return NO for "isDuplicate". Since a write operation may + // change values, only read requests after this should be considered for duplicate requests. + MTRAsyncCallbackDuplicateCheckHandler duplicateCheckHandler = ^(id opaqueItemData, BOOL * isDuplicate, BOOL * stop) { + *isDuplicate = NO; + *stop = YES; + }; MTRAsyncCallbackReadyHandler readyHandler = ^(MTRDevice * device, NSUInteger retryCount) { MTR_LOG_DEFAULT("%@ dequeueWorkItem %@", logPrefix, self->_asyncCallbackWorkQueue); MTRBaseDevice * baseDevice = [self newBaseDevice]; @@ -855,6 +963,7 @@ - (void)writeAttributeWithEndpointID:(NSNumber *)endpointID }]; }; workItem.readyHandler = readyHandler; + [workItem setDuplicateTypeID:MTRDeviceWorkItemDuplicateReadTypeID handler:duplicateCheckHandler]; MTR_LOG_DEFAULT("%@ enqueueWorkItem %@", logPrefix, _asyncCallbackWorkQueue); [_asyncCallbackWorkQueue enqueueWorkItem:workItem]; } @@ -889,6 +998,12 @@ - (void)invokeCommandWithEndpointID:(NSNumber *)endpointID } } MTRAsyncCallbackQueueWorkItem * workItem = [[MTRAsyncCallbackQueueWorkItem alloc] initWithQueue:self.queue]; + // The command operation will install a duplicate check handler, to return NO for "isDuplicate". Since a command operation may + // change values, only read requests after this should be considered for duplicate requests. + MTRAsyncCallbackDuplicateCheckHandler duplicateCheckHandler = ^(id opaqueItemData, BOOL * isDuplicate, BOOL * stop) { + *isDuplicate = NO; + *stop = YES; + }; MTRAsyncCallbackReadyHandler readyHandler = ^(MTRDevice * device, NSUInteger retryCount) { MTR_LOG_DEFAULT("%@ dequeueWorkItem %@", logPrefix, self->_asyncCallbackWorkQueue); MTRBaseDevice * baseDevice = [self newBaseDevice]; @@ -914,6 +1029,7 @@ - (void)invokeCommandWithEndpointID:(NSNumber *)endpointID }]; }; workItem.readyHandler = readyHandler; + [workItem setDuplicateTypeID:MTRDeviceWorkItemDuplicateReadTypeID handler:duplicateCheckHandler]; MTR_LOG_DEFAULT("%@ enqueueWorkItem %@", logPrefix, _asyncCallbackWorkQueue); [_asyncCallbackWorkQueue enqueueWorkItem:workItem]; } diff --git a/src/darwin/Framework/CHIPTests/MTRAsyncCallbackQueueTests.m b/src/darwin/Framework/CHIPTests/MTRAsyncCallbackQueueTests.m index 6612d8f2e3cebc..01651afa5a1edb 100644 --- a/src/darwin/Framework/CHIPTests/MTRAsyncCallbackQueueTests.m +++ b/src/darwin/Framework/CHIPTests/MTRAsyncCallbackQueueTests.m @@ -19,7 +19,7 @@ // system dependencies #import -#import "MTRAsyncCallbackWorkQueue.h" +#import "MTRAsyncCallbackWorkQueue_Internal.h" @interface MTRAsyncCallbackQueueTests : XCTestCase @@ -256,4 +256,219 @@ - (void)testInvalidation [self waitForExpectations:@[ expectation, cancelExpectation ] timeout:5]; } +- (void)testBatching +{ + XCTestExpectation * workItem1ReadyExpectation = [self expectationWithDescription:@"Work item 1 called"]; + __block BOOL workItem2BatchingCalled = NO; + __block BOOL workItem2ReadyCalled = NO; + XCTestExpectation * workItem3ReadyExpectation = [self expectationWithDescription:@"Work item 3 called"]; + + MTRAsyncCallbackWorkQueue * workQueue = [[MTRAsyncCallbackWorkQueue alloc] initWithContext:nil queue:dispatch_get_main_queue()]; + + // Have a work item sleep so the testing items can queue + MTRAsyncCallbackQueueWorkItem * workItem0 = + [[MTRAsyncCallbackQueueWorkItem alloc] initWithQueue:dispatch_get_global_queue(QOS_CLASS_DEFAULT, 0)]; + MTRAsyncCallbackReadyHandler readyHandler0 = ^(MTRDevice * _Nonnull device, NSUInteger retryCount) { + // While processing item 0, enqueue additional items to test batching + MTRAsyncCallbackQueueWorkItem * workItem1 = + [[MTRAsyncCallbackQueueWorkItem alloc] initWithQueue:dispatch_get_global_queue(QOS_CLASS_DEFAULT, 0)]; + MTRAsyncCallbackReadyHandler readyHandler1 = ^(MTRDevice * _Nonnull device, NSUInteger retryCount) { + [workItem1ReadyExpectation fulfill]; + [workItem1 endWork]; + }; + workItem1.readyHandler = readyHandler1; + [workItem1 setBatchingID:1 + data:@(1) + handler:^(id _Nonnull opaqueDataFirst, id _Nonnull opaqueDataSecond, BOOL * _Nonnull fullyMerged) { + XCTAssertEqualObjects(opaqueDataFirst, @(1)); + XCTAssertEqualObjects(opaqueDataSecond, @(2)); + *fullyMerged = YES; + }]; + // No cancel handler on purpose. + [workQueue enqueueWorkItem:workItem1]; + + MTRAsyncCallbackQueueWorkItem * workItem2 = + [[MTRAsyncCallbackQueueWorkItem alloc] initWithQueue:dispatch_get_global_queue(QOS_CLASS_DEFAULT, 0)]; + MTRAsyncCallbackReadyHandler readyHandler2 = ^(MTRDevice * _Nonnull device, NSUInteger retryCount) { + workItem2ReadyCalled = YES; + [workItem2 endWork]; + }; + workItem2.readyHandler = readyHandler2; + [workItem2 setBatchingID:1 + data:@(2) + handler:^(id _Nonnull opaqueDataFirst, id _Nonnull opaqueDataSecond, BOOL * _Nonnull fullyMerged) { + workItem2BatchingCalled = YES; + }]; + // No cancel handler on purpose. + [workQueue enqueueWorkItem:workItem2]; + + MTRAsyncCallbackQueueWorkItem * workItem3 = + [[MTRAsyncCallbackQueueWorkItem alloc] initWithQueue:dispatch_get_global_queue(QOS_CLASS_DEFAULT, 0)]; + MTRAsyncCallbackReadyHandler readyHandler3 = ^(MTRDevice * _Nonnull device, NSUInteger retryCount) { + [workItem3ReadyExpectation fulfill]; + [workItem3 endWork]; + }; + workItem3.readyHandler = readyHandler3; + [workQueue enqueueWorkItem:workItem3]; + + [workItem0 endWork]; + }; + workItem0.readyHandler = readyHandler0; + // No cancel handler on purpose. + [workQueue enqueueWorkItem:workItem0]; + + [self waitForExpectations:@[ workItem1ReadyExpectation, workItem3ReadyExpectation ] timeout:5]; + + XCTAssertFalse(workItem2BatchingCalled); + XCTAssertFalse(workItem2ReadyCalled); +} + +- (void)testDuplicate +{ + XCTestExpectation * workItem0ReadyExpectation = [self expectationWithDescription:@"Work item 0 called"]; + XCTestExpectation * workItem6ReadyExpectation = [self expectationWithDescription:@"Work item 6 called"]; + + MTRAsyncCallbackWorkQueue * workQueue = [[MTRAsyncCallbackWorkQueue alloc] initWithContext:nil queue:dispatch_get_main_queue()]; + + // Have a work item sleep so the testing items can queue + MTRAsyncCallbackQueueWorkItem * workItem0 = + [[MTRAsyncCallbackQueueWorkItem alloc] initWithQueue:dispatch_get_global_queue(QOS_CLASS_DEFAULT, 0)]; + MTRAsyncCallbackReadyHandler readyHandler0 = ^(MTRDevice * _Nonnull device, NSUInteger retryCount) { + // While processing item 0, enqueue additional items to test duplicate checking + MTRAsyncCallbackQueueWorkItem * workItem1 = + [[MTRAsyncCallbackQueueWorkItem alloc] initWithQueue:dispatch_get_global_queue(QOS_CLASS_DEFAULT, 0)]; + MTRAsyncCallbackReadyHandler readyHandler1 = ^(MTRDevice * _Nonnull device, NSUInteger retryCount) { + [workItem1 endWork]; + }; + workItem1.readyHandler = readyHandler1; + [workItem1 setDuplicateTypeID:1 + handler:^(id _Nonnull opaqueItemData, BOOL * _Nonnull isDuplicate, BOOL * stop) { + if ([opaqueItemData isEqual:@(1)]) { + *isDuplicate = YES; + *stop = YES; + } else { + *stop = NO; + } + }]; + [workQueue enqueueWorkItem:workItem1]; + + MTRAsyncCallbackQueueWorkItem * workItem2 = + [[MTRAsyncCallbackQueueWorkItem alloc] initWithQueue:dispatch_get_global_queue(QOS_CLASS_DEFAULT, 0)]; + MTRAsyncCallbackReadyHandler readyHandler2 = ^(MTRDevice * _Nonnull device, NSUInteger retryCount) { + [workItem2 endWork]; + }; + workItem2.readyHandler = readyHandler2; + [workItem2 setDuplicateTypeID:1 + handler:^(id _Nonnull opaqueItemData, BOOL * _Nonnull isDuplicate, BOOL * stop) { + if ([opaqueItemData isEqual:@(2)]) { + *isDuplicate = YES; + *stop = YES; + } else { + *stop = NO; + } + }]; + [workQueue enqueueWorkItem:workItem2]; + + MTRAsyncCallbackQueueWorkItem * workItem3 = + [[MTRAsyncCallbackQueueWorkItem alloc] initWithQueue:dispatch_get_global_queue(QOS_CLASS_DEFAULT, 0)]; + MTRAsyncCallbackReadyHandler readyHandler3 = ^(MTRDevice * _Nonnull device, NSUInteger retryCount) { + [workItem3 endWork]; + }; + workItem3.readyHandler = readyHandler3; + [workItem3 setDuplicateTypeID:2 + handler:^(id _Nonnull opaqueItemData, BOOL * _Nonnull isDuplicate, BOOL * stop) { + if ([opaqueItemData isEqual:@(1)]) { + *isDuplicate = YES; + *stop = YES; + } else { + *stop = NO; + } + }]; + [workQueue enqueueWorkItem:workItem3]; + + // At this point we should have duplicate type 1 with data @(1) and @(2), and type 2 with data @(1). + XCTAssertTrue([workQueue isDuplicateForTypeID:1 workItemData:@(1)]); + XCTAssertTrue([workQueue isDuplicateForTypeID:1 workItemData:@(2)]); + XCTAssertTrue([workQueue isDuplicateForTypeID:2 workItemData:@(1)]); + + XCTAssertFalse([workQueue isDuplicateForTypeID:0 workItemData:@(1)]); + XCTAssertFalse([workQueue isDuplicateForTypeID:0 workItemData:@(2)]); + XCTAssertFalse([workQueue isDuplicateForTypeID:1 workItemData:@(0)]); + XCTAssertFalse([workQueue isDuplicateForTypeID:1 workItemData:@(3)]); + XCTAssertFalse([workQueue isDuplicateForTypeID:2 workItemData:@(2)]); + + // Test returning *isDuplicate=NO and queuing one extra duplicate item, and that the extra item runs + + // First have a regular item with ID/data == 3/1 + MTRAsyncCallbackQueueWorkItem * workItem4 = + [[MTRAsyncCallbackQueueWorkItem alloc] initWithQueue:dispatch_get_global_queue(QOS_CLASS_DEFAULT, 0)]; + MTRAsyncCallbackReadyHandler readyHandler4 = ^(MTRDevice * _Nonnull device, NSUInteger retryCount) { + [workItem4 endWork]; + }; + workItem4.readyHandler = readyHandler4; + [workItem4 setDuplicateTypeID:3 + handler:^(id _Nonnull opaqueItemData, BOOL * _Nonnull isDuplicate, BOOL * stop) { + if ([opaqueItemData isEqual:@(1)]) { + *isDuplicate = YES; + *stop = YES; + } else { + *stop = NO; + } + }]; + [workQueue enqueueWorkItem:workItem4]; + + XCTAssertTrue([workQueue isDuplicateForTypeID:3 workItemData:@(1)]); + + // Have a barrier item with ID/data == 3/1 that returns *isDuplicate=NO + MTRAsyncCallbackQueueWorkItem * workItem5 = + [[MTRAsyncCallbackQueueWorkItem alloc] initWithQueue:dispatch_get_global_queue(QOS_CLASS_DEFAULT, 0)]; + MTRAsyncCallbackReadyHandler readyHandler5 = ^(MTRDevice * _Nonnull device, NSUInteger retryCount) { + [workItem5 endWork]; + }; + workItem5.readyHandler = readyHandler5; + [workItem5 setDuplicateTypeID:3 + handler:^(id _Nonnull opaqueItemData, BOOL * _Nonnull isDuplicate, BOOL * stop) { + if ([opaqueItemData isEqual:@(1)]) { + *isDuplicate = NO; + *stop = YES; + } else { + *stop = NO; + } + }]; + [workQueue enqueueWorkItem:workItem5]; + + // After the above, the same ID/data should no longer be considered duplicate + XCTAssertFalse([workQueue isDuplicateForTypeID:3 workItemData:@(1)]); + + // Now add regular regular item with ID/data == 3/1 + MTRAsyncCallbackQueueWorkItem * workItem6 = + [[MTRAsyncCallbackQueueWorkItem alloc] initWithQueue:dispatch_get_global_queue(QOS_CLASS_DEFAULT, 0)]; + MTRAsyncCallbackReadyHandler readyHandler6 = ^(MTRDevice * _Nonnull device, NSUInteger retryCount) { + [workItem6 endWork]; + [workItem6ReadyExpectation fulfill]; + }; + workItem6.readyHandler = readyHandler6; + [workItem6 setDuplicateTypeID:3 + handler:^(id _Nonnull opaqueItemData, BOOL * _Nonnull isDuplicate, BOOL * stop) { + if ([opaqueItemData isEqual:@(1)]) { + *isDuplicate = YES; + *stop = YES; + } else { + *stop = NO; + } + }]; + [workQueue enqueueWorkItem:workItem6]; + + // After the above, the same ID/data should no longer be considered duplicate + XCTAssertTrue([workQueue isDuplicateForTypeID:3 workItemData:@(1)]); + + [workItem0 endWork]; + [workItem0ReadyExpectation fulfill]; + }; + workItem0.readyHandler = readyHandler0; + [workQueue enqueueWorkItem:workItem0]; + + [self waitForExpectations:@[ workItem0ReadyExpectation, workItem6ReadyExpectation ] timeout:5]; +} + @end diff --git a/src/darwin/Framework/CHIPTests/MTRDeviceTests.m b/src/darwin/Framework/CHIPTests/MTRDeviceTests.m index 1c2c1520f586d5..07999e06cafeef 100644 --- a/src/darwin/Framework/CHIPTests/MTRDeviceTests.m +++ b/src/darwin/Framework/CHIPTests/MTRDeviceTests.m @@ -1454,6 +1454,32 @@ - (void)test017_TestMTRDeviceBasics [device setDelegate:delegate queue:queue]; + // Test batching and duplicate check + // - Read 13 different attributes in a row, expect that the 1st to go out by itself, the next 9 batch, and then the 3 after + // are correctly queued in one batch + // - Then read 3 duplicates and expect them to be filtered + // - Note that these tests can only be verified via logs + [device readAttributeWithEndpointID:@(1) clusterID:@(MTRClusterIDTypeScenesID) attributeID:@(0) params:nil]; + + [device readAttributeWithEndpointID:@(1) clusterID:@(MTRClusterIDTypeScenesID) attributeID:@(1) params:nil]; + [device readAttributeWithEndpointID:@(1) clusterID:@(MTRClusterIDTypeScenesID) attributeID:@(2) params:nil]; + [device readAttributeWithEndpointID:@(1) clusterID:@(MTRClusterIDTypeScenesID) attributeID:@(3) params:nil]; + [device readAttributeWithEndpointID:@(1) clusterID:@(MTRClusterIDTypeScenesID) attributeID:@(4) params:nil]; + [device readAttributeWithEndpointID:@(1) clusterID:@(MTRClusterIDTypeScenesID) attributeID:@(5) params:nil]; + + [device readAttributeWithEndpointID:@(1) clusterID:@(MTRClusterIDTypeScenesID) attributeID:@(6) params:nil]; + [device readAttributeWithEndpointID:@(1) clusterID:@(MTRClusterIDTypeScenesID) attributeID:@(7) params:nil]; + [device readAttributeWithEndpointID:@(1) clusterID:@(MTRClusterIDTypeLevelControlID) attributeID:@(0) params:nil]; + [device readAttributeWithEndpointID:@(1) clusterID:@(MTRClusterIDTypeLevelControlID) attributeID:@(1) params:nil]; + + [device readAttributeWithEndpointID:@(1) clusterID:@(MTRClusterIDTypeLevelControlID) attributeID:@(2) params:nil]; + [device readAttributeWithEndpointID:@(1) clusterID:@(MTRClusterIDTypeLevelControlID) attributeID:@(3) params:nil]; + [device readAttributeWithEndpointID:@(1) clusterID:@(MTRClusterIDTypeLevelControlID) attributeID:@(4) params:nil]; + + [device readAttributeWithEndpointID:@(1) clusterID:@(MTRClusterIDTypeLevelControlID) attributeID:@(4) params:nil]; + [device readAttributeWithEndpointID:@(1) clusterID:@(MTRClusterIDTypeLevelControlID) attributeID:@(4) params:nil]; + [device readAttributeWithEndpointID:@(1) clusterID:@(MTRClusterIDTypeLevelControlID) attributeID:@(4) params:nil]; + [self waitForExpectations:@[ subscriptionExpectation ] timeout:60]; XCTAssertNotEqual(attributeReportsReceived, 0);