[Proposal] `ProgressManager` Post-Review Update #1519

chloe-yeo · 2025-09-24T00:12:55Z

Proposal change to reflect minor implementation change due to performance concerns of previously-proposed version of ProgressManager API.

Updated implementation found here: #1468

chloe-yeo · 2025-09-24T00:14:18Z

@swift-ci please test

itingliu · 2025-09-24T00:33:56Z

Proposals/0023-progress-manager.md

 We initially allowed the full flexibility of allowing developers to declare `ProgressManager.Property` types to be of any type, including structs. However, we realized that this has a severely negative impact on performance of the API. Thus, for now, we allow developers to only declare `ProgressManager.Property` with only certain `Value` and `Summary` types. 

+### Introduce `withProperties` closure as an entry point to atomically mutate multiple properties
+We initially considered introducing a withProperties closure as an entry point to atomically mutate multiple properties. This approach would have used a dedicated ProgressManager.Values struct with `@dynamicMemberLookup` to provide a unified interface for modifying all properties within a single atomic operation. 


Suggested change

We initially considered introducing a withProperties closure as an entry point to atomically mutate multiple properties. This approach would have used a dedicated ProgressManager.Values struct with `@dynamicMemberLookup` to provide a unified interface for modifying all properties within a single atomic operation.

We initially considered introducing a `withProperties` closure as an entry point to atomically mutate multiple properties. This approach would have used a dedicated `ProgressManager.Values` struct with `@dynamicMemberLookup` to provide a unified interface for modifying all properties within a single atomic operation.

itingliu · 2025-09-24T00:38:55Z

Proposals/0023-progress-manager.md

+We initially considered introducing a withProperties closure as an entry point to atomically mutate multiple properties. This approach would have used a dedicated ProgressManager.Values struct with `@dynamicMemberLookup` to provide a unified interface for modifying all properties within a single atomic operation. 
+
+However, this design had several drawbacks:
+- Limited atomicity: While it allowed atomic mutation of `totalCount` and `completedCount`, it could not achieve the same atomicity for additional properties due to their differences in backing storage


I would add a few sentences here about why backing storage (an implementation detail) would affect this change of design. Why does it matter whether they're all stored into one single storage or multiple separate ones?

Either way, is it correct to say that if we are to enforce a whole atomicity for all of the additional properties, we would have to synchronize the access with a big lock for all of the properties, and would hinder performances?

For reasons related to backing storage, my line of thinking is that If we were to allow totalFileCount and completedFileCount to be stored in a struct the way totalCount and completedCount are, we should then also allow developers to do the same. But this will then then mean we have to allow developers to declare any ProgressManager.Property, making us store (any Sendable). But for performance reasons, we do restrict them to only certain types such as Int, UInt64 etc. I think it would be more about consistency between what our pre-declared custom properties can do vs what developers' custom properties can do.

Why should an API user care about how totalFileCount and completedFileCount are stored, whether they're stored in a struct or not 😉 ? It could also be guarded by a semaphore for concurrent access. Would that allow atomically mutating everything?

Hmm you're right, those are implementation details, I think the thing that would matter most to developers, especially UI developers, will be the performance cost that comes with having withProperties. Because withProperties acquires the lock and does the access and mutation of additional properties after, and releasing the lock before returning the values, the way we make these properties Observable is much less performant. We have to register all access and mutations of additional properties onto only one keypath because the access and withMutation calls on the ObservationRegistrar, which may cause unexpected and unnecessary redraws of the UI, putting performance cost on developers who opt to report additional properties with their progress via withProperties.

itingliu · 2025-09-24T00:39:55Z

Proposals/0023-progress-manager.md

+
+However, this design had several drawbacks:
+- Limited atomicity: While it allowed atomic mutation of `totalCount` and `completedCount`, it could not achieve the same atomicity for additional properties due to their differences in backing storage
+- Less intuitive API: The withProperties closure promoted a less natural pattern for accessing and mutating additional properties, particularly when combined with `@dynamicMemberLookup` on the nested `ProgressManager.Values` struct


Suggested change

- Less intuitive API: The withProperties closure promoted a less natural pattern for accessing and mutating additional properties, particularly when combined with `@dynamicMemberLookup` on the nested `ProgressManager.Values` struct

- Less intuitive API: The `withProperties` closure promoted a less natural pattern for accessing and mutating additional properties, particularly when combined with `@dynamicMemberLookup` on the nested `ProgressManager.Values` struct

itingliu · 2025-09-24T00:41:22Z

Proposals/0023-progress-manager.md

