Initial port of WinObjC's KVO implementation to GNUstep

[hmelder]

Summary:
This pull request introduces an initial port of the Key-Value Observing (KVO) implementation from Microsoft's WinObjC to GNUstep. The existing implementation in GNUstep has various issues (See #324).

Details:

• The KVO implementation has been ported from ObjC++ to ObjC.
• This port is specifically available for Clang with libobjc2.
• The existing KVO implementation is now marked as legacy and is maintained for the GCC toolchain.
• Test cases have been ported to Objective-C and integrated with the GNUstep test framework.

References:

• Original issue: Various issues with Key-Value Observing implementation #324

[triplef]

These changes are based on the patch we’ve been using in our app (see #324) and also contain additional fixes on top of the WinObjC implementation.

[rfm]

I would really have liked to upgrade the existing implementation with the few missing features rather than depending on objc2, but having looked at the code the dependency on the runtime library and the degree of optimisation (particularly the fine control of ARC) I can see why that would be difficult.

I'm not sure that the reworking of GSAtomic.h is necessary, but renaming functions with a gs_ prefix seems like a good thing even if it's not needed.

I think the C++ change in GSPThread.h should be removed though, since we aren't using C++ internally.

The rest looks reasonable, once we accept that we are tolerating the internal use of objc2 features (and minor coding style issues), though I can't say I'm familiar with the runtime library internals that the class swizzling depends upon.

[triplef]

I think adding the prefix for our atomic macros had been necessary to avoid name clashes with the stdatomic.h header.

All the extern "C" code should indeed be removed since none of the code is C++ (any more).

Maybe it would be a good idea to start a list with features that require libobjc2 e.g. in the readme (I think NSURLSession and this so far)?

[triplef]

We can probably remove all these @Status Interoperable comments from WinObjC.

[rfm]

On 12 Jun 2024, at 13:14, Frederik Seiffert ***@***.***> wrote: I think adding the prefix for our atomic macros had been necessary to avoid name clashes with the stdatomic.h header. All the extern "C" code should indeed be removed since none of the code is C++ (any more). Maybe it would be a good idea to start a list with features that require libobjc2 e.g. in the readme (I think NSURLSession and this so far)?

We really aren't supposed to have features that don't work with one compiler/runtime as logn as we are supporting both. It's inevitable that there are some differences between gnustep built on/for different systems, but portability is a key feature and we don't want major differences. While NSURLSession currently requires blocks, we are supposed to be getting rid of that dependency (using the pseudo-blocks we use elsewhere), and it shouldn't depend on any other objc2 features (or any libobjc2 stuff) AFAIK. For KVO, I understood the idea was to have two implementations so while one might perform better than the other, the feature-set is pretty similar either way.

[hmelder]

We really aren't supposed to have features that don't work with one compiler/runtime as logn as we are supporting both.

If one toolchain does not offer or restricts the capabilities needed, it should not slow down overall progress. Associated Objects (which are used in this KVO implementation) are an example for this.

[rfm]

On 12 Jun 2024, at 15:01, Hugo Melder ***@***.***> wrote: We really aren't supposed to have features that don't work with one compiler/runtime as logn as we are supporting both. If one toolchain does not offer or restricts the capabilities needed, it should not slow down overall progress. Associated Objects (which are used in this KVO implementation) are an example for this.

Well yes, a project with portability as a key feature does suffer from some slowdown. If something that's not generally available (like associated objects) is convenient for a task, you either spend a little time implementing a compatibility layer (there are already various compatibility features between the gnu and gnustep runtimes in gnustep-base), or do things a slightly different way in order to have portable code. That often takes a little longer but it means the project is usable by far more people. Ganerally I'd advocate adding general purpose compatibility features where sutiable features are available, rather than designing new ones. So probably implementing objc_setAssociatedObject() and objc_getAssociatedObject() for the gnu runtime would be better than implementing a private equivalent for KVO.

[rfm]

Or of course, as in this case, we can have two KVO implementations ... OK, but not as good from a maintainability point of view as a single implementation.

[hmelder]

Ganerally I'd advocate adding general purpose compatibility features where sutiable features are available, rather than designing new ones.

I understand your point, but even for NSURLSession, this would mean using the libdispatch APIs with C functions, adding autorelease pools to each C function, and using methods instead of blocks for delegate dispatching.

Quite a pain and increase in complexity...
I'd be willing to review the changes tho :)

[hmelder]

On macOS, all change messages are sent after the change happened:

* thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1
  * frame #0: 0x00000001000012bc a.out`-[Observer observeValueForKeyPath:ofObject:change:context:](self=0x0000600001514fe0, _cmd="observeValueForKeyPath:ofObject:change:context:", keyPath=@"Test Suite Bool", object=0x0000600001b143c0, change=0x0000600000015740, context=0x0000000000000000) at test.m:42:13
    frame #1: 0x00000001883bacf4 Foundation`NSKeyValueNotifyObserver + 252
    frame #2: 0x00000001883bca78 Foundation`NSKeyValueWillChange + 452
    frame #3: 0x00000001883add44 Foundation`-[NSObject(NSKeyValueObservingPrivate) _changeValueForKeys:count:maybeOldValuesDict:maybeNewValuesDict:usingBlock:] + 468
    frame #4: 0x00000001883bc484 Foundation`-[NSObject(NSKeyValueObservingPrivate) _notifyObserversOfChangeFromValuesForKeys:toValuesForKeys:] + 652
    frame #5: 0x0000000187273cd4 CoreFoundation`-[CFPrefsSource forEachObserver:] + 320
    frame #6: 0x00000001872b4198 CoreFoundation`-[CFPrefsSource _notifyObserversOfChangeFromValuesForKeys:toValuesForKeys:] + 112
    frame #7: 0x00000001872b4004 CoreFoundation`___CFPrefsDeliverPendingKVONotificationsGuts_block_invoke + 432
    frame #8: 0x00000001872b3e48 CoreFoundation`__CFDictionaryApplyFunction_block_invoke + 28
    frame #9: 0x0000000187278bd0 CoreFoundation`CFBasicHashApply + 148
    frame #10: 0x000000018726bcec CoreFoundation`CFDictionaryApplyFunction + 224
    frame #11: 0x00000001872b3dbc CoreFoundation`_CFPrefsDeliverPendingKVONotificationsGuts + 300
    frame #12: 0x000000018725d2cc CoreFoundation`-[_CFXPreferences _deliverPendingKVONotifications] + 96
    frame #13: 0x000000018725dd4c CoreFoundation`__108-[_CFXPreferences(SearchListAdditions) withSearchListForIdentifier:container:cloudConfigurationURL:perform:]_block_invoke + 404
    frame #14: 0x00000001873e17c8 CoreFoundation`-[_CFXPreferences withSearchListForIdentifier:container:cloudConfigurationURL:perform:] + 440
    frame #15: 0x00000001872b8af8 CoreFoundation`-[_CFXPreferences setValue:forKey:appIdentifier:container:configurationURL:] + 124
    frame #16: 0x00000001872b8a3c CoreFoundation`_CFPreferencesSetAppValueWithContainerAndConfiguration + 120
    frame #17: 0x00000001883f2e10 Foundation`-[NSUserDefaults(NSUserDefaults) setObject:forKey:] + 68
    frame #18: 0x000000010000271c a.out`main at test.m:298:3
    frame #19: 0x0000000186e4a0e0 dyld`start + 2360
    ```

[hmelder]

This avoids repeated locking of NSUserDefaults to retrieve the values.

[hmelder]

I need to test the changes made to the legacy KVO implementation.

[hmelder]

The Signature Cache in NSMethodSignature results in up to 36% speed-up of KVO willChange/didChange dispatching.

@rfm can you review the changes to NSMethodSignature, NSKeyValueObserving, and other existing classes?

[hmelder]

We can shave of another 100-150ms if we reuse change dictionaries. KVO on NSSet's is also really inefficient atm because of a copy operation inside willChange[...] and computation of the changes.

[rfm]

I actually looked at the signature cache code yesterday, and was thinking I should suggest that you commit that separately since it's not actually linked to the KVO code in any way, and it seemed a very clearly good idea!

[rfm]

We can shave of another 100-150ms if we reuse change dictionaries.
This we need to check carefully (more than I've had chance to). Re-use makes sense but last version I looked at, I thought the change dictionaries were mutable, and re-use would be bad if user code can alter the dictionary. From my cursory review I wasn't sure it would be safe.

[triplef]

Yes good point @rfm, plus we need to ensure multi-thread support is maintained (see also my comment on the static NSNumber above).

[hmelder]

we need to ensure multi-thread support is maintained

It is probably worth then to write a few test cases...

[hmelder]

(see also my comment on the static NSNumber above).

I cannot see it on GH :(

[fredkiefer]

Could we try to get the tests working with the old ObjC runtime as well? Of course we need to flag some tests as hopeful there. But that way we better see the missing functionality.
i am willing to help with the rewrite if we can agree on it.

[hmelder]

Could we try to get the tests working with the old ObjC runtime as well?

Sure!

[triplef]

I rebased onto master to fix some merge conflicts.

[hmelder]

@rfm what's the status of this PR? Can we merge it, or are there still some tests that need to be refactored for older versions of ObjC?

[rfm]

You were going to address Fred's request for portable testcases. I have done partial fixes recently but afaik there's still quite a bit of broken (ie dependent on Apple/clang specific features) code there.

[fredkiefer]

I can also help in that process. Mostly it will be rewriting ObjC2 code the old way and flagging tests as hopeful that fail.