<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><blockquote type="cite" class="">On Feb 22, 2017, at 12:52 PM, Tony Parker <<a href="mailto:anthony.parker@apple.com" class="">anthony.parker@apple.com</a>> wrote:<br class=""></blockquote><div><blockquote type="cite" class=""><br class="Apple-interchange-newline"><div class=""><meta http-equiv="Content-Type" content="text/html charset=utf-8" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="" applecontenteditable="true">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.</div></div></blockquote><div><br class=""></div><div>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.</div><br class=""><blockquote type="cite" class=""><div class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="" applecontenteditable="true"><div class="">Responding to some of the other notes in your description:</div><div class=""><br class=""></div><div class=""><br class=""></div><div class="">* KVO</div><div class=""><br class=""></div><div class="">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.</div></div></div></blockquote><div><br class=""></div><div>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.</div><br class=""><blockquote type="cite" class=""><div class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="" applecontenteditable="true"><div class="">* Implicit tree composition</div><div class=""><br class=""></div><div class=""><div class="">I agree that implicit tree composition is not a great idea except in very controlled circumstances. That’s why I introduced the explicit API.</div></div></div></div></blockquote><div><br class=""></div><div>I also agree; unfortunately, the explicit API as it currently exists is not ideal for procedural tasks (see below).</div><br class=""><blockquote type="cite" class=""><div class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="" applecontenteditable="true"><div class="">* Explicit tree composition</div><div class=""><br class=""></div><div class="">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.</div><div class=""><br class=""></div><div class=""><font face="Menlo" class="">// disclaimer: typed in Mail</font></div><div class=""><font face="Menlo" class="">class X {</font></div><div class=""><font face="Menlo" class=""> var progress: Progress {</font></div><div class=""><font face="Menlo" class=""> let p = Progress.discreteProgress(totalUnitCount: 10)</font></div><div class=""><font face="Menlo" class=""> // start with some progress</font></div><div class=""><font face="Menlo" class=""> p.completedUnitCount = 5</font></div><div class=""><font face="Menlo" class=""> }</font></div><div class=""><font face="Menlo" class="">}</font></div><div class=""><font face="Menlo" class=""><br class=""></font></div><div class=""><font face="Menlo" class="">var x = X()</font></div><div class=""><font face="Menlo" class="">var p = Progress.discreteProgress(totalUnitCount: 2)</font></div><div class=""><font face="Menlo" class="">var childA = Progress(totalUnitCount: 4, parent: p, pendingUnitCount: 1) // childA is 50% of p</font></div><div class=""><font face="Menlo" class="">p.addChild(x.progress, pendingUnitCount: 1) // x.progress is 50% of p</font></div><div class=""><br class=""></div><div class=""><font face="Menlo" class="">p.fractionCompleted // 0.25</font></div></div></div></blockquote><div><br class=""></div><div>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:</div><div><br class=""></div><div>class SomeClass {</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>…</div><div><br class=""></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>func doSomething {</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>self.subTaskA()</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>self.subTaskB()</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>self.subTaskC()</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>}</div><div><br class=""></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>func subTaskA() {</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>// do something</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>}</div><div><br class=""></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>func subTaskB() {</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>// do something</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>}</div><div><br class=""></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>func subTaskC() {</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>self.subTaskD()</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>self.subTaskE()</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>}</div><div><br class=""></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>… etc ...</div><div>}</div><div><br class=""></div><div>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.</div><br class=""><blockquote type="cite" class=""><div class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="" applecontenteditable="true"><div class=""><div class="">* Updating progress in a tight loop</div><div class=""><br class=""></div><div class="">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.</div></div></div></div></blockquote><div><br class=""></div><div>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:</div><div><br class=""></div><div>0.0</div><div>0.003</div><div>0.009</div><div>0.012 <- FIRE: 0.012 - 0.0 is greater than 0.01</div><div>0.015</div><div>0.020</div><div>0.023 <- FIRE: 0.023 - 0.012 is greater than 0.01</div><div>0.025</div><div>0.027</div><div><br class=""></div><div>… etc …</div><div><br class=""></div><div>This removes the burden of coalescing updates from the worker code, freeing it up to focus on its actual work.</div><br class=""><blockquote type="cite" class=""><div class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="" applecontenteditable="true"><div class=""><div class="">* Updating completedUnitCount atomically</div><div class=""><br class=""></div><div class="">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.</div></div></div></div></blockquote><br class=""></div><div>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.</div><div><br class=""></div><div>Charles</div><div class=""><br class=""></div></body></html>