Background image load api #1007

wsdwsd0829 · 2018-07-04T00:15:28Z

Currently delegate methods are called on main.
We may want to deprecated those and introduce the calls on background thread?
The benefit is that user have flexibility to log time of image fetched to image displayed/decoded.
This is similar to NSURLSession's didReceive data.

Adlai-Holler

I'd be totally fine moving forward with this – we see the main thread getting pretty clogged e.g. during launch.

That said, does it make more sense to have a global property on ASNetworkImageNode @property (class) BOOL delegateCallbacksOnMainThread; // default YES?

As a matter of pure curiosity, what is the motivation behind this for your use case? It's worthwhile regardless but it might be worth knowing.

Finally, it may make sense to send these callbacks on a serial queue. Using a concurrent queue, for example, two load events could theoretically be delivered out-of-order (didFetch: and then didFailToFetch: when in fact the failure came before the success).

Adlai-Holler · 2018-07-09T16:53:03Z

Source/ASNetworkImageNode.h

+ *
+ * @discussion Called on a background queue.
+ */
+- (void)imageNode:(ASNetworkImageNode *)imageNode didFetchImage:(UIImage *)image;


Creating ASNetworkImageLoadInfo objects is really cheap, so if we're revising this API I'd advocate for removing this method and only providing didFetchImage:info: and didFailFetchingWithError:.

If you choose to go that route, update the docs for didFetchImage:info: to remove the reference to this method.

Adlai-Holler · 2018-07-09T16:53:17Z

Source/ASNetworkImageNode.h


 /**
 * Notification that the image node finished downloading an image.
 *
 * @param imageNode The sender.
 * @param image The newly-loaded image.
 *
- * @discussion Called on a background queue.
+ * @discussion Called on a main queue.


Nit: Called on the main thread.

main or background. 2. remove non-info api.

wsdwsd0829 · 2018-07-12T18:43:51Z

+1 for @Property (class) BOOL delegateCallbacksOnMainThread; I was thinking to add an experiment flag instead, but seems an overkill.
it's for logging purpose, to match ticks for existing image loading system (pre PRI).
I did not get this, looks like didFetch & didFailToFetch are mutual exclusive?
Dropped didFetchImage: to use info: only.

Adlai-Holler · 2018-07-12T22:34:58Z

@wsdwsd0829 3 is a pretty fantastical case, but theoretically you could have the following sequence:

set URL to x
didFailToFetch: is enqueued
set URL to y
didFetch: is enqueued
didFailToFetch: is dequeued and the thread is put to sleep
didFetch: is dequeued and executed
didFailToFetch: thread wakes up and executes

From the observer's perspective, the first load succeeded and the second one failed, but in truth the order is reversed.

However, that risk is so unlikely and the proper solution would be to share a pool of serial queues, where each node gets assigned a queue, which isn't worth the extra effort.

Adlai-Holler · 2018-07-12T22:36:05Z

Source/ASNetworkImageNode.mm

+  static dispatch_once_t onceToken;
+  dispatch_once(&onceToken, ^{
+    [self class].delegateCallbacksOnMainThread = YES;
+  });


I think we can remove this since the variable is initialized to YES.

Adlai-Holler · 2018-07-12T22:38:04Z

Source/ASNetworkImageNode.h

+ *
+ * @discussion Called on a background queue.
+ */
+- (void)imageNode:(ASNetworkImageNode *)imageNode didFailFetchingWithError:(NSError *)error;


Now that we have this flag, we can continue using the old methods and remove these.

We also need to update the documentation of the existing methods to say:

@discussion Called on the main thread if delegateCallbacksOnMainThread=YES (the default,) otherwise on a background thread.

I thought the goal is to remove the delegate to main ones eventually. If we use same call out block. Would it be harder handle the deprecate marks?
However, maybe your suggest is better and make code much cleaner, so I will go for it.

wsdwsd0829 · 2018-07-12T23:20:30Z

I see for 3, I was assuming user would only use one image node for one url. (I do not see a use case for one node with multiple urls). I suggest we postpone the work until we need it? ( current impl have the issue right?)

Adlai-Holler · 2018-07-12T23:21:28Z

Yes

…

Sent from my iPhone

On Jul 12, 2018, at 4:20 PM, Max Wang ***@***.***> wrote: I see for 3, I was assuming user would only use one image node for one url. (I do not see a use case for one node with multiple urls). I suggest we postpone the work until we need it? ( current impl have the issue right?) — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

appleguy · 2018-07-13T18:24:36Z

Source/ASNetworkImageNode.h