+However, this design had several drawbacks:
+- Limited atomicity: While it allowed atomic mutation of `totalCount` and `completedCount`, it could not achieve the same atomicity for additional properties due to their differences in backing storage
+- Less intuitive API: The withProperties closure promoted a less natural pattern for accessing and mutating additional properties, particularly when combined with `@dynamicMemberLookup` on the nested `ProgressManager.Values` struct
+- Inconsistent access patterns: We would need to use different patterns for accessing fraction-related properties and additional properties


Were there discussions or feedback in the current version that was supportive of this pattern? What was the justification back then, and are they no longer valid?

Actually the discussion that I had in mind was covered under less intuitive API, this is not needed.

Proposals/0023-progress-manager.md

itingliu · 2025-09-24T00:43:25Z

Proposals/0023-progress-manager.md

+- Inconsistent access patterns: We would need to use different patterns for accessing fraction-related properties and additional properties
+
+We ultimately decided to replace this approach with a more streamlined design:
+- `setCounts` closure: Atomically update `totalCount` and `completedCount` 


Can you clarify why only totalCount and completedCount are considered here, and why do you think that there needs to be a closure to atomically update both of them?

Yep, sounds good!

itingliu · 2025-09-24T00:44:03Z

Proposals/0023-progress-manager.md

+With this change, we can atomically update `totalCount` and `completedCount` via the `setCounts` closure, and access and mutate additional properties via dot syntax. 
+
+### Minimal requirements for `ProgressManager.Property` protocol 
+We initially considered a simpler version of the ProgressManager.Property protocol that required only two components:


Suggested change

We initially considered a simpler version of the ProgressManager.Property protocol that required only two components:

We initially considered a simpler version of the `ProgressManager.Property` protocol that had two requirements:

I think we just call them requirements

I'll change this!

itingliu · 2025-09-24T00:46:16Z

Proposals/0023-progress-manager.md

+- `Value` - The type for individual property values
+- `defaultValue` - A default value when the property isn't explicitly set
+
+In this simplified approach, the framework would automatically:


Can you clarify what "the framework" refers to? Did you mean Foundation would do the below?

Yeah, but let me reword this!

chloe-yeo · 2025-09-24T21:43:37Z

@swift-ci please test

add alternatives considered

e6af6b7

itingliu reviewed Sep 24, 2025

View reviewed changes

chloe-yeo added 3 commits September 23, 2025 18:36

address PR comments

48621b6

refine Property protocol alternatives considered

e12818e

refine withProperties alternatives considered

2e08960

chloe-yeo requested a review from parkera September 24, 2025 18:16

chloe-yeo added 6 commits September 24, 2025 12:06

refine withProperties alternatives considered

6845fc6

more detail for alternatives considerd

864df54

edit proposal revision history

67a5a09

edit alternatives considered

3fcafe7

fix typo

655e19d

fix alternatives considered

6342785

parkera approved these changes Sep 25, 2025

View reviewed changes

	We initially considered introducing a withProperties closure as an entry point to atomically mutate multiple properties. This approach would have used a dedicated ProgressManager.Values struct with `@dynamicMemberLookup` to provide a unified interface for modifying all properties within a single atomic operation.
	We initially considered introducing a `withProperties` closure as an entry point to atomically mutate multiple properties. This approach would have used a dedicated `ProgressManager.Values` struct with `@dynamicMemberLookup` to provide a unified interface for modifying all properties within a single atomic operation.

	- Less intuitive API: The withProperties closure promoted a less natural pattern for accessing and mutating additional properties, particularly when combined with `@dynamicMemberLookup` on the nested `ProgressManager.Values` struct
	- Less intuitive API: The `withProperties` closure promoted a less natural pattern for accessing and mutating additional properties, particularly when combined with `@dynamicMemberLookup` on the nested `ProgressManager.Values` struct

	We initially considered a simpler version of the ProgressManager.Property protocol that required only two components:
	We initially considered a simpler version of the `ProgressManager.Property` protocol that had two requirements:

[Proposal] ProgressManager Post-Review Update #1519

Are you sure you want to change the base?

[Proposal] ProgressManager Post-Review Update #1519

Uh oh!

Conversation

chloe-yeo commented Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chloe-yeo commented Sep 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

itingliu Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chloe-yeo Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chloe-yeo commented Sep 24, 2025

Uh oh!

Uh oh!

[Proposal] `ProgressManager` Post-Review Update #1519

[Proposal] `ProgressManager` Post-Review Update #1519

chloe-yeo commented Sep 24, 2025 •

edited

Loading

itingliu Sep 24, 2025 •

edited

Loading

chloe-yeo Sep 24, 2025 •

edited

Loading