Skip to content
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

RUMM-1274 Implement Memory reader #495

Merged
merged 1 commit into from
Jun 2, 2021

Conversation

xgouchet
Copy link
Member

What and why?

Implement a class to read the Resident Memory used by the current application

How?

This uses the task_vm_info struct to read the relevant memory information

Review checklist

  • Feature or bugfix MUST have appropriate tests (unit, integration)
  • Make sure each commit and the PR mention the Issue number or JIRA reference

@xgouchet xgouchet requested a review from a team as a code owner May 21, 2021 12:30
);
path = RUMVitals;
sourceTree = "<group>";
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is sth wrong around this lines and it malforms the pbxproj file:

--- xcodebuild: WARNING: Unable to open project file '/Users/vagrant/git/Datadog/Datadog.xcodeproj' in workspace '/Users/vagrant/git/Datadog.xcworkspace'.

You may want to reset changes in pbxproj, do rebase and then add Vital* files once again.

import Foundation

internal class VitalMemoryReader: VitalReader {
static let TASK_VM_INFO_COUNT = MemoryLayout<task_vm_info>.size / MemoryLayout<natural_t>.size
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't use uppercase for defining constants - they are reserved for compiler directives. Also, seems that this one can be private:

private static let task_vm_info_count = MemoryLayout<task_vm_info>.size / MemoryLayout<natural_t>.size

@testable import Datadog

internal class VitalMemoryReaderTest: XCTestCase {
/// This is definitely not an ideal test, but I'm out of my depth on how to mock the `task_info` method
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about testing its property?

func testWhenMemoryConsumptionGrows() {
   // Given
   let reader = VitalMemoryReader()
   threshold = reader.readVitalData()

   // When
   allocateHeapMemory(size: 100 * 1_024)

   // Then
   measure = reader.readVitalData()
   XCTAssertGreatherThan(measure, threshold)
}

func testWhenMemoryConsumptionDecreases() {
   // ...
   XCTAssertGreatherThan(threshold, measure)
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great idea

@xgouchet xgouchet force-pushed the xgouchet/RUMM-1274/track_memory_usage branch from 705476c to 518b0c9 Compare May 26, 2021 13:43
@xgouchet xgouchet requested review from ncreated and buranmert May 26, 2021 13:43
Copy link
Member

@ncreated ncreated left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to go 🚀👌.

for i in 0..<allocatedSize {
(intPointer + i).initialize(to: i)
}
let measure = reader.readVitalData()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way I read from the docs about the UnsafeMutablePointer the capacity should be the number of memory blocks specific for that type. The way I understand this is that the memory consumed should be allocatedSize * 32, isn't this correct ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's correct. delta should be >= allocatedSize * MemoryLayout<Int>.stride where MemoryLayout<Int>.stride is 8. Int is a typalias to Int64 in 64bit systems and 64bit = 8 bytes.

the assertion below should be XCTAssertGreaterThanOrEqual(delta, Double(allocatedSize) * 8).
but this way doesn't work in testWhenMemoryConsumptionShrinks case below. i run it in my local and delta is smaller than deallocated memory.
can we test if delta is positive or negative (resident_size increases or decreases) only?

@xgouchet xgouchet force-pushed the xgouchet/RUMM-1274/track_memory_usage branch from 518b0c9 to f061e69 Compare June 1, 2021 10:36
@xgouchet xgouchet merged commit 999358a into master Jun 2, 2021
@xgouchet xgouchet deleted the xgouchet/RUMM-1274/track_memory_usage branch June 2, 2021 07:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants