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

Base.purgeAddressSpace() via madvise() #336

Closed
wants to merge 1 commit into from

Conversation

dsm9000
Copy link
Contributor

@dsm9000 dsm9000 commented Oct 31, 2023

Split from #335 , but does not depend on it.

This feature permits the purging of an arbitrary run of dirty pages, rendering it clean, as requested in #270 .

Uses the "lazy" madvise operation MADV_FREE on Linux (where supported) and the equiv. but eager MADV_DONTNEED operation on all other supported platforms.

AFAIK not feasible to unit test, as the OS provides no immediate confirmation of the effects of madvise.

Required by #337 .

Comment on lines +15 to +16
enum MADV_DONTNEED = 4;
enum MADV_FREE = 8;
Copy link
Contributor

Choose a reason for hiding this comment

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

Put them in each platform, there is no guarantee that values are the same.

Comment on lines +26 to +28
enum Advise {
Purge = MADV_DONTNEED
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not match definitions in mman.h . If there is a notion of purge for the GC, it belongs in the GC.

@@ -86,6 +86,18 @@ public:
return (cast(Base*) &this).reserveAddressSpaceImpl(size, alignment);
}

void purgeAddressSpace(void* address, size_t size) shared {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this in base? it's using none of the data from the base, so that doesn't seems necessary. In addition, why is this taking the lock?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's in base because that's where mmap and all related system calls are found.
Where precisely do you want it? And what's the reasoning for it not needing the lock?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is not using anything in base. It's not about where I want it, it's about where it belongs. You need to be finding these thing out or we are condemned to have endless back and forth.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would it be appropriate to move the madvise etc. to purgeAddressSpace per se?

Copy link
Contributor

Choose a reason for hiding this comment

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

What is purgeAdressSpace for here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Why is it in the base?

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.

2 participants