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

fiber context update for macOS #4313

Merged
merged 1 commit into from Mar 28, 2021

Conversation

devnexen
Copy link
Contributor

it is more about memory accounting sake. At allocation time,
we make clear we re possibly reusing regions marked as reusable.
Noted also calls might not necessarily succeed at first so we do
only when necessary.

@ioquatix
Copy link
Member

ioquatix commented Mar 23, 2021

Thanks for this.

Have you ever seen it return EAGAIN? Under what situation can it happen?

This is just advisory so maybe it's not important. I would be concerned about introducing infinite loop.

It makes sense we do it after allocating pages because they are not going to be used right away. However there should be a performance cost to this. Also, I wondered is this not the default state, or can we provide the flags to mmap?

@ioquatix ioquatix self-assigned this Mar 23, 2021
@ioquatix ioquatix self-requested a review March 23, 2021 22:57
@@ -433,6 +433,12 @@ fiber_pool_allocate_memory(size_t * count, size_t stride)
*count = (*count) >> 1;
}
else {
#if defined(MADV_FREE_REUSE)
Copy link
Member

Choose a reason for hiding this comment

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

Is MADV_FREE_REUSE a MacOS specific thing? Because it's different from below (MADV_FREE_REUSABLE).

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 it s Darwin specific.

Copy link
Member

Choose a reason for hiding this comment

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

We should add a comment to explain this.

@devnexen
Copy link
Contributor Author

Thanks for this.

Have you ever seen it return EGAIN? Under what situation can it happen?

This is just advisory so maybe it's not important. I would be concerned about introducing infinite loop.

It makes sense we do it after allocating pages because they are not going to be used right away. However there should be a performance cost to this. Also, I wondered is this not the default state, or can we provide the flags to mmap?

There might be tiny performance cost indeed but it's for the system to give hint about proper resident memory accounting. Unfortunately there is no mmap flag for this also the EAGAIN case occurs rarely but sometimes the system gives little extra chance to complete its task, the internal allocator of mac does similarly for example for committing/decommitting pages.

@ioquatix
Copy link
Member

ioquatix commented Mar 24, 2021

It's not so much the performance cost, it's more that we now have an infinite loop in the interpreter, where the OS can validly return EAGAIN forever. I don't personally mind one implementation or another, but as an alternative:

if (madvise(...) results in EAGAIN)
  madvise(...) # try once more, ignore results, we did our best.

What do you think?

@ioquatix
Copy link
Member

samuel@Fukurou /tmp> cat test.c
#include <stdio.h>
#include <sys/mman.h>

int main(int argc, const char ** argv) {
	printf("MADV_FREE_REUSABLE = %d\n", MADV_FREE_REUSABLE);
	printf("MADV_FREE_REUSE = %d\n", MADV_FREE_REUSE);
	
	return 0;
}
samuel@Fukurou /tmp> clang test.c
samuel@Fukurou /tmp> ./a.out
MADV_FREE_REUSABLE = 7
MADV_FREE_REUSE = 8

I tried it and it is valid on macOS, but it doesn't seem to be documented in the man page. Weird. Are they actually different?

@ioquatix
Copy link
Member

ioquatix commented Mar 24, 2021

I noticed this implementation in Go:

    23  func sysUnused(v unsafe.Pointer, n uintptr) {
    24  	// MADV_FREE_REUSABLE is like MADV_FREE except it also propagates
    25  	// accounting information about the process to task_info.
    26  	madvise(v, n, _MADV_FREE_REUSABLE)
    27  }
    28  
    29  func sysUsed(v unsafe.Pointer, n uintptr) {
    30  	// MADV_FREE_REUSE is necessary to keep the kernel's accounting
    31  	// accurate. If called on any memory region that hasn't been
    32  	// MADV_FREE_REUSABLE'd, it's a no-op.
    33  	madvise(v, n, _MADV_FREE_REUSE)
    34  }

It seems one is used for reporting that it's in use and one is used for reporting it's not in use. Is that correct?

@devnexen
Copy link
Contributor Author

samuel@Fukurou /tmp> cat test.c
#include <stdio.h>
#include <sys/mman.h>

int main(int argc, const char ** argv) {
	printf("MADV_FREE_REUSABLE = %d\n", MADV_FREE_REUSABLE);
	printf("MADV_FREE_REUSE = %d\n", MADV_FREE_REUSE);
	
	return 0;
}
samuel@Fukurou /tmp> clang test.c
samuel@Fukurou /tmp> ./a.out
MADV_FREE_REUSABLE = 7
MADV_FREE_REUSE = 8

I tried it and it is valid on macOS, but it doesn't seem to be documented in the man page. Weird. Are they actually different?

Yes they are, they complete each other. reusable means "now this chunk can be reused" reuse means "I actually reuse this chunk previously marked by reusable" (note if it was not marked it s no op).

@devnexen
Copy link
Contributor Author

It's not so much the performance cost, it's more that we now have an infinite loop in the interpreter, where the OS can validly return EAGAIN forever. I don't personally mind one implementation or another, but as an alternative:

if (madvise(...) results in EAGAIN)
  madvise(...) # try once more, ignore results, we did our best.

What do you think?

Another example, webkit
https://github.com/adobe/webkit/blob/master/Source/WTF/wtf/OSAllocatorPosix.cpp#L49 and https://github.com/adobe/webkit/blob/master/Source/WTF/wtf/OSAllocatorPosix.cpp#L169

Note they both do not appear in the man page and it is poorly documented indeed, it is however grabbing hints and implementing it (eg in custom allocators) that I got to know how.

@hsbt hsbt changed the title fiber context update for Mac OS. fiber context update for macOS Mar 25, 2021
@ioquatix
Copy link
Member

IF you believe the loop is necessary, please add it for all instances of madvise in cont.c, or explain in what cases it's needed or not.

Once it's done, we can merge.

@devnexen
Copy link
Contributor Author

It is only needed for the Mac case I ll comment the code.

it is more about memory accounting sake. At allocation time,
 we make clear we re possibly reusing regions marked as reusable.
Noted also calls might not necessarily succeed at first so we do
 only when necessary.
@devnexen devnexen force-pushed the mac_coroutine_task_info_awareness branch from 2354e0a to c120f34 Compare March 27, 2021 09:06
@ioquatix ioquatix merged commit 875c85a into ruby:master Mar 28, 2021
@ioquatix
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants