Page MenuHomePhabricator

Introduce memory monitoring in NSE
ClosedPublic

Authored by marcin on Aug 22 2023, 3:59 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Nov 14, 11:35 PM
Unknown Object (File)
Sun, Oct 27, 11:11 PM
Unknown Object (File)
Oct 18 2024, 3:06 AM
Unknown Object (File)
Oct 18 2024, 3:06 AM
Unknown Object (File)
Oct 18 2024, 3:06 AM
Unknown Object (File)
Oct 18 2024, 3:06 AM
Unknown Object (File)
Oct 18 2024, 3:06 AM
Unknown Object (File)
Oct 18 2024, 3:06 AM
Subscribers
None

Details

Summary

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.

Test Plan

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
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Rebase to update commit message

Harbormaster returned this revision to the author for changes because remote builds failed.Aug 22 2023, 4:28 AM
Harbormaster failed remote builds in B21999: Diff 30208!

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

tomek requested changes to this revision.Aug 22 2023, 11:12 AM
tomek added inline comments.
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)
  1. What's the purpose of this method?
  2. Why do we use NSLock instead of @synchronized?
  3. Is it guaranteed that unlock is always called?
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?

This revision now requires changes to proceed.Aug 22 2023, 11:12 AM
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
Definition for TASK_VM_INFO_COUNT (code): https://github.com/xybp888/iOS-SDKs/blob/master/iPhoneOS13.0.sdk/usr/include/mach/task_info.h#L406

367–381 ↗(On Diff #30212)
  1. The purpose of this method is explained in the summary.

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).

  1. NSLock has tryLock method which is non-blocking. But perhaps it is a premature optimization? I didn't want to block every thread of NSE process when calling this method. This is different from the case in child differential where we synchronize access to instance variable since it is a class variable set per entire process.
  2. It might not be called if an exception is thrown in the middle of its execution provided that this error does not kill the process. Unless we use @synchronized we don't have a way to guarantee that unlock will be called however it is unlikely that it won't be. I decided that it is better not to block every thread when calling this method than guarantee 100% sure that unlock will be called.

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:

  • NORMAL
  • WARNING
  • CRITICAL

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.
There is a method dispatch_suspend: https://developer.apple.com/documentation/dispatch/1452801-dispatch_suspend, but the docs advice that the memory source must be in resumed state before it is dealocated. That said if we were to suspend it we would have to ensure it is resumed befre NSE terminates which we can't.

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

tomek added inline comments.
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

This revision is now accepted and ready to land.Aug 24 2023, 7:34 AM
  1. Refactor to reflect changes from parent differential.

Rebase to reflect changes in parent differential

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.

This revision was automatically updated to reflect the committed changes.