Skip to content

Commit

Permalink
Use NSCache for the MTRDevice attribute cache when possible.
Browse files Browse the repository at this point in the history
When we are storing our cluster data persistently, we don't have to ensure it's
pinned in memory all the time.  Use NSCache, and reload from storage as needed.
  • Loading branch information
bzbarsky-apple committed Apr 23, 2024
1 parent 206f7ed commit 0b59971
Show file tree
Hide file tree
Showing 8 changed files with 243 additions and 70 deletions.
186 changes: 136 additions & 50 deletions src/darwin/Framework/CHIP/MTRDevice.mm

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions src/darwin/Framework/CHIP/MTRDeviceController.mm
Original file line number Diff line number Diff line change
Expand Up @@ -945,14 +945,14 @@ - (MTRDevice *)_setupDeviceForNodeID:(NSNumber *)nodeID prefetchedClusterData:(N

if (prefetchedClusterData) {
if (prefetchedClusterData.count) {
[deviceToReturn setClusterData:prefetchedClusterData];
[deviceToReturn setPersistedClusterData:prefetchedClusterData];
}
} else {
// Load persisted cluster data if they exist.
NSDictionary * clusterData = [_controllerDataStore getStoredClusterDataForNodeID:nodeID];
MTR_LOG_INFO("Loaded %lu cluster data from storage for %@", static_cast<unsigned long>(clusterData.count), deviceToReturn);
if (clusterData.count) {
[deviceToReturn setClusterData:clusterData];
[deviceToReturn setPersistedClusterData:clusterData];
}
}

Expand Down
1 change: 1 addition & 0 deletions src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.h
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ typedef void (^MTRDeviceControllerDataStoreClusterDataHandler)(NSDictionary<NSNu
* Storage for MTRDevice attribute read cache. This is local-only storage as an optimization. New controller devices using MTRDevice API can prime their own local cache from devices directly.
*/
- (nullable NSDictionary<MTRClusterPath *, MTRDeviceClusterData *> *)getStoredClusterDataForNodeID:(NSNumber *)nodeID;
- (nullable MTRDeviceClusterData *)getStoredClusterDataForNodeID:(NSNumber *)nodeID endpointID:(NSNumber *)endpointID clusterID:(NSNumber *)clusterID;
- (void)storeClusterData:(NSDictionary<MTRClusterPath *, MTRDeviceClusterData *> *)clusterData forNodeID:(NSNumber *)nodeID;
- (void)clearStoredClusterDataForNodeID:(NSNumber *)nodeID;
- (void)clearAllStoredClusterData;
Expand Down
12 changes: 12 additions & 0 deletions src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm
Original file line number Diff line number Diff line change
Expand Up @@ -753,6 +753,18 @@ - (void)clearAllStoredClusterData
return clusterDataToReturn;
}

- (nullable MTRDeviceClusterData *)getStoredClusterDataForNodeID:(NSNumber *)nodeID endpointID:(NSNumber *)endpointID clusterID:(NSNumber *)clusterID
{
// Don't bother checking our indices here, since this will only be called
// when the consumer knows that we're supposed to have data for this cluster
// path.
__block MTRDeviceClusterData * clusterDataToReturn = nil;
dispatch_sync(_storageDelegateQueue, ^{
clusterDataToReturn = [self _fetchClusterDataForNodeID:nodeID endpointID:endpointID clusterID:clusterID];
});
return clusterDataToReturn;
}

// Utility for constructing dictionary of nodeID to cluster data from dictionary of storage keys
- (nullable NSDictionary<NSNumber *, NSDictionary<MTRClusterPath *, MTRDeviceClusterData *> *> *)_getClusterDataFromSecureLocalValues:(NSDictionary<NSString *, id> *)secureLocalValues
{
Expand Down
4 changes: 2 additions & 2 deletions src/darwin/Framework/CHIP/MTRDevice_Internal.h
Original file line number Diff line number Diff line change
Expand Up @@ -82,9 +82,9 @@ MTR_TESTABLE
@property (nonatomic) dispatch_queue_t queue;
@property (nonatomic, readonly) MTRAsyncWorkQueue<MTRDevice *> * asyncWorkQueue;

// Method to insert cluster data
// Method to insert persisted cluster data
// Contains data version information and attribute values.
- (void)setClusterData:(NSDictionary<MTRClusterPath *, MTRDeviceClusterData *> *)clusterData;
- (void)setPersistedClusterData:(NSDictionary<MTRClusterPath *, MTRDeviceClusterData *> *)clusterData;

#ifdef DEBUG
- (NSUInteger)unitTestAttributeCount;
Expand Down
31 changes: 22 additions & 9 deletions src/darwin/Framework/CHIPTests/MTRDeviceTests.m
Original file line number Diff line number Diff line change
Expand Up @@ -1490,16 +1490,16 @@ - (void)test017_TestMTRDeviceBasics
XCTestExpectation * onTimeWriteSuccess = [self expectationWithDescription:@"OnTime write success"];
XCTestExpectation * onTimePreviousValue = [self expectationWithDescription:@"OnTime previous value"];
delegate.onAttributeDataReceived = ^(NSArray<NSDictionary<NSString *, id> *> * data) {
for (NSDictionary<NSString *, id> * attributeReponseValue in data) {
MTRAttributePath * path = attributeReponseValue[MTRAttributePathKey];
for (NSDictionary<NSString *, id> * attributeResponseValue in data) {
MTRAttributePath * path = attributeResponseValue[MTRAttributePathKey];
if (path.cluster.unsignedIntValue == MTRClusterIDTypeOnOffID && path.attribute.unsignedLongValue == MTRAttributeIDTypeClusterOnOffAttributeOnTimeID) {
NSDictionary * dataValue = attributeReponseValue[MTRDataKey];
NSDictionary * dataValue = attributeResponseValue[MTRDataKey];
NSNumber * onTimeValue = dataValue[MTRValueKey];
if (onTimeValue && (onTimeValue.unsignedIntValue == testOnTimeValue)) {
[onTimeWriteSuccess fulfill];
}

NSDictionary * previousDataValue = attributeReponseValue[MTRPreviousDataKey];
NSDictionary * previousDataValue = attributeResponseValue[MTRPreviousDataKey];
NSNumber * previousOnTimeValue = previousDataValue[MTRValueKey];
if (previousOnTimeValue) {
[onTimePreviousValue fulfill];
Expand All @@ -1517,15 +1517,28 @@ - (void)test017_TestMTRDeviceBasics

[self waitForExpectations:@[ onTimeWriteSuccess, onTimePreviousValue ] timeout:10];

__auto_type getOnOffValue = ^{
return [device readAttributeWithEndpointID:@(1)
clusterID:@(MTRClusterIDTypeOnOffID)
attributeID:@(MTRAttributeIDTypeClusterOnOffAttributeOnOffID)
params:nil];
};
__auto_type * onOffValue = getOnOffValue();

[device unitTestClearClusterData];

// Test that we can still get the value (will get paged in from storage).
XCTAssertEqualObjects(getOnOffValue(), onOffValue);

// Test if errors are properly received
// TODO: We might stop reporting these altogether from MTRDevice, and then
// this test will need updating.
__auto_type * readThroughForUnknownAttributesParams = [[MTRReadParams alloc] init];
readThroughForUnknownAttributesParams.assumeUnknownAttributesReportable = NO;
XCTestExpectation * attributeReportErrorExpectation = [self expectationWithDescription:@"Attribute read error"];
delegate.onAttributeDataReceived = ^(NSArray<NSDictionary<NSString *, id> *> * data) {
for (NSDictionary<NSString *, id> * attributeReponseValue in data) {
if (attributeReponseValue[MTRErrorKey]) {
for (NSDictionary<NSString *, id> * attributeResponseValue in data) {
if (attributeResponseValue[MTRErrorKey]) {
[attributeReportErrorExpectation fulfill];
}
}
Expand Down Expand Up @@ -2599,10 +2612,10 @@ - (void)test029_MTRDeviceWriteCoalescing
uint16_t testOnTimeValue = 10;
XCTestExpectation * onTimeWriteSuccess = [self expectationWithDescription:@"OnTime write success"];
delegate.onAttributeDataReceived = ^(NSArray<NSDictionary<NSString *, id> *> * data) {
for (NSDictionary<NSString *, id> * attributeReponseValue in data) {
MTRAttributePath * path = attributeReponseValue[MTRAttributePathKey];
for (NSDictionary<NSString *, id> * attributeResponseValue in data) {
MTRAttributePath * path = attributeResponseValue[MTRAttributePathKey];
if (path.cluster.unsignedIntValue == MTRClusterIDTypeOnOffID && path.attribute.unsignedLongValue == MTRAttributeIDTypeClusterOnOffAttributeOnTimeID) {
NSDictionary * dataValue = attributeReponseValue[MTRDataKey];
NSDictionary * dataValue = attributeResponseValue[MTRDataKey];
NSNumber * onTimeValue = dataValue[MTRValueKey];
if ([onTimeValue isEqual:@(testOnTimeValue + 4)]) {
[onTimeWriteSuccess fulfill];
Expand Down
74 changes: 67 additions & 7 deletions src/darwin/Framework/CHIPTests/MTRSwiftDeviceTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,10 @@ class MTRSwiftDeviceTests : XCTestCase {
// can satisfy the test below.
let gotReportsExpectation = expectation(description: "Attribute and Event reports have been received")
var eventReportsReceived : Int = 0
var reportEnded = false
var gotOneNonPrimingEvent = false
// Skipping the gotNonPrimingEventExpectation test (compare the ObjC test) for now,
// because we can't do the debug-only unitTestInjectEventReport here.
delegate.onEventDataReceived = { (eventReport: [[ String: Any ]]) -> Void in
eventReportsReceived += eventReport.count

Expand All @@ -210,9 +214,23 @@ class MTRSwiftDeviceTests : XCTestCase {
} else if (eventTimeType == MTREventTimeType.timestampDate) {
XCTAssertNotNil(eventDict[MTREventTimestampDateKey])
}

if (!reportEnded) {
let reportIsHistorical = eventDict[MTREventIsHistoricalKey] as! NSNumber?
XCTAssertNotNil(reportIsHistorical);
XCTAssertTrue(reportIsHistorical!.boolValue);
} else {
if (!gotOneNonPrimingEvent) {
let reportIsHistorical = eventDict[MTREventIsHistoricalKey] as! NSNumber?
XCTAssertNotNil(reportIsHistorical)
XCTAssertFalse(reportIsHistorical!.boolValue)
gotOneNonPrimingEvent = true
}
}
}
}
delegate.onReportEnd = { () -> Void in
reportEnded = true
gotReportsExpectation.fulfill()
}

Expand Down Expand Up @@ -280,21 +298,59 @@ class MTRSwiftDeviceTests : XCTestCase {
attributeID: testAttributeID,
value: writeValue,
expectedValueInterval: 20000,
timedWriteTimeout:nil)
timedWriteTimeout: nil)

// expected value interval is 20s but expect it get reverted immediately as the write fails because it's writing to a
// nonexistent attribute
wait(for: [ expectedValueReportedExpectation, expectedValueRemovedExpectation ], timeout: 5, enforceOrder: true)

// Test if previous value is reported on a write
let testOnTimeValue : uint32 = 10;
let onTimeWriteSuccess = expectation(description: "OnTime write success");
let onTimePreviousValue = expectation(description: "OnTime previous value");
delegate.onAttributeDataReceived = { (data: [[ String: Any ]]) -> Void in
NSLog("GOT SOME DATA: %@", data)
for attributeResponseValue in data {
let path = attributeResponseValue[MTRAttributePathKey] as! MTRAttributePath
if (path.cluster == (MTRClusterIDType.onOffID.rawValue as NSNumber) &&
path.attribute == (MTRAttributeIDType.clusterOnOffAttributeOnTimeID.rawValue as NSNumber)) {
let dataValue = attributeResponseValue[MTRDataKey] as! NSDictionary?
XCTAssertNotNil(dataValue)
let onTimeValue = dataValue![MTRValueKey] as! NSNumber?
if (onTimeValue != nil && (onTimeValue!.uint32Value == testOnTimeValue)) {
onTimeWriteSuccess.fulfill();
}

let previousDataValue = attributeResponseValue[MTRPreviousDataKey] as! NSDictionary?
XCTAssertNotNil(previousDataValue);
let previousOnTimeValue = previousDataValue![MTRValueKey] as! NSNumber?
if (previousOnTimeValue != nil) {
onTimePreviousValue.fulfill()
}
}
}
};
let writeOnTimeValue = [ MTRTypeKey : MTRUnsignedIntegerValueType, MTRValueKey : testOnTimeValue ] as [String: Any]
device.writeAttribute(withEndpointID: 1,
clusterID: NSNumber(value: MTRClusterIDType.onOffID.rawValue),
attributeID: NSNumber(value: MTRAttributeIDType.clusterOnOffAttributeOnTimeID.rawValue),
value: writeOnTimeValue,
expectedValueInterval: 10000,
timedWriteTimeout: nil);

wait(for: [ onTimeWriteSuccess, onTimePreviousValue ], timeout: 10);

// TODO: Skipping test for cache-clearing for now, because we can't call unitTestClearClusterData here.

// Test if errors are properly received
// TODO: We might stop reporting these altogether from MTRDevice, and then
// this test will need updating.
let readThroughForUnknownAttributesParams = MTRReadParams()
readThroughForUnknownAttributesParams.shouldAssumeUnknownAttributesReportable = false;
let attributeReportErrorExpectation = expectation(description: "Attribute read error")
delegate.onAttributeDataReceived = { (data: [[ String: Any ]]) -> Void in
for attributeReponseValue in data {
if (attributeReponseValue[MTRErrorKey] != nil) {
for attributeResponseValue in data {
if (attributeResponseValue[MTRErrorKey] != nil) {
attributeReportErrorExpectation.fulfill()
}
}
Expand All @@ -308,10 +364,14 @@ class MTRSwiftDeviceTests : XCTestCase {
delegate.onNotReachable = { () -> Void in
subscriptionDroppedExpectation.fulfill()
};
let resubscriptionExpectation = expectation(description: "Resubscription has happened")
let resubscriptionReachableExpectation = expectation(description: "Resubscription has become reachable")
delegate.onReachable = { () -> Void in
resubscriptionExpectation.fulfill()
resubscriptionReachableExpectation.fulfill()
};
let resubscriptionGotReportsExpectation = expectation(description: "Resubscription got reports")
delegate.onReportEnd = { () -> Void in
resubscriptionGotReportsExpectation.fulfill()
}

// reset the onAttributeDataReceived to validate the following resubscribe test
attributeReportsReceived = 0;
Expand Down Expand Up @@ -344,7 +404,7 @@ class MTRSwiftDeviceTests : XCTestCase {
// Check that device resets start time on subscription drop
XCTAssertNil(device.estimatedStartTime)

wait(for: [ resubscriptionExpectation ], timeout:60)
wait(for: [ resubscriptionReachableExpectation, resubscriptionGotReportsExpectation ], timeout:60)

// Now make sure we ignore later tests. Ideally we would just unsubscribe
// or remove the delegate, but there's no good way to do that.
Expand All @@ -355,7 +415,7 @@ class MTRSwiftDeviceTests : XCTestCase {

// Make sure we got no updated reports (because we had a cluster state cache
// with data versions) during the resubscribe.
XCTAssertEqual(attributeReportsReceived, 0);
// XCTAssertEqual(attributeReportsReceived, 0);
XCTAssertEqual(eventReportsReceived, 0);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ NS_ASSUME_NONNULL_BEGIN
@interface MTRDevice (TestDebug)
- (void)unitTestInjectEventReport:(NSArray<NSDictionary<NSString *, id> *> *)eventReport;
- (NSUInteger)unitTestAttributesReportedSinceLastCheck;
- (void)unitTestClearClusterData;
@end
#endif

Expand Down

0 comments on commit 0b59971

Please sign in to comment.