* [ASNetworkImageNode] Assert if both URL and image are set
Here's one other idea for addressing this problem. Essentially,
we assert if you've set both the URL and the image. I've played around
with Pinterest and there's only one case where we hit this (the transition).
So I've also added another method (which is a bummer, it's weird I know)
but there's one good reason to add this method, ephemeralImage, which is
the user doesn't have to manually clear it out like they would if they
used defaultImage to save memory.
Putting this up for discussion.
* Fix comment
* Oh yeah, @dynamic, thanks @adlai
* Remove ephemeralImage
* Don't lock while calling downloader
Addresses #2785
To avoid performance issues, we should avoid locking the downloader.
To achieve this we need to do some kinda gross things. Essentially
the cost is the code is more complex and potentially far less performant
in edge cases. In testing, edge cases are nearly never hit, but I'm not
sure how good I feel about the cost in code complexity. This exacerbates
the locking issues in ASNetworkImageNode:
1. There is no convention for which methods lock.
2. There's no indication which vars are only set on init and therefore
safe to access except in the class extension definition.
* Shouldn't have checked in product changes.
* Using ivar instead of local var copied within lock.
* Implement mutex ownership and use it to check potential upward lock gathering
* Don't hold instance lock and call willEnterHierarchy/didExitHierarchy of ASDisplayNode
- This can cause deadlocks (e.g #2605) if subsequent methods, that are implemented by developers, walk up the node tree.
- This is a way to keep the optimizations introduced in 9e87813 while making sure the locking situation is a bit safer.
* More lock ownership assertions in ASDisplayNode
* Document main thread contract of -clearContents
* ASNetworkImageNode shoud not call setNeedsPreload while holding instance lock
- This helps to avoid potentially deadlocks caused if the node (esp in case it's a subclass of ASNetworkImageNode) walks up the tree in didEnterPreloadState, for example to build logging context.
* ASVideoNode should not call setNeedsPreload while holding instance lock
- This helps to avoid potentially deadlocks caused if the node (esp. if it's a subclass of ASVideoNode) walks up the tree in didEnterPreloadState, for example to build logging context.
* Don't hold instance lock for the entire insert subnode operation
- The recursive lock should not be held throughout `_insertSubnode:atSubnodeIndex:sublayerIndex:andRemoveSubnode:`. The method instead should manage the lock itself and acquire it as shortly as possible. The reason is that this method calls many methods outside the scope of `self`. `[subnode __setSupernode:self]` is especially troublesome because it causes the subnode to migrate to new hierarchy and interface states, which triggers `didEnter/Exit(.*)State` methods. These methods are meant to be overriden by subclasses. Thus they might walk up the node tree and potentially cause deadlocks, or they perform expensive tasks and cause the lock to be held for too long.
- Other methods that call this method should release the lock before doing so.
* Lock getter and setter of `synchronous` flag
* Address comment in ASVideoNode
* Add main thread assertions to methods that change asset and assetURL of ASVideoNode
* Explain CHECK_LOCKING_SAFETY flag
* More thread and locking assertions in ASVideNode
- It's not safe to call `-[subnode __setSupernode:self]` while holding instance lock of soon-to-be supernode (e.g `self`).
- It's also not safe to call `[subnode __setSupernode:nil]` while holding the instance lock of the old supernode (e.g `self`).
- did(Enter|Exit)(.*)State methods are always called on main. Add main thread assertions to indicate that.
* Minor change in explanation of CHECK_LOCKING_SAFETY flag
After much discussion I think this is the correct behavior. It seems
like it's much more likely to be the expected behavior but still enables
you to use the network image node like an regular image node.
* [ASDisplayNode] Ensure all subclasses are using base class __instanceLock__ and not re-defining their own.
This also moves the @package definition of the instance variable to +FrameworkPrivate instead of Internal.h,
because Internal.h should ideally not be used outside of the ASDisplayNode file setup. This has greatly reduced
the number of imports of Internal.h.
* [ASDisplayNode] Add ASDisplayNode+FrameworkSubclasses.h to share __instanceLock__ definition.
* Replace fetch data with preload terminology
- Deprecate `-fetchData` and `-clearFetchedData` in favor of `-preload` and `-clearPreloadedData`
- Move `-setNeedsPreload`, `-recursivelyPreload` and `-recursivelyClearPreloadedData` to ASDisplayNode+FrameworkPrivate.h
- Update internal implementation, comments and tests
* Folllow up on #2642:
- Remove -preload and -clearPreloadedData in favor of -didEnterPreloadState and -didExitPreloadState.
- -didEnterPreloadState and -didExitPreloadState call the deprecated -fetchData and -clearFetchedData methods if they are overriden.
* Missed one in a test
* Get rid of behavior change for now.
* Revert more behavior changes, fix tests.
* Don't need these anymore.
* Check in ASLayout if size is valid for sizing instead of valid for layout
* Return constrainedSize from calculateSizeThatFits:
Remove invalid constrainedSize check within ASNetworkImageNode. Furthermore as ASDisplayNode does not return CGSizeZero anymore we have to give the display nodes we use in tests and are involving a stack spec an intrinsic content size.
* Remove extra constrainedSize handling in ASNetworkImageNode handling
* Change test to use FLT_MAX
* [ASRunLoopQueue - Performance] Add ASDeallocQueue for efficient object teardown.
This measurably reduces block overhead and context switching. In the layout benchmark,
it increases ops/s while actually reducing CPU utilization. This suggests that we are
now at a lock-bounded local maximum, at least for tri-core devices.
* [ASDeallocQueue] Update convenience helper method and adopt in ASImageNode etc.
* [ASDeallocQueue] Reimplement the queue using a timer-based runloop.
* [Debugging] Re-enable ASDisplayNode Event Log.
* [ASDeallocQueue] Final refinements, comments, code minimization.
* [ASDeallocQueue] Fix for lock release needed in early return (refactoring typo from last commit)
* Remove old deprecated methods. Will restore ones that were removed recently based on PR.
* Update example to use non-deprecated method.
* Don't remove locking / unlocking, insets or willDisplayNode deprecated methods yet.
* Renamed range update callbacks
We finally settled on
didEnter/ExitDisplayState
didEnter/ExitPreloadState
didEnter/ExitVisibleState
This change is meant to unify the range update methods to relate to each
other and hopefully be a bit more self explanatory.
* Guarantee interface callbacks happen on main.
* move fetchData / clearFetchedData to default implementations
* Move deprecated methods to new deprecated category
* Don't bring in cocoapod change.
* Nits
* Capetalize
* [ASTextNode, ASVideoNode] Use ASDisplayNode base class lock for subclass property synchronization
* fix headers to match master
* address @appleguy, @maicki comments
* import header
* Swap lock in ASNetworkImageNode as well
* remove invalid comment
* more cleanup of locks
Summary:
Because we trampoline to main when setting _currentImageQuality, there would be situations where displayWillStart was called, we get a cached image and set currentImageQuality to 1.0, and then a background thread calling setDefaultImage would enqueue an operation that sets currentImageQuality to 0 and overwrites the correct value
Calling _updateProgressImageBlockOnDownloaderIfNeeded should be called without _lock held. We will lock super to read our interface state and it's best to avoid acquiring both locks.
