<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=""><div class="">Charles and Tony,</div><div class=""><br class=""></div><div class="">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?</div><div class=""><br class=""></div><div class="">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?</div><div class=""><br class=""></div><div class="">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.</div><div class=""><br class=""></div><div class="">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.</div><div class=""><br class=""></div><div class="">There are also a few comments inline.</div><div class=""><br class=""></div><div class="">-Rod</div><div class=""><br class=""></div><br class=""><div><blockquote type="cite" class=""><div class="">On 23 Feb 2017, at 6:41 am, Charles Srstka <<a href="mailto:cocoadev@charlessoft.com" class="">cocoadev@charlessoft.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><blockquote type="cite" class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;">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 style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><blockquote type="cite" class=""><br class="Apple-interchange-newline"><div class=""><div class="" applecontenteditable="true" style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">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 class=""><br class=""></div><div class="">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></div></div></div></blockquote><div><br class=""></div>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?<br class=""><div><br class=""></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=""><div style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><blockquote type="cite" class=""><div class=""><div class="" applecontenteditable="true" style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><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 class=""><br class=""></div><div class="">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 class="" applecontenteditable="true" style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><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 class=""><br class=""></div><div class="">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 class="" applecontenteditable="true" style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><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 class=""><br class=""></div><div class="">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 class=""><br class=""></div><div class="">class SomeClass {</div><div class=""><span class="Apple-tab-span" style="white-space: pre;"> </span>…</div><div class=""><br class=""></div><div class=""><span class="Apple-tab-span" style="white-space: pre;"> </span>func doSomething {</div><div class=""><span class="Apple-tab-span" style="white-space: pre;"> </span>self.subTaskA()</div><div class=""><span class="Apple-tab-span" style="white-space: pre;"> </span>self.subTaskB()</div><div class=""><span class="Apple-tab-span" style="white-space: pre;"> </span>self.subTaskC()</div><div class=""><span class="Apple-tab-span" style="white-space: pre;"> </span>}</div><div class=""><br class=""></div><div class=""><span class="Apple-tab-span" style="white-space: pre;"> </span>func subTaskA() {</div><div class=""><span class="Apple-tab-span" style="white-space: pre;"> </span>// do something</div><div class=""><span class="Apple-tab-span" style="white-space: pre;"> </span>}</div><div class=""><br class=""></div><div class=""><span class="Apple-tab-span" style="white-space: pre;"> </span>func subTaskB() {</div><div class=""><span class="Apple-tab-span" style="white-space: pre;"> </span>// do something</div><div class=""><span class="Apple-tab-span" style="white-space: pre;"> </span>}</div><div class=""><br class=""></div><div class=""><span class="Apple-tab-span" style="white-space: pre;"> </span>func subTaskC() {</div><div class=""><span class="Apple-tab-span" style="white-space: pre;"> </span>self.subTaskD()</div><div class=""><span class="Apple-tab-span" style="white-space: pre;"> </span>self.subTaskE()</div><div class=""><span class="Apple-tab-span" style="white-space: pre;"> </span>}</div><div class=""><br class=""></div><div class=""><span class="Apple-tab-span" style="white-space: pre;"> </span>… etc ...</div><div class="">}</div><div class=""><br class=""></div><div class="">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 class="" applecontenteditable="true" style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><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 class=""><br class=""></div><div class="">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 class=""><br class=""></div><div class="">0.0</div><div class="">0.003</div><div class="">0.009</div><div class="">0.012 <- FIRE: 0.012 - 0.0 is greater than 0.01</div><div class="">0.015</div><div class="">0.020</div><div class="">0.023 <- FIRE: 0.023 - 0.012 is greater than 0.01</div><div class="">0.025</div><div class="">0.027</div><div class=""><br class=""></div><div class="">… etc …</div><div class=""><br class=""></div><div class="">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 class="" applecontenteditable="true" style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><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 style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">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></div></blockquote><div><br class=""></div><div>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.</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=""><div style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br class=""></div><div style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">Charles</div></div></div></blockquote></div><br class=""></body></html>