Let a thread reuse the defaults it has already read

[DTW-Thalion]

objectForKey: takes the instance lock and walks the search list on every call, so every read of a default serialises on one lock whatever key it asks for.

A thread now keeps the last few answers it was given and uses one again while nothing has changed the defaults. Every change increments a counter under the lock, and an entry stamped with an earlier value is not used, so a read that hits takes no lock and does not walk the list.

ns per objectForKey:, each thread on its own key, 32 cores, median of 3 interleaved runs: 1 thread 500.6 to 41.2, 8 threads 24270.0 to 46.0, 24 threads 86965.4 to 80.2. On one shared key 24 threads is 81016.1 to 2167.0, the retain and release of that value.

The whole suite gives 13466 passed and 45 dashed hopes before and after. A probe with 16 readers and a writer checks 26M reads with no mismatch.

The counter is global, so a change to any defaults object costs every thread its entries.

[rfm]

This is quite nice/clever, but I'm not convinced it would be of much benefit for real world code.

Typically defaults are used in two situations, and this change would speed things a little in the case where we don't care about performance and simply repeatedly ask for the same key/keys (which sometimes happens).
The other case, where we care about performance, is when we register code to receive a notification when the defaults have changed. The code then caches the values for the keys it is interested in until the next time defaults change.
So what this patch implements is a limited version of the caching that performance sensitive code will already be doing, and because it's a general purpose design it is likely, in most cases, to be less efficient than existing dedicated caching.
My gut feeling is that this won't significantly harm performance critical code (because defaults updates are rare, so any overheads will only occur on the rare occasions that happens). It will clearly slow the common case where we look up a key only once, but the overhead on a single lookup is so small as to be unimportant.

So I'm curious where you saw code that was repeatedly looking at the same default values rather than using the notification mechanism to cache them?

[rfm]

Or do you have a specific use case in mind? Like creating huge numbers of programs which need config from the defaults system, where that config is likely to be updated frequently, and because you have so many programs you don't want the overhead of writing dedicated notification handling / config caching in each?

[DTW-Thalion]

Nothing so well thought out :) I am simply doing performance runs across the libs-base classes using various workloads I have available, and wherever I see something that appears to be a candidate for fixes under multi-threaded loads, I flag it. That is why I have been making these all Draft PRs to start - some may be worth fixing, some not, or they may lead us towards another architectural approach.

Targets are KVC cache, NSMethodSignature, NSBundle, NSUserDefaults, NSNotificationCenter, then a couple things I found in libobjc2.

I have found a defect in objc_setProperty which I have to fix still, and NSNotificationCenter will need a design rework to reach the potential (ceiling) gains I am seeing in my tests.

[rfm]

One other thing; GNUstep posts an NSUserDefaultsDidChangeNotification whenever defaults change.
However it does post for some internal changes changes (volatile domains and search list) which would alter the map.
The current Apple documentation says that OSX is different again, and doesn't post the notification when there are changes made by a different app. It says you should use KVO to monitor that, but this must be a change from the original OpenStep code which didn't have KVO.
Now, I don't think we want to mimic OSX in reducing the circumstances where the notification is posted, because a whole lot of code depends on the original implementation where, if a synchronise results in a change being loaded from disk, the notification is posted. What I think we may want to do instead is actually expand it to work as a reasonable reader would expect: so we post the notification for any change (or group of changes when loading from disk) which alter the key->value mapping. If so, your defaultsDidChange() function might occur wherever we post the notification (though perhaps we might make the GSMacOSXCompatible setting control that).

There's an internal call to the updateCache() function which is supposed to be called whenever the defaults mapping actually changes, so I think your defaultsDidChange() is supposed to be called in the same places as that, so probably either updateCache() is not called everywhere it should be, or defaultsDidChange() is being called more than it should. I think the two should be combined.

[DTW-Thalion]

On combining defaultsDidChange() with updateCache(), I do not think one call can do both jobs.

updateCache() recomputes the process settings derived from the shared defaults, the GNU-Debug levels and the flags array, and returns at once unless self is sharedDefaults. In _changePersistentDomain: and the two volatile domain methods it is further conditional on the domain being in the search list. The generation counter has to move whenever any instance's mapping changes, so its call sites are a superset.

Where the two can share a site they already do, 9 methods, adjacent lines. The ones that cannot are in GSPersistentDomain. setObject:forKey: writes the domain, reads the value back through objectForKey: for the KVO change dictionary, and only then calls _changePersistentDomain:. With the increment only at that outer call the read returns the pre-change value, so new in every KVO notification is wrong, 8 failures in NSKVOSupport. removeObjectForKey: makes those two calls the other way round, which is why it looks correct.

On expanding the notification, the methods which alter the key->value mapping and post nothing today are registerDefaults:, setVolatileDomain:forName:, removeVolatileDomainForName:, addSuiteNamed:, removeSuiteNamed: and setSearchList:. I have a change adding a post to each, in the shape _changePersistentDomain: already uses, a flag set under the lock and the post after unlocking, with a test that fails on master for all 6. setSearchList: keeps its existing guard so an equal list still posts nothing; the rest post unconditionally, matching the note in setObject:forKey: about notifying even where the value has not changed.

Before I take it further: should +resetStandardUserDefaults post as well, since it reinstalls the registration domain on the new instance? And was GSMacOSXCompatible meant to control the extra posts, or the combining?

[rfm]

I thought I replied to the last comment ages ago, but apparently I forgot to hit send or something.

Yes, I agree that it would make sense for +resetStandardUserDefaults to post the notification too.

The GSMacOSXCompatible setting is more nuanced. The design of this is to allow someone to specify whether they want to maximise OSX compatibility (and if so, define which version of OSX). The motivation is to cope with differences of opinion: Often we simply follow OSX behavior, but sometimes those behaviors are wrong or just seem less sensible than behaviors we have in GNUstep so we don't want to slavishly copy them. However, someone taking an app developed on OSX and wanting to port it to run on other platforms may not want to change their source code to avoid depending on OSX (mis)behavior that's not normally present in GNUstep. In those cases they can contribute patches to mimic the OSX behavior gated on this setting, leaving their own app/apps unchanged.

So you are not compelled to use it to make the notification posting behavior configurable: generally I'd expect that sort of configurability to be implemented only if it turns out that real users need it.

[rfm]

Yes, I agree that it would make sense for +resetStandardUserDefaults to post the notification too.

But I just realised I was wrong about wanting to post that notification for all defaults changed.
The problem is with code where a notification observer wants to adjust/correct defaults settings. If it does that then the adjustment it makes will cause the notification to be sent again and we can get recursion. So we need to be very careful to only post a notification if the defaults actually changed.

[rfm]

In fact I'm now thinking that perhaps to avoid recursion issues and excessive posting, most alterations should perhaps use a notification queue with coalescing so that a single notification is posted (at the start of the next run loop iteration) for all changes made. perhaps with an exception that changes from an explicit call to synchronise would be immediate.