-
Notifications
You must be signed in to change notification settings - Fork 510
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactors out KVO usage on NSURLSessionTask #410
Conversation
Generated by 🚫 Danger |
This was causing various crashes due to what the internet suggests is an Apple bug. Sadly, this means that we'll only be getting valid bps and time to first byte on iOS 10 and 11. I think this is a reasonable tradeoff however.
14f86a9
to
6d0f7c5
Compare
Source/Classes/PINSpeedRecorder.h
Outdated
@@ -21,8 +21,27 @@ typedef enum : NSUInteger { | |||
lowQualityQPSThreshold:(float)lowQualityQPSThreshold | |||
highQualityQPSThreshold:(float)highQualityQPSThreshold; | |||
|
|||
- (void)addTaskBPS:(float)bytesPerSecond endDate:(NSDate *)endDate; | |||
- (float)currentBytesPerSecond; | |||
- (void)processMetrics:(NSURLSessionTaskMetrics *)metrics forTask:(NSURLSessionTask *)task host:(NSString *)host; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to remove the host
argument, and instead read it from task.currentRequest.URL.host
?
Source/Classes/PINSpeedRecorder.m
Outdated
@interface PINSpeedRecorder () | ||
{ | ||
NSCache *_speedMeasurements; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put generics on this cache?
Source/Classes/PINSpeedRecorder.m
Outdated
firstByte = metric.responseStartDate; | ||
} | ||
if ([requestEnd compare:metric.responseEndDate] != NSOrderedDescending) { | ||
requestEnd = metric.responseEndDate; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: This could be rephrased as requestStart = [requestStart earlierDate:metric.requestStartDate];
etc.
Source/Classes/PINSpeedRecorder.m
Outdated
[self updateSpeedsForHost:host | ||
bytesPerSecond:contentLength / [requestEnd timeIntervalSinceDate:requestStart] | ||
startAdjustedBytesPerSecond:contentLength / [requestEnd timeIntervalSinceDate:firstByte] | ||
timeToFirstByte:[firstByte timeIntervalSinceDate:requestStart]]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To reduce the amount of math in here, maybe just pass bytesPerSecond:measurement.bytesPerSecond
etc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, that measurement object was supposed to be deleted :)
@garrettmoon What time to releease new version? |
@garrettmoon @workhardupc +1 This is causing quite a few crashes for us, and we're waiting for release of |
+1, this is one of the top crashes in my app too. Let's get a new version of PINRemoteImage and Texture pushed out. |
@appleguy we also found the crash, but the latest version of Texture is not support the 'PINRemoteImage/WebP', '= 3.0.0-beta.13', so when you pushed out? |
@garrettmoon we are also look forward to the latest version of Texture can support PinRemoveImage with '= 3.0.0-beta.13' , do you know when the release will happen? |
This was causing various crashes due to what the internet suggests is
an Apple bug. Sadly, this means that we'll only be getting valid bps
and time to first byte on iOS 10 and 11. I think this is a reasonable
tradeoff however.
Should fix #390