-
Notifications
You must be signed in to change notification settings - Fork 71
Address the slow growth of memory utilization with perf #1000
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,6 +37,8 @@ class PerfProcess: | |
| # default number of pages used by "perf record" when perf_event_mlock_kb=516 | ||
| # we use double for dwarf. | ||
| _MMAP_SIZES = {"fp": 129, "dwarf": 257} | ||
| _RSS_GROWTH_THRESHOLD = 100 * 1024 * 1024 # 100MB in bytes | ||
| _BASELINE_COLLECTION_COUNT = 3 # Number of function calls to collect RSS before setting baseline | ||
|
|
||
| def __init__( | ||
| self, | ||
|
|
@@ -65,6 +67,8 @@ def __init__( | |
| self._extra_args = extra_args | ||
| self._switch_timeout_s = switch_timeout_s | ||
| self._process: Optional[Popen] = None | ||
| self._baseline_rss: Optional[int] = None | ||
| self._collected_rss_values: List[int] = [] | ||
|
|
||
| @property | ||
| def _log_name(self) -> str: | ||
|
|
@@ -132,6 +136,7 @@ def is_running(self) -> bool: | |
|
|
||
| def restart(self) -> None: | ||
| self.stop() | ||
| self._clear_baseline_data() | ||
| self.start() | ||
|
|
||
| def restart_if_not_running(self) -> None: | ||
|
|
@@ -145,18 +150,50 @@ def restart_if_not_running(self) -> None: | |
| def restart_if_rss_exceeded(self) -> None: | ||
| """Checks if perf used memory exceeds threshold, and if it does, restarts perf""" | ||
| assert self._process is not None | ||
| perf_rss = Process(self._process.pid).memory_info().rss | ||
| if ( | ||
| time.monotonic() - self._start_time >= self._RESTART_AFTER_S | ||
| and perf_rss >= self._PERF_MEMORY_USAGE_THRESHOLD | ||
| ): | ||
| current_rss = Process(self._process.pid).memory_info().rss | ||
|
|
||
| # Collect RSS readings for baseline calculation | ||
| if self._baseline_rss is None: | ||
| self._collected_rss_values.append(current_rss) | ||
|
|
||
| if len(self._collected_rss_values) < self._BASELINE_COLLECTION_COUNT: | ||
| return # Still collecting, don't check thresholds yet | ||
|
|
||
| # Calculate average from collected samples | ||
| self._baseline_rss = sum(self._collected_rss_values) // len(self._collected_rss_values) | ||
| logger.debug( | ||
| f"RSS baseline established for {self._log_name}", | ||
| collected_samples=self._collected_rss_values, | ||
| calculated_baseline=self._baseline_rss, | ||
| ) | ||
|
|
||
| # Now check memory thresholds with established baseline | ||
| memory_growth = current_rss - self._baseline_rss | ||
| time_elapsed = time.monotonic() - self._start_time | ||
|
|
||
| should_restart_time_based = ( | ||
| time_elapsed >= self._RESTART_AFTER_S and current_rss >= self._PERF_MEMORY_USAGE_THRESHOLD | ||
| ) | ||
| should_restart_growth_based = memory_growth > self._RSS_GROWTH_THRESHOLD | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thanks, looks good
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The system I tested is an AWS instance with small number of cpus. Therefore, our tests do not represent your use case. On idle system I do see around 50MB as baseline RSS and 1~2MB growth every duration. I tried to run more cpu intensive workloads but it seems not increase the rss much because the system configuration has limited memory and cpus. So, it would be good if you can evaluate this change on your side with real use cases. Can you try that?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, this would need testing in environments where there are lot of processes ( over 1k-1.5k) running. I would suggest we can start testing the root cause fix here #1002 and then test this fix. |
||
|
|
||
| if should_restart_time_based or should_restart_growth_based: | ||
| restart_cause = "time+memory limits" if should_restart_time_based else "memory growth" | ||
| logger.debug( | ||
| f"Restarting {self._log_name} due to memory exceeding limit", | ||
| limit_rss=self._PERF_MEMORY_USAGE_THRESHOLD, | ||
| perf_rss=perf_rss, | ||
| f"Restarting {self._log_name} due to {restart_cause}", | ||
| current_rss=current_rss, | ||
| baseline_rss=self._baseline_rss, | ||
| memory_growth=memory_growth, | ||
| time_elapsed=time_elapsed, | ||
| threshold_limit=self._PERF_MEMORY_USAGE_THRESHOLD, | ||
| ) | ||
| self._clear_baseline_data() | ||
| self.restart() | ||
|
|
||
| def _clear_baseline_data(self) -> None: | ||
| """Reset baseline tracking for next process instance""" | ||
| self._baseline_rss = None | ||
| self._collected_rss_values = [] | ||
|
|
||
| def switch_output(self) -> None: | ||
| assert self._process is not None, "profiling not started!" | ||
| # clean stale files (can be emitted by perf timing out and switching output file). | ||
|
|
@@ -191,6 +228,17 @@ def wait_and_script(self) -> str: | |
| # (unlike Popen.communicate()) | ||
| if self._process is not None and self._process.stderr is not None: | ||
| logger.debug(f"{self._log_name} run output", perf_stderr=self._process.stderr.read1()) # type: ignore | ||
| # Safely drain stdout buffer without interfering with error handling | ||
| if self._process is not None and self._process.stdout is not None: | ||
| try: | ||
| # Use read1() to avoid blocking, but don't necessarily log it | ||
| stdout_data = self._process.stdout.read1() # type: ignore | ||
| # Only log if there's unexpected stdout data (diagnostic value) | ||
| if stdout_data: | ||
| logger.debug(f"{self._log_name} unexpected stdout", perf_stdout=stdout_data) | ||
| except (OSError, IOError): | ||
| # Handle case where stdout is already closed/broken | ||
| pass | ||
|
|
||
| try: | ||
| inject_data = Path(f"{str(perf_data)}.inject") | ||
|
|
||
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.
thanks, i would suggest having some data backed to configure this collection count baseline