-
Notifications
You must be signed in to change notification settings - Fork 279
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Initial port of WinObjC's KVO implementation to GNUstep #420
base: master
Are you sure you want to change the base?
Conversation
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
I think adding the prefix for our atomic macros had been necessary to avoid name clashes with the stdatomic.h header. All the 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)? |
Source/NSKVOSupport.m
Outdated
NSObject (NSKeyValueObserving) | ||
/** | ||
@Status Interoperable | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can probably remove all these @Status Interoperable
comments from WinObjC.
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.
|
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. |
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... |
if ([pd setObject: value forKey: defaultName]) | ||
{ | ||
[self _changePersistentDomain: processName]; | ||
[self _notifyObserversOfChangeForKey: defaultName oldValue:old newValue:value]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
```
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This avoids repeated locking of NSUserDefaults to retrieve the values.
I need to test the changes made to the legacy KVO implementation. |
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! |
|
Yes good point @rfm, plus we need to ensure multi-thread support is maintained (see also my comment on the |
It is probably worth then to write a few test cases... |
I cannot see it on GH :( |
1eebad8
to
dbe8f19
Compare
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. |
Sure! |
b0743eb
to
64b4f0e
Compare
I rebased onto master to fix some merge conflicts. |
@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? |
…set on set mutation
…pile-time constant
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. |
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:
References: