This differential introduces memory event monitoring in NSE. If critical memory event occurs we are displaying notification with current memory usage. events occurs per process and there is a proper synchronization for the memory message holder.
Details
- Reviewers
tomek bartek ashoat - Commits
- rCOMMb0277def662b: Introduce memory monitoring in NSE
I implemented a small function that allocates more and more bytes (200000 unsigned long) to a static array upon each notification arrival. I observed that I see memory usage notification twice since the static array is first allocated (new NSE process starts). After the second time the process was killed - I didn't see "Cleaning-up extension" log after notification display. Additionally I was logging allocated array size after each notification and after the second memory usage notification array size dropped to 200000 which means it was freshly allocated so new process was started.
I also tested whether entire notification functionality (normal notifs, rescinds and badgeonly notifs) work correctly without using memory-leak simulation function.
I tested everything on a release build connected to my local keyserver.
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Thanks for breaking this out into a separate diff!
My time is rather limited right now, and to properly review this diff I feel like I'd need to review some Apple docs. Given that, I'm going to resign – hoping that one of the other reviewers can take a close look
Separately: if this code was inspired by another implementation, it might be helpful to the reviewers to link the source code for that implementation here
native/ios/NotificationService/NotificationService.mm | ||
---|---|---|
24–27 ↗ | (On Diff #30212) | What's the meaning of count? |
34–35 ↗ | (On Diff #30212) | We can divide it, but I don't think it's necessary (I would prefer not to do that so that our results are more precise and could be used to compare measurements more accurately). But what we should do is to inform the caller about units we're returning - if they are bytes or megabytes (in a function name). |
367–381 ↗ | (On Diff #30212) |
|
387 ↗ | (On Diff #30212) | Does that mean we're listening only for critical evets? Won't that be beneficial to listen for more? |
393–394 ↗ | (On Diff #30212) | We should include info about what unit was used. |
409 ↗ | (On Diff #30212) | Should we somehow deactivate later? |
native/ios/NotificationService/NotificationService.mm | ||
---|---|---|
24–27 ↗ | (On Diff #30212) | TASK_VM_INFO_COUNT is the size of memory taken by task_vm_info_data_t structure. This method casts task_vm_info_data_t to task_info_t so it has to know how much memory the original structure takes. Docs for task_info function: https://www.gnu.org/software/hurd/gnumach-doc/Task-Information.html |
367–381 ↗ | (On Diff #30212) |
When memory events occurs event handler uses this method to set memory error message. Then NSE instance checks before calling content handler if the memory error message was set. If it was it atomically gets it and sets to null. Then it uses the memory error message to modify notification content (for staff users only).
Assuming that some error occurs that prevents unlock from being called but doesn't kill the process then we just stop displaying memory event notifications but other NSE functionalities remain unchanged. |
387 ↗ | (On Diff #30212) | There are three events possible:
We could listen to WARNING but I am afraid we would be getting to much events and didn't have a clear picture of what is going on. |
409 ↗ | (On Diff #30212) | Unfortunately there is no way to tell when NSE process is going to terminate - there is not callback we can overwrite. Alternatively we could create and delete memory source at the beginning and at the end of didReceive... callback. However there are disadvantages. The first is that we would have to synchronise it so that only one NSE instance creates and deletes memory source at a time (it is a per process variable). The other is that we would miss events that occur in between didReceive... callbacks. Signal keeps a global instance of memoruSource and they don't call any suspension or deactivation/cancellation methods: https://github.com/signalapp/Signal-iOS/blob/6cc18788850558251db5f772ca62bce6b576dfc4/SignalNSE/NSEContext.swift |
native/ios/NotificationService/NotificationService.mm | ||
---|---|---|
376–379 ↗ | (On Diff #30257) | Do we have to copy twice? I guess that if we're making the copy while assigning to memoryEventMessage, we can then return this value without copying it the second time. |
367–381 ↗ | (On Diff #30212) | Thanks! That makes sense |
387 ↗ | (On Diff #30212) | Ok, makes sense |
native/ios/NotificationService/NotificationService.mm | ||
---|---|---|
376–379 ↗ | (On Diff #30257) | I don't think that any value is copied here twice. The first time we copy the current value of memoryEventMessage (or not if it is nil). Then we overwrite memoryEventMessage with the copy of it new value (provided as a method argument). Then we return the old value. This is basically getAndSetAtomically that atomically reads old value, writes the new one and returns the old one. No value is copied twice. |