[swift-evolution] Pitch: Much better-performing, Swift-native Progress replacement

[Rod Brown]

Charles and Tony,

Would it be more appropriate to consider suggesting that the Foundation team to consider adding a method of choosing their “notification granularity”, and avoid KVO notifications until it meets the granularity requirement, or some other way of minimising the reporting period of the Progress?

I think the big argument from you, Charles, is that the progress can become arbitrarily badly performing, depending on its use, and that could have untold effects up the chain with KVO notifications firing for minor elements of work?

I think this is a fair observation that the responsibility of a reporter shouldn’t be to rate limit the reporting rate for performance reasons. Rather, the observer should be able to adjust the reporting rate to something that is appropriate for the updates they wish to perform. A 50 pixel progress bar only theoretically needs reporting every adjustment of 1/50 of the total work, and other reporting and updating is superfluous and simply a performance problem. But why is a data task responsible for knowing that? It seems backwards.

Perhaps we need to examine the problem here, and how to solve that, rather than the cause? Because I agree with Tony - replacing every use of KVO on a performance basis isn’t a scalable solution.

There are also a few comments inline.

-Rod


> On 23 Feb 2017, at 6:41 am, Charles Srstka <cocoadev at charlessoft.com> wrote:
> 
>> On Feb 22, 2017, at 12:52 PM, Tony Parker <anthony.parker at apple.com <mailto:anthony.parker at apple.com>> wrote:
>> 
>> It seems like the main complaints about NSProgress revolve around KVO, which there is no question about not being right for Swift in many ways. I’m aware of that. I think the right answer there is instead to improve KVO in Swift, not to replace all KVO in the SDK on a case-by-case basis with ad-hoc observation mechanisms. I acknowledge that improving KVO is not a small task.
> 
> I am not proposing, and would never propose, replacing all KVO in the entire SDK. I do, however, believe that KVO is not the correct mechanism to use for progress reporting, a use case which incurs all of KVO’s drawbacks, while taking advantage of none of its benefits. The main advantage of KVO is in reducing glue code, allowing you to bind UI elements directly to your model in IB without having to set up the connections manually. Unfortunately, since UI elements must be updated from the main thread, and NSProgress is likely to be used on secondary threads to avoid blocking the UI, this advantage is nullified, since custom code must be written to receive KVO notifications and do work on the main thread. In fact, NSProgress’s use of KVO requires you to write much *more* glue code than otherwise, since the code needed to observe a KVO property manually is much longer, uglier, more error-prone, and generally more painful than a simple closure-based notification. Therefore, the use of KVO in NSProgress strikes me as akin to using a hammer to drive in a screw. There are better tools for this particular task.

I think the argument for KVO is more that this is the convention for binding UI to data in Obj-C and Cocoa, which would seem consistent. The question is can we find a way to improve this? Is that replacement, or evolution of the API?


>> Responding to some of the other notes in your description:
>> 
>> 
>> * KVO
>> 
>> NSProgress does not use KVO to update its parent progress objects. You can actually see this in the swift-corelibs-foundation version of NSProgress. It does post notifications for its properties this way though.
> 
> It does not use KVO to update the parent, but it does cause its parent, and its grandparent, and its great-grandparent, etc. all to post KVO notifications going all the way back up to the root of the tree. That is a lot of notifications.
> 
>> * Implicit tree composition
>> 
>> I agree that implicit tree composition is not a great idea except in very controlled circumstances. That’s why I introduced the explicit API.
> 
> I also agree; unfortunately, the explicit API as it currently exists is not ideal for procedural tasks (see below).
> 
>> * Explicit tree composition
>> 
>> It looks like you’ve used this incorrectly. The reason the ProgressReporting protocol exists is for types to expose a progress that the caller can then compose into their own progress tree. There is no requirement to use it, but it establishes the pattern.
>> 
>> // disclaimer: typed in Mail
>> class X {
>>     var progress: Progress {
>>         let p = Progress.discreteProgress(totalUnitCount: 10)
>>         // start with some progress
>>         p.completedUnitCount = 5
>>     }
>> }
>> 
>> var x = X()
>> var p = Progress.discreteProgress(totalUnitCount: 2)
>> var childA = Progress(totalUnitCount: 4, parent: p, pendingUnitCount: 1) // childA is 50% of p
>> p.addChild(x.progress, pendingUnitCount: 1) // x.progress is 50% of p
>> 
>> p.fractionCompleted // 0.25
> 
> Using ProgressReporting requires each subtask to be wrapped in a class (each of which needs to be Objective-C, to boot). When performing complex operations with many steps, it does not help:
> 
> class SomeClass {
> 	…
> 
> 	func doSomething {
> 		self.subTaskA()
> 		self.subTaskB()
> 		self.subTaskC()
> 	}
> 
> 	func subTaskA() {
> 		// do something
> 	}
> 
> 	func subTaskB() {
> 		// do something
> 	}
> 
> 	func subTaskC() {
> 		self.subTaskD()
> 		self.subTaskE()
> 	}
> 
> 	… etc ...
> }
> 
> Using ProgressReporting for tasks such as this requires that each function be a class, which not only bloats the Objective-C class hierarchy, but also requires substantial boilerplate, since each of those calls now requires three lines of code; one to create the class, one to attach its progress object to yours, and one to actually execute the function. It will also likely perform worse. A more elegant solution, IMO, would allow the sort of loose coupling that the implicit composition method provides, while avoiding the dark magics involved in using the implicit composition method.
> 
>> * Updating progress in a tight loop
>> 
>> No matter how efficient you make updating the properties (and, again, I acknowledge that KVO support adds a cost here), the fact that progress forms trees and the trees are designed to be arbitrarily large, means that you should always consider the cost of updating too frequently. Reducing the cost per update is still a noble goal. As is reducing autoreleases.
> 
> This is true, which is why my CSProgress class includes a property called “granularity”, which causes its notifications to fire only when the delta between the fractionCompleted property and its value at the time of the last fire exceeds some amount. So if the granularity is 0.01 (which is currently my default value for it), and the fraction updates like below, it’ll behave like:
> 
> 0.0
> 0.003
> 0.009
> 0.012 <- FIRE: 0.012 - 0.0 is greater than 0.01
> 0.015
> 0.020
> 0.023 <- FIRE: 0.023 - 0.012 is greater than 0.01
> 0.025
> 0.027
> 
> … etc …
> 
> This removes the burden of coalescing updates from the worker code, freeing it up to focus on its actual work.
> 
>> * Updating completedUnitCount atomically
>> 
>> The best practice here is to keep the progress object thread local. I think that updating one progress from multiple threads could be a code smell. Perhaps then you are doing several parts of the work and you should instead form a tree. This also leads in a direction where completed unit count is either 100% handed out to children or 100% controlled by your work-update-progress loop. Mixing the two leads to easy confusion about who is responsible for which portion. If you follow this rule, you never have to get the completed unit count, which means the race you describe does not exist.
> 
> NSProgress is designed to be thread-safe by protecting access to its properties with an NSLock, but it is not in fact thread-safe. It is making a promise to its users that it is not keeping. In addition, the lack of an atomic increment means that if you are getting data from an API that only reports deltas, you either have to get the old completedUnitCount, or keep track of the running total yourself. An atomic increment operation would not only improve thread safety, but it would improve the experience, since it would reduce the work the user needs to do, while remaining safe and performant.

I would agree with Tony that this would seem to be a code smell. Updating a single progress from two threads seems like you don’t have a handle on a centralised value for how much work is updated.

> 
> Charles

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.swift.org/pipermail/swift-evolution/attachments/20170223/17296d9f/attachment.html>