Replies: 1 comment
-
This looks like a great plan! I totally agree that the concurrency story is very complex and it will take some time to iron out all the details.
I was thinking about calling it synchronize() too, but this confuses me in a few ways:
That's why I personally prefer something like "load()" or "restore()". However, I recognize that NSUbiquitousKeyValueStore has a synchronize method that works exactly like that, so maybe it's more familiar to people. |
Beta Was this translation helpful? Give feedback.
0 replies
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
-
As discussed in #19, I've developed a few principles for furthering Boutique.
Principle 1: Boutique must have a simple API
Principle 2: There must be cache consistency
Principle 3: Users expect good performance
But I also have plans for the Boutique's next features and improvements, and would like to discuss them here.
Upcoming Priorities
1. Fix low-hanging performance issues
This set of issues from @pofat is incredibly well documented and I'd like to start by solving them since they have the potential to make the biggest performance impact with the smallest affected surface area, a great combination. I intend to address both issues they mentioned about excessive writes to disk, both on app launch and every time items in the store are modified. I intend to apply the strategy he noted, to add some state to ensure that on first launch we don't rewrite data, and to only write or delete the newly added or removed items to disk.
To fix the issue of writes on launch, my simple solution is to add a flag that checks whether this is the first launch, and if it is then we don’t write any files to disk.
To fix the issue of rewriting all
items
every time there’s a mutation I intend to add a new private property for tracking the items that were changed. It will be an@Published
property likeitems
is, that way we can subscribe to changes the same way we currently do withitems
. Rather than being an array ofObject
this will be an array of a new type, aItemsChangeset
struct (name tbd) that contains an array of items added an an array of items removed. This will be used in both theadd()
andremove()
methods, and the current logic which separately removes files during a cache invalidation will be folded into here.I’m also considering a data structure to switch to internally, currently leaning towards
OrderedDictionary
. I'm also considering aSet
orDictionary
since files are loaded into memory in order based on their hashed name, a generally not useful sorting mechanism, and generally sorted by the developer at the Controller or View layer.2. Build a profiler app
To ensure that code changes do not cause regressions, especially community-submitted patches, I want to build an app for profiling performance. I’ve already started prototyping an app and it should be something I can finish pretty quickly. An added benefit is that this profiler app will be bundled as a target of the MVCS Demo project so users can have a quantifiable way to see just how many files can be held in memory and on disk. This also means I can make promises on the Boutique readme, allowing people to judge for themselves if Boutique is best for their needs.
3. Initial Load Policies + Cache Synchronization
This was inspired by @StefanCosminR’s patch and my discussion with @samalone about startup performance here. While the issue of long launch time ended up being related to Stuart storing large images in the Store, the initial load executes on the main actor regardless, rending it ostensibly a blocking operation. It's possible similar issues can occur if a user has a large Store, another reason building a profiler app before implementing this is my preferred priority.
To work around this Stefan proposed providing the initializer with a
prefetchPersistedItems: Bool
parameter, but I'd like to take a more generic approach. I plan on creating a new enum that will be a parameter passed into the initializer,InitialLoadPolicy
. It will have three cases,.none
,.all
, and.items(where: (Item) -> Bool)
, the first two should be pretty straightforward policies, and the last one will allow you to read a subset of items when initializing the Store. *If you choose to not load all items at Store creation there will be a promise enforced that by the time the
items
array is first accessed, the memory store must be fully loaded to match what's on disk. This is in line with principle 2, that we must have cache consistency, but this compromise also allows us to improve launch and Store creation performance in line with principle 3.The initializer will default to
.all
for simplicity (see principle 1), assuming that it should be sufficient for most use cases but giving people the ability to opt-out of immediate loading for larger Stores. Stefan's patch created a function calledrestorePersistedItems()
, and I think a more general name such assynchronize()
will be better. If you choose to opt out of all theitems
being loaded on Store creation it’ll be best to choose a good time for your app's needs to callsynchronize()
manually, otherwise it will automatically be called called on first access ofitems
..deferred(DispatchTimeInterval)
policy which lets you defer it by a certain amount of time, but this seems like it could create side effects — though on the other hand it's possible people may do this on their own by adding aTask.sleep
call beforesynchronize()
.4. Assess and integrate patch #12
I would like to integrate @StefanCosminR’s patch #12, thank you for all the work you put into it. I want to approach it with a bit of caution though as it’s a patch that has many changes, ones that I generally understand but can't speak too confidently about. I still have questions like whether the
Store
should be constrained to@MainActor
and whether to enforcingSendable
conformance is necessary (which we can discuss on the patch rather than here). To minimize potential side effects users may experience I'd like to merge the patch into a newconcurrency
branch, and make sure it integrates the documentation changes merged so far, along with the upcoming performance improvements noted above. Merging all these changes onto a separate branch will allow me and others to use the new code in a few apps before deciding on merging it into main. I understand you put some good work into this Stefan and I again want to thank you, hope this makes sense.5. Property wrapper for handling data/images
It seems intuitive to see Boutique and think of using it to store images, but unfortunately that can have real performance issues since those images can balloon the size of your app's memory. In fact it’s what @samalone completely reasonably attempted to do here, and I'd like to offer a good solution that promises performance, avoids code duplication, and is simple, leaning into principles 1 and 3.
I spent some time this morning prototyping two property wrappers
@PersistedItem
and@PersistedImage
, which can be used to create a value stored on disk, can be used directly in your models, but is never read into memory. Instead they would require astoragePath
URL and a keyPath that gets you the specific a piece of data on disk, which is then loaded by the property wrapper using Bodega’sDiskStorage
. This is just a rough draft, but the implementation I came up with looked like this. (I unfortunately can't pass in aStore
as a parameter because then@PersistedItem
won't beCodable
, and that's required since this property would exist on a model that's required to beCodable
to be stored in Boutique.)Out of all the work here this is the one that’s most nice-to-have and least essential, but I very much like the way this aids users in a common task, and helps them avoid a costly mistake. While Stuart found a good answer by using Bodega, it would be nice to offer this natively in Boutique.
Beta Was this translation helpful? Give feedback.
All reactions