[KSCRASH] Add Observers to Memory Tracker #681
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
7e19ff0 to
0957613
Compare
e2e0328 to
6e79526
Compare
|
@kstenerud I've moved our whole observer pattern to blocks. |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the memory tracker to follow a singleton pattern with an observer-based architecture, bringing it in line with the app state tracker. The changes enable multiple observers to watch memory changes while maintaining a single instance per process.
Key changes:
- Added a shared singleton instance with observer pattern support
- Replaced delegate-based callbacks with block-based observers
- Updated internal memory monitor to use the shared instance
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| KSCrashAppMemoryTracker.h | Added shared instance property, observer block typedef, and deprecated delegate property |
| KSCrashMonitor_Memory.m | Updated to use shared instance and observer pattern instead of creating own tracker |
| KSCrashAppMemoryTracker.m | Implemented singleton pattern, observer management, and updated memory change handling |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| /** | ||
| Deprecated | ||
| */ | ||
| @property(nonatomic, weak) id<KSCrashAppMemoryTrackerDelegate> delegate DEPRECATED_ATTRIBUTE; |
There was a problem hiding this comment.
@GLinnik21 the delegate was moved here and deprecated.
There was a problem hiding this comment.
Not at all, better safe than sorry.
There was a problem hiding this comment.
I noticed that in other places we use __attribute__((deprecated)) with a message explaining the reason and version. It might make sense to keep it consistent across the codebase, instead of mixing it with DEPRECATED_ATTRIBUTE. Either way, adding a clear deprecation message (and a Doxygen @deprecated tag) would be helpful.
a0f95af to
1eee817
Compare
e3c3489 to
bbd5b63
Compare
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
bbd5b63 to
2e38d80
Compare
| /** | ||
| Deprecated | ||
| */ | ||
| @property(nonatomic, weak) id<KSCrashAppMemoryTrackerDelegate> delegate DEPRECATED_ATTRIBUTE; |
There was a problem hiding this comment.
I noticed that in other places we use __attribute__((deprecated)) with a message explaining the reason and version. It might make sense to keep it consistent across the codebase, instead of mixing it with DEPRECATED_ATTRIBUTE. Either way, adding a clear deprecation message (and a Doxygen @deprecated tag) would be helpful.
|
|
||
| /** Deprecated delegate */ | ||
| NS_SWIFT_NAME(AppMemoryTrackerDelegate) | ||
| @protocol KSCrashAppMemoryTrackerDelegate <NSObject> |
There was a problem hiding this comment.
This can be deprecated as well with __attribute__((deprecated(“Message. Since 2.4.0"))) with corresponding docs
| /** | ||
| * Start/Stop | ||
| * | ||
| * WARNING: Don't call these on the shared tracker. | ||
| */ | ||
| - (void)start; | ||
| - (void)stop; |
There was a problem hiding this comment.
It’s better to document these methods separately and highlight the caveat with Doxygen’s @warning tag.
There was a problem hiding this comment.
Moved them to the implementation as they're not useful as public API's.
There was a problem hiding this comment.
Are these new methods in API?
There was a problem hiding this comment.
Crap, I was sure they weren't but yeah, they are, I'll add them back in. Good thing you've got your eye on us :)
The memory tracker is really meant as a single instance per process thing, having more than one is a waste of resources. This brings the memory tracker on par with the app state tracker by allowing multiple observers to be added to the singleton, as well as uses the singleton internally.