add more delegate methods for monitoring network image node progress #1247

ernestmama · 2018-11-21T01:30:20Z

add more delegate methods for monitoring network image node progress so we can better keep track of optimization opportunity

nguyenhuy

Straight-forward additions, LGTM!

maicki · 2018-11-26T23:16:10Z

Source/ASNetworkImageNode.mm

@@ -335,6 +343,10 @@ - (void)displayWillStartAsynchronously:(BOOL)asynchronously
 {
  [super displayWillStartAsynchronously:asynchronously];

+  if (_delegateFlags.delegateWillStartDisplayAsynchronously) {


We don't grab a lock here to access the _delegateFlags and the _delegate here

will update

maicki · 2018-11-26T23:16:52Z

Source/ASNetworkImageNode.mm

@@ -773,18 +785,31 @@ - (void)_lazilyLoadImageIfNecessary
          }

          if ([imageContainer asdk_image] == nil && self->_downloader != nil) {
+            if (self->_delegateFlags.delegateWillLoadImageFromNetwork) {
+              [delegate imageNodeWillLoadImageFromNetwork:self];


All of this methods are called with the lock held. This can cause deadlocks. We should at least document this behavior if we cannot get around this.

I don't think these are locked. I tried putting a ASUnlockScope which result in an assertion failure. @nguyenhuy

This is inside a completion block and the lock is no longer held by the time it's executed. That's why we grabbed the lock before checking the cache sentinel a few line above. So we don't need to unlock here, but we need to grab the lock before accessing _delegateFlags. We can do so at the beginning of this method, similar to how we check the _delegateFlags.delegateDidStartFetchingData flag.

nguyenhuy

Good call on locking before accessing these properties, @maicki. Reviewed this diff again (more carefully this time).

nguyenhuy · 2018-11-27T00:01:01Z

Source/ASNetworkImageNode.mm

@@ -773,18 +785,31 @@ - (void)_lazilyLoadImageIfNecessary
          }

          if ([imageContainer asdk_image] == nil && self->_downloader != nil) {
+            if (self->_delegateFlags.delegateWillLoadImageFromNetwork) {
+              [delegate imageNodeWillLoadImageFromNetwork:self];


This is inside a completion block and the lock is no longer held by the time it's executed. That's why we grabbed the lock before checking the cache sentinel a few line above. So we don't need to unlock here, but we need to grab the lock before accessing _delegateFlags. We can do so at the beginning of this method, similar to how we check the _delegateFlags.delegateDidStartFetchingData flag.

nguyenhuy · 2018-11-27T00:06:20Z

Source/ASNetworkImageNode.mm

            [self _downloadImageWithCompletion:^(id<ASImageContainerProtocol> imageContainer, NSError *error, id downloadIdentifier, id userInfo) {
              finished(imageContainer, error, downloadIdentifier, ASNetworkImageSourceDownload, userInfo);
            }];
          } else {
+            if (self->_delegateFlags.delegateDidLoadImageFromCache) {


Grab the flag at the beginning of this method and use it here.

nguyenhuy · 2018-11-27T00:06:34Z

Source/ASNetworkImageNode.mm

            as_log_verbose(ASImageLoadingLog(), "Decached image for %@ img: %@ url: %@", self, [imageContainer asdk_image], URL);
            finished(imageContainer, nil, nil, ASNetworkImageSourceAsynchronousCache, nil);
          }
        };
+
+        if (self->_delegateFlags.delegateWillLoadImageFromCache) {


nguyenhuy · 2018-11-27T00:06:40Z

Source/ASNetworkImageNode.mm

        [_cache cachedImageWithURL:URL
                     callbackQueue:[self callbackQueue]
                        completion:completion];
      } else {
+        if (self->_delegateFlags.delegateWillLoadImageFromNetwork) {


nguyenhuy · 2018-11-27T00:09:04Z

Source/ASNetworkImageNode.mm

+        id<ASNetworkImageNodeDelegate> delegate;
+        {
+          delegate = _delegate;
+        }


Just use the cached delegate above.

…ure into networkImageNodeDelegate

nguyenhuy

1 last nit.

nguyenhuy · 2018-11-27T00:33:56Z

Source/ASNetworkImageNode.mm

+  id<ASNetworkImageNodeDelegate> delegate;
+  BOOL notifyDelegate;
+  {
+    ASLockedSelf();


Should be ASLockScopeSelf.

nguyenhuy · 2018-11-27T01:06:44Z

@maicki Can you take another look? Thanks!

ernestmama · 2018-11-27T01:09:10Z

Thanks @maicki and @nguyenhuy for helping on the Lock issue!

maicki · 2018-11-27T17:39:00Z

Source/ASNetworkImageNode.mm

@@ -335,6 +343,17 @@ - (void)displayWillStartAsynchronously:(BOOL)asynchronously
 {
  [super displayWillStartAsynchronously:asynchronously];

+  id<ASNetworkImageNodeDelegate> delegate;
+  BOOL notifyDelegate;
+  {


It's fine to grab the delegate in here with the lock held. One problem that will occur, what is not new and in fact we have it in almost all of our unlocked delegate calls, if the delegate needs more information from the node the information could already be out of date. Especially if you use this methods for logging this could introduce inconsistencies.

This is not a new problem though and we think about different solutions to find some general solution for this though.

Yeah, it's not a new problem, and I'd say delegates should rely on the data being passed via the delegate method. Not an ideal solution, but it's a fine trade-off.

I'm gonna land this PR now. Thanks for reviewing.

maicki · 2018-11-27T17:40:56Z

@ernestmama @nguyenhuy Added one comment. This should not be a blocker though as in fact we have this problem in other places too.

add more delegate methods for monitoring network image node progress

2e3aa22

nguyenhuy approved these changes Nov 26, 2018

View reviewed changes

maicki reviewed Nov 26, 2018

View reviewed changes

ernestmama and others added 3 commits November 26, 2018 15:53

add lock when accessing delegate flags and delegate

3807af4

grab the delegate before release the lock

660b4bf

Delete IDEWorkspaceChecks.plist

105ed20

nguyenhuy requested changes Nov 27, 2018

View reviewed changes

ernestmama added 2 commits November 26, 2018 16:15

get delegate from lock

552f94b

Merge branch 'networkImageNodeDelegate' of github.com:ernestmama/Text…

c21b5e2

…ure into networkImageNodeDelegate

nguyenhuy requested changes Nov 27, 2018

View reviewed changes

update lock method

7cca4a3

nguyenhuy approved these changes Nov 27, 2018

View reviewed changes

maicki reviewed Nov 27, 2018

View reviewed changes

nguyenhuy merged commit 4d1ffcc into TextureGroup:master Nov 27, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add more delegate methods for monitoring network image node progress #1247

add more delegate methods for monitoring network image node progress #1247

ernestmama commented Nov 21, 2018

nguyenhuy left a comment

maicki Nov 26, 2018

ernestmama Nov 26, 2018

maicki Nov 26, 2018

ernestmama Nov 26, 2018 •

edited

Loading

nguyenhuy Nov 27, 2018 •

edited

Loading

nguyenhuy left a comment

nguyenhuy Nov 27, 2018 •

edited

Loading

nguyenhuy Nov 27, 2018

nguyenhuy Nov 27, 2018

nguyenhuy Nov 27, 2018

nguyenhuy Nov 27, 2018 •

edited

Loading

nguyenhuy left a comment

nguyenhuy Nov 27, 2018

nguyenhuy commented Nov 27, 2018

ernestmama commented Nov 27, 2018

maicki Nov 27, 2018

nguyenhuy Nov 27, 2018

maicki commented Nov 27, 2018

add more delegate methods for monitoring network image node progress #1247

add more delegate methods for monitoring network image node progress #1247

Conversation

ernestmama commented Nov 21, 2018

nguyenhuy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ernestmama Nov 26, 2018 • edited Loading

Choose a reason for hiding this comment

nguyenhuy Nov 27, 2018 • edited Loading

Choose a reason for hiding this comment

nguyenhuy left a comment

Choose a reason for hiding this comment

nguyenhuy Nov 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nguyenhuy Nov 27, 2018 • edited Loading

Choose a reason for hiding this comment

nguyenhuy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nguyenhuy commented Nov 27, 2018

ernestmama commented Nov 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maicki commented Nov 27, 2018

ernestmama Nov 26, 2018 •

edited

Loading

nguyenhuy Nov 27, 2018 •

edited

Loading

nguyenhuy Nov 27, 2018 •

edited

Loading

nguyenhuy Nov 27, 2018 •

edited

Loading