Support operation coallescing #141
Conversation
|
@garrettmoon @appleguy: Appreciate a review here :) |
nguyenhuy
force-pushed
the
135_coalescing_operations
branch
from
bceac42 to
de1b23f
Compare
… files inside one by one
nguyenhuy
force-pushed
the
135_coalescing_operations
branch
from
de1b23f to
21ae7a0
Compare
| typedef NS_ENUM(NSUInteger, PINDiskCacheCondition) { | ||
| PINDiskCacheConditionNotReady = 0, | ||
| PINDiskCacheConditionReady = 1, | ||
| }; | ||
|
|
||
| static PINOperationDataCoallescingBlock PINTrimmingDataCoallescingBlock = ^id(id existingData, id newData) { |
There was a problem hiding this comment.
This block works for both dates and bytes, right? I'm wondering if it would still be better to separate them so that we don't have to pass in ids. Is that possible? I'm not sure if Xcode will be happy with upcasting, but if it is, it would make this more readable even though it's duplicated code.
On another note, should they be named with PINDisk prefixing them?
| dispatch_block_t completion = nil; | ||
| if (block) { | ||
| completion = ^{ | ||
| PINDiskCache *strongSelf = weakSelf; | ||
| block(strongSelf); |
There was a problem hiding this comment.
Because we guarantee the instance of PINDiskCache to be non-null, I think we may need to not weakify here. I realize it was wrong before.
| dispatch_block_t completion = nil; | ||
| if (block) { | ||
| completion = ^{ | ||
| PINDiskCache *strongSelf = weakSelf; |
There was a problem hiding this comment.
Same thing here, we shouldn't weakify / strongify I don't think.
| dispatch_block_t completion = nil; | ||
| if (block) { | ||
| completion = ^{ | ||
| PINDiskCache *strongSelf = weakSelf; |
There was a problem hiding this comment.
No weakify / strongify
| @@ -27,6 +30,7 @@ typedef NS_ENUM(NSUInteger, PINOperationQueuePriority) { | |||
|
|
|||
| - (id <PINOperationReference>)addOperation:(dispatch_block_t)operation; | |||
| - (id <PINOperationReference>)addOperation:(dispatch_block_t)operation withPriority:(PINOperationQueuePriority)priority; | |||
| - (id <PINOperationReference>)addOperation:(PINOperationBlock)operation withPriority:(PINOperationQueuePriority)priority identifier:(NSString *)identifier data:(nullable id)data dataCoallescingBlock:(nullable PINOperationDataCoallescingBlock)dataCoallescingBlock completion:(nullable dispatch_block_t)completion; | |||
There was a problem hiding this comment.
Mind putting newlines between the params? This is getting a bit hairy :)
| [_queuedOperations addObject:operation]; | ||
| [_referenceToOperations setObject:operation forKey:reference]; | ||
| if (identifier != nil && (operation = [_identifierToOperations objectForKey:identifier]) != nil) { | ||
| // There is an exisiting operation with the provided identifier, let's coallescing these operations |
There was a problem hiding this comment.
nit: should be "let's coalesce these operations"
| } else { | ||
| operation.completion = ^{ | ||
| initialCompletion(); | ||
| completion(); |
There was a problem hiding this comment.
I'm worried that if you had a really large number of operations this could cause a significant overhead. Should it be an array of completions instead? That way it doesn't need to be nested within blocks?
| if (identifier != nil && (operation = [_identifierToOperations objectForKey:identifier]) != nil) { | ||
| // There is an exisiting operation with the provided identifier, let's coallescing these operations | ||
| if (dataCoallescingBlock != nil) { | ||
| operation.data = dataCoallescingBlock(operation.data, data); |
There was a problem hiding this comment.
I think we should probably unlock while calling this method in case someone tries to do something very expensive. We'd probably need to grab a reference to operation.data before unlocking.
There was a problem hiding this comment.
I'm worried that if we release the lock here, the operation might be grabbed on another thread and executed. That's the last think we want to happen.
|
|
||
| return reference; | ||
| return operation.reference; |
There was a problem hiding this comment.
Is it safe to access operation.reference without holding the lock? Should we grab a copy before unlocking? I guess since we don't mutate it anywhere it's safe, but we should add a comment to that effect at the very least?
| @@ -98,6 +105,7 @@ - (instancetype)initWithMaxConcurrentOperations:(NSUInteger)maxConcurrentOperati | |||
| _highPriorityOperations = [[NSMutableOrderedSet alloc] init]; | |||
|
|
|||
| _referenceToOperations = [NSMapTable weakToWeakObjectsMapTable]; | |||
| _identifierToOperations = [NSMapTable weakToWeakObjectsMapTable]; | |||
There was a problem hiding this comment.
Is it safe for this to be weakToWeak? I think it may need to be weakToStrong, as we don't know if callers of the api will keep strong references to their identifiers (i.e. they may just pass in a locally allocated NSString which could fall out of scope).
I think that should be ok though because they'll fall out when the identifier gets released?
There was a problem hiding this comment.
I think we're good here because the operation already keeps a strong reference to its identifier.
|
I am soooo excited by this BTW. |
nguyenhuy
force-pushed
the
135_coalescing_operations
branch
from
214165a to
f195dd2
Compare
nguyenhuy
force-pushed
the
135_coalescing_operations
branch
from
f195dd2 to
92c8587
Compare
Author: Huy Nguyen
Add a generic operation coallescing mechanism and use it to fix #135, #137.