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

std.process.Child: implement maxrss on Darwin #15050

Merged
merged 1 commit into from
Mar 25, 2023

Conversation

tjog
Copy link
Contributor

@tjog tjog commented Mar 22, 2023

Notably the Darwin (XNU) kernel the maxrss field is number of bytes and not kilobytes (kibibytes) like other platforms (e.g. Linux, BSD).

This is my first PR, although very small in scope, feel welcome to give input.

Fixes #14957

@tjog tjog marked this pull request as draft March 23, 2023 03:13
@tjog
Copy link
Contributor Author

tjog commented Mar 23, 2023

Turns out iOS does not match the OSX implementation (related node GH issue, and libuv solution PR):

Will get a simulator up for .ios, .watchos, .tvos to verify the archived man paged still applies to those.

@tjog
Copy link
Contributor Author

tjog commented Mar 23, 2023

Turns out the archived page is wrong. Darwin is consistent across apple platforms for rusage, at least if you are to believe the Swift GH repo: https://github.com/apple/swift/blob/dfcc4fce9883ac9989eef9893651a780bad88cb4/lib/Basic/Unix/TaskQueue.inc#L60-L63

I verified this on iOS simulator (see code block below). One complication is that on iOS the first wait4 call is always interrupted leaving garbage values in rusage. However, the second time around it succeeds. Discovered through how Swift uses wait4. MacOS does not need the retry. It always succeeds on the first wait4.

But, fork and posix_spawn is not available on other Apple platforms than iOS and MacOS. From what I gather online the iOS sandbox should block all attempts to create a subprocess anyway. Only jailbroken iOS devices which do not have this restriction would succeed. The only argument against not setting iOS as can_spawn=false are using fork as a way to detect if the device is jailbroken or not. I have no clue how common this is though.

The fact fork works in iOS simulator is odd, maybe the sandbox is disabled for performance reasons.

I still think the best option is to set these other Darwin targets as unable to spawn and generate compile errors. This will allow reverting back to switch cases for getMaxRss and rusage_init. A single platform .macos is also cleaner than the is_darwin version. Will do this and await input.

For completeness, here is the C code ran through Xcode that verifies iOS also returns maxrss in bytes. The same code was run on MacOS by compiling with clang (no Xcode project was created).

Code and results from macOS and iOS simulator, `wait4` perror is logged once for iOS simulator but never for macOS.
#include <assert.h>
#include <sys/errno.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <sys/wait.h>
#include <sys/resource.h>

int main(int argc, char * argv[]) {
    pid_t pid = fork();
    if (pid == 0) {
        // Child process
        size_t mem_size = (20 * 1024 * 1024); // Allocate 20MB of memory
        // maxrss results:
        // (MB granularity to ensure underlying allocator always grabs
        //  different amount of pages):
        // mem_size    macOS       iOS
        // 1MB       1417216   2273280
        // 4MB       4571136   5455872
        // 10MB     10862592  11714560
	// 20MB     21348352  22167552
        char* mem = malloc(mem_size);
        if (mem == NULL) {
            perror("malloc");
            exit(1);
        }
        // Use the memory to ensure it's not optimized away
        for (size_t i = 0; i < mem_size; i++) {
            mem[i] = i % 256;
        }
        sleep(1); // simulate some work
        free(mem);
        printf("Child exiting\n");
        exit(0);
    } else if (pid > 0) {
        // Parent process
        // From Swift example https://github.com/apple/swift/blob/dfcc4fce9883ac9989eef9893651a780bad88cb4/lib/Basic/Unix/TaskQueue.inc#L599-L619
        int Status = 0;
        struct rusage Usage;
        for (;;) {
            // NOTE: only succeeds second time for iOS, need for loop
            const pid_t pidFromWait = wait4(pid, &Status, 0, &Usage);
            
            if (pidFromWait == pid)
                break;
            assert(pidFromWait == -1 &&
                   "Did not pass WNOHANG, should only get pidToWaitFor or -1");
            if (errno == ECHILD || errno == EINVAL)
                exit(1);
            else {
                perror("wait4");
            }
        }
        int exit_status = WEXITSTATUS(Status);
        printf("Child process terminated with status %d\n", exit_status);
        printf("User time used: %ld.%06d s\n", Usage.ru_utime.tv_sec, Usage.ru_utime.tv_usec);
        printf("System time used: %ld.%06d s\n", Usage.ru_stime.tv_sec, Usage.ru_stime.tv_usec);
        printf("Max RSS: %ld\n", Usage.ru_maxrss);
    } else {
        // Fork failed
        perror("fork");
        exit(1);
    }
}

@kubkon
Copy link
Member

kubkon commented Mar 23, 2023

Turns out iOS does not match the OSX implementation (related node GH issue, and libuv solution PR):

* OSX: https://github.com/apple/darwin-xnu/blob/2ff845c2e033bd0ff64b5b6aa6063a1f8f65aa32/bsd/man/man2/getrusage.2#L95

* iOS: https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man2/getrusage.2.html

Will get a simulator up for .ios, .watchos, .tvos to verify the archived man paged still applies to those.

For the future, official Apple OSS distributions can be found here: https://github.com/apple-oss-distributions

Copy link
Member

@kubkon kubkon left a comment

Choose a reason for hiding this comment

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

Nice work! I've got a couple nits and then i think we can go ahead and merge it. When it comes to iOS and other Apple platforms, I don't think anyone would want to spawn a child process using this API for the reasons you mentioned. I believe that for those platforms Apple prescribes you use their frameworks more than anything else. Having said that even if you could spawn a child with this API and it didn't work, we can always revisit later for better iOS support (and other Apple platforms).

lib/std/child_process.zig Outdated Show resolved Hide resolved
lib/std/child_process.zig Outdated Show resolved Hide resolved
@tjog tjog force-pushed the darwin-detect-child-maxrss branch 3 times, most recently from 1f123be to 43c50f8 Compare March 23, 2023 21:12
@tjog
Copy link
Contributor Author

tjog commented Mar 23, 2023

Updated with your comments in mind! Did not split the switch cases since they share the same implementation.

@tjog tjog requested a review from kubkon March 23, 2023 21:18
@tjog tjog marked this pull request as ready for review March 23, 2023 21:18
Copy link
Member

@kubkon kubkon left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@kubkon
Copy link
Member

kubkon commented Mar 24, 2023

Can you resolve conflicts with the master branch please?

Notably the Darwin (XNU) kernel the maxrss field is number of bytes
and not kilobytes (kibibytes) like other platforms (e.g. Linux, BSD).

watchOS and tvOS are not supported because they do not have the ability
to spawn a child process. iOS is enabled but due to OS sandboxing it
should fail with a permission error.
@tjog tjog force-pushed the darwin-detect-child-maxrss branch from 43c50f8 to 25a0896 Compare March 24, 2023 20:39
@tjog
Copy link
Contributor Author

tjog commented Mar 24, 2023

Rebased and resolved! @kubkon

@kubkon kubkon enabled auto-merge (rebase) March 24, 2023 22:29
@kubkon kubkon merged commit f6a2b72 into ziglang:master Mar 25, 2023
@tjog tjog deleted the darwin-detect-child-maxrss branch March 25, 2023 06:48
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.

add Darwin support for detecting maxrss of child process
2 participants