+/**
+ * The delegate will receive callbacks on main thread. Default to YES.
+ */
+@property (class) BOOL delegateCallbacksOnMainThread;


Thinking of various names that might be good to establish a standard here - I wonder if "useMainThreadDelegateCallbacks" is any better?

appleguy · 2018-07-13T18:25:23Z

Source/ASNetworkImageNode.mm

@@ -426,6 +428,14 @@ - (void)didEnterPreloadState
  [self _lazilyLoadImageIfNecessary];
 }

+ (void)setDelegateCallbacksOnMainThread:(BOOL)delegateCallbacksOnMainThread {


Match file style with { on next line for all method definitions.

appleguy · 2018-07-13T18:25:43Z

Source/ASNetworkImageNode.mm

@@ -716,7 +726,7 @@ - (void)_lazilyLoadImageIfNecessary
          strongSelf->_cacheSentinel++;

          void (^calloutBlock)(ASNetworkImageNode *inst);
-          
+


Revert this whitespace change

Adlai-Holler · 2018-07-18T20:04:06Z

Source/ASNetworkImageNode.mm

@@ -85,6 +85,8 @@ @interface ASNetworkImageNode ()

 @implementation ASNetworkImageNode

+static BOOL _useMainThreadDelegateCallbacks = YES;


Technically this should be static std::atomic_bool _useMainThreadDelegateCallbacks(true) or some variant. The other code in here should be OK, since c++ defines the operators to implicitly do store/load.

In practice the risk is trivial. I approve either way – let's get this landed!

…und-image-load-api

Adlai-Holler · 2018-07-23T16:55:02Z

@nguyenhuy @garrettmoon @maicki Would someone be willing to approve the build?

maicki · 2018-07-23T16:59:05Z

@Adlai-Holler You got it

ghost · 2018-07-23T17:01:15Z

🚫 CI failed with log

wsdwsd0829 · 2018-07-24T17:13:13Z

How to trigger a new build? (I made a commit and seems not working).

…

On Tue, Jul 24, 2018 at 10:12 AM Huy Nguyen ***@***.***> wrote: ***@***.**** approved this pull request. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1007 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFFq5AtF_5bHbMxJ-b-RIh8uEy783qc2ks5uJ1VogaJpZM4VByYK> .

nguyenhuy · 2018-07-24T17:17:13Z

@wsdwsd0829 I've just triggered it manually. You have to modify "testable files", rather than markdown or json files, for the test suite to be run.

wsdwsd0829 · 2018-07-24T17:18:23Z

I see, thanks.

…

On Tue, Jul 24, 2018 at 10:17 AM Huy Nguyen ***@***.***> wrote: @wsdwsd0829 <https://github.com/wsdwsd0829> I've just triggered it manually. You have to modify "testable files", rather than markdown or json files, for the test suite to be run. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1007 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFFq5Hwgr9A1AVRoOagYza7Jy1197s0oks5uJ1adgaJpZM4VByYK> .

nguyenhuy · 2018-07-24T22:56:22Z

For the record, CI passed on d6c133a but GH's stuck on the old failure. I'm gonna land this PR.

* fix SIMULATE_WEB_RESPONSE not imported TextureGroup#449 * Fix to make rangeMode update in right time * remove uncessary assert * add api to allow delegated calls in background. * fix typo * 1. Add class property to decide whether to send delegate callbacks on main or background. 2. remove non-info api. * Refactor. * add ivar for class property. * Donot use extra api. * Refactor * refactor * revert to use let * refactor * make class property atomic. * kick of new ci test. * kick off new ci

wsdwsd0829 added 22 commits July 16, 2017 16:59

fix SIMULATE_WEB_RESPONSE not imported TextureGroup#449

929d118

Merge branch 'master' of github.com:TextureGroup/Texture

dd24d8f

Merge branch 'master' of github.com:TextureGroup/Texture

b8eaffa

Merge branch 'master' of github.com:TextureGroup/Texture

2918ea0

Merge branch 'master' of github.com:TextureGroup/Texture

9c42266

Fix to make rangeMode update in right time

329f35f

Merge branch 'master' of github.com:TextureGroup/Texture

5f8b7ec

merge master from upstream

d87bb11

Merge branch 'master' of github.com:TextureGroup/Texture

269c2ab

Merge branch 'master' of github.com:TextureGroup/Texture

24c1ce8

Merge branch 'master' of github.com:TextureGroup/Texture

233169e

Merge branch 'master' of github.com:TextureGroup/Texture

b19f90d

Merge branch 'master' of github.com:TextureGroup/Texture

b50cec4

Merge branch 'master' of github.com:TextureGroup/Texture

b75a5f3

Merge branch 'master' of github.com:TextureGroup/Texture

64b46e0

remove uncessary assert

5fabc1e

Merge branch 'master' of github.com:TextureGroup/Texture

3d5b84b

Merge branch 'master' of github.com:TextureGroup/Texture

098b978

Merge branch 'master' of github.com:TextureGroup/Texture

77eefd6

Merge branch 'master' of github.com:TextureGroup/Texture

0310ed7

add api to allow delegated calls in background.

324d3b0

fix typo

018867e

Adlai-Holler reviewed Jul 9, 2018

View reviewed changes

wsdwsd0829 added 3 commits July 12, 2018 10:09

1. Add class property to decide whether to send delegate callbacks on

1da91a6

main or background. 2. remove non-info api.

Refactor.

bdbd7de

add ivar for class property.

8480a75

merge masster

30fed6b

TextureGroup deleted a comment Jul 12, 2018

Adlai-Holler reviewed Jul 12, 2018

View reviewed changes

wsdwsd0829 added 5 commits July 13, 2018 10:55

Donot use extra api.

c17c0a1

Refactor

33a80df

refactor

4134330

revert to use let

f0419c2

Merge branch 'master' into background-image-load-api

6d1ce31

appleguy approved these changes Jul 13, 2018

View reviewed changes

refactor

0a0dbcc

Adlai-Holler approved these changes Jul 18, 2018

View reviewed changes

wsdwsd0829 added 3 commits July 18, 2018 14:17

make class property atomic.

c2d8d9b

kick of new ci test.

056a3d9

Merge branch 'master' of github.com:TextureGroup/Texture into backgro…

d6c133a

…und-image-load-api

TextureGroup deleted a comment Jul 19, 2018

maicki approved these changes Jul 23, 2018

View reviewed changes

kick off new ci

645adce

nguyenhuy approved these changes Jul 24, 2018

View reviewed changes

nguyenhuy merged commit 905c582 into TextureGroup:master Jul 24, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Background image load api #1007

Background image load api #1007

wsdwsd0829 commented Jul 4, 2018 •

edited

Loading

Adlai-Holler left a comment

Adlai-Holler Jul 9, 2018

Adlai-Holler Jul 9, 2018

wsdwsd0829 commented Jul 12, 2018

Adlai-Holler commented Jul 12, 2018

Adlai-Holler Jul 12, 2018

Adlai-Holler Jul 12, 2018

wsdwsd0829 Jul 12, 2018 •

edited

Loading

wsdwsd0829 commented Jul 12, 2018

Adlai-Holler commented Jul 12, 2018 via email

appleguy Jul 13, 2018

appleguy Jul 13, 2018

appleguy Jul 13, 2018

Adlai-Holler Jul 18, 2018

Adlai-Holler commented Jul 23, 2018

maicki commented Jul 23, 2018

ghost commented Jul 23, 2018

wsdwsd0829 commented Jul 24, 2018 via email

nguyenhuy commented Jul 24, 2018

wsdwsd0829 commented Jul 24, 2018 via email

nguyenhuy commented Jul 24, 2018

		@@ -716,7 +726,7 @@ - (void)_lazilyLoadImageIfNecessary
		strongSelf->_cacheSentinel++;

		void (^calloutBlock)(ASNetworkImageNode *inst);

		@@ -85,6 +85,8 @@ @interface ASNetworkImageNode ()

		@implementation ASNetworkImageNode

		static BOOL _useMainThreadDelegateCallbacks = YES;

Background image load api #1007

Background image load api #1007

Conversation

wsdwsd0829 commented Jul 4, 2018 • edited Loading

Adlai-Holler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wsdwsd0829 commented Jul 12, 2018

Adlai-Holler commented Jul 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wsdwsd0829 Jul 12, 2018 • edited Loading

Choose a reason for hiding this comment

wsdwsd0829 commented Jul 12, 2018

Adlai-Holler commented Jul 12, 2018 via email

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

Adlai-Holler commented Jul 23, 2018

maicki commented Jul 23, 2018

ghost commented Jul 23, 2018

wsdwsd0829 commented Jul 24, 2018 via email

nguyenhuy commented Jul 24, 2018

wsdwsd0829 commented Jul 24, 2018 via email

nguyenhuy commented Jul 24, 2018

wsdwsd0829 commented Jul 4, 2018 •

edited

Loading

wsdwsd0829 Jul 12, 2018 •

edited

Loading