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

feat: add --track-peak-rss to daemon #2696

Merged
merged 7 commits into from
Mar 23, 2023
Merged

feat: add --track-peak-rss to daemon #2696

merged 7 commits into from
Mar 23, 2023

Conversation

hanabi1224
Copy link
Contributor

@hanabi1224 hanabi1224 commented Mar 22, 2023

Summary of changes

Changes introduced in this pull request:

RSS is included in Prometheus metrics but it's still helpful to track and print it when forest is not in long-running mode (e.g. --halt-after-import), so that we could easily get peak RSS usage from log when running forest --import-snapshot [path] --halt-after-snapshot --track-peak-rss without using an external monitoring script, to fill in benchmark tables

Reference issue to close (if applicable)

Closes

Other information and links

Change checklist

  • I have performed a self-review of my own code,
  • I have made corresponding changes to the documentation,
  • I have added tests that prove my fix is effective or that my feature works
    (if possible),
  • I have made sure the
    CHANGELOG is
    up-to-date. All user-facing changes should be reflected in this document.

None
};
if let Some(mem_stats_tracker) = &mem_stats_tracker {
mem_stats_tracker.run_async();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this background task be added to the services?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but that will make its usage a bit complicated for other tools, e.g. statediff, forest-cli

Copy link
Contributor

Choose a reason for hiding this comment

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

How about exposing the async loop and then letting the caller spawn a task? Or maybe just use std::thread::spawn.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! Now I won't accidentally run the function from a non-async context. :)

Copy link
Member

@LesnyRumcajs LesnyRumcajs left a comment

Choose a reason for hiding this comment

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

I guess this may be helpful to track the daemonized process, but in general, for benchmark, I'd use /usr/bin/time -v. Application self-reporting memory metrics of itself is shady. I'm okay with merging this, though.

        Command being timed: "target/release/forest --chain calibnet"
        User time (seconds): 1.18
        System time (seconds): 0.57
        Percent of CPU this job got: 8%
        Elapsed (wall clock) time (h:mm:ss or m:ss): 0:19.70
        Average shared text size (kbytes): 0
        Average unshared data size (kbytes): 0
        Average stack size (kbytes): 0
        Average total size (kbytes): 0
        Maximum resident set size (kbytes): 120804
        Average resident set size (kbytes): 0
        Major (requiring I/O) page faults: 32
        Minor (reclaiming a frame) page faults: 36876
        Voluntary context switches: 16040
        Involuntary context switches: 347
        Swaps: 0
        File system inputs: 900872
        File system outputs: 89856
        Socket messages sent: 0
        Socket messages received: 0
        Signals delivered: 0
        Page size (bytes): 4096
        Exit status: 0

@hanabi1224
Copy link
Contributor Author

@LesnyRumcajs Thanks! That's a much better option, I kinda confused /usr/bin/time with time which does not support -v : )

@hanabi1224 hanabi1224 enabled auto-merge (squash) March 23, 2023 08:15
@hanabi1224 hanabi1224 merged commit e499495 into main Mar 23, 2023
@hanabi1224 hanabi1224 deleted the hm/track-peak-rss branch March 23, 2023 08:22
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.

None yet

3 participants