gnustep
Initial port of WinObjC's KVO implementation to GNUstep
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: #324
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.
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)?
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.
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.
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.
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.
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 :)
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?
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.
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!
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.
Yes good point @rfm, plus we need to ensure multi-thread support is maintained (see also my comment on the static NSNumber above).
we need to ensure multi-thread support is maintained
It is probably worth then to write a few test cases...
(see also my comment on the static NSNumber above).
I cannot see it on GH :(
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.
Could we try to get the tests working with the old ObjC runtime as well?
Sure!
@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?
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.
I can also help in that process. Mostly it will be rewriting ObjC2 code the old way and flagging tests as hopeful that fail.
Converting kvoToMany.m to legacy ObjC is going to be difficult without block support, and would require an extensive rewrite. Otherwise, all tests now run on GCC.
I suggest leaving kvoToMany as is and not running in when __OBJC2__ is not defined.
I'm on OpenBSD amd64, with latest GNUstep release, libobjc2 and ARC. And I was just in need for: +keyPathsForValuesAffectingValueForKey: for my application.
I can't comment on the code and what you're doing here, but providing some feedback: So I took a recent git snapshot of gnustep-base I had around, and applied this MR.
In my little test app: https://github.com/buzzdeee/TestObserver
I can now use:
- (NSSet *)keyPathsForValuesAffectingIsActiveSpell
and the observing works as expected. but trying to use the more general: +(NSSet *)keyPathsForValuesAffectingValueForKey: (NSString *)key
doesn't work. Just swich commenting out the two methods here: https://github.com/buzzdeee/TestObserver/blob/main/DSASpell.m
and see the difference.
Saying that, it's a great step forward, but the keyPathsForValuesAffectingValueForKey: method would also be awesome to have ;)
doesn't work. Just swich commenting out the two methods here: https://github.com/buzzdeee/TestObserver/blob/main/DSASpell.m
Fixed in https://github.com/gnustep/libs-base/pull/420/commits/bf4091684f54b91e658bd5ce55b2ec0f816eec11
doesn't work. Just swich commenting out the two methods here: https://github.com/buzzdeee/TestObserver/blob/main/DSASpell.m
Fixed in bf40916
Just tested this last patch version, in my main app, where I have the keyPathsForValuesAffectingValueForKey: method already prepared, suddenly started to work with that. Awesome!
Seems like the NSURLSession tests fail sometimes because of https://github.com/apple/swift-corelibs-libdispatch/issues/833
This is unrelated.