Conversation
The patch is relatively easy; what's not clear to me is how something like this really wants to be exported. Should `sysdeps` export a `MappedRegion` that would be backed by `VirtualAlloc` on Windows? Should `sysdeps` just export everything with their platform names and let `lucet-runtime` use conditionals to choose what to export? This patch punts on those questions for now; we're just trying to move the obvious system-dependent code into `sysdeps`.
|
Hello! Thanks for this work! I know that you requested that @acfoltzer review this PR. He asked me to review the PR so that you could get a timely response. My intention is to continue the conversation you've started around this valuable reorganization of system-dependent Region implementations. At a high-level, I appreciate the sensibility of putting system-dependent code into a directory called System-dependent Region code requires a less-straightforward refactor. As you can see from the merge conflicts on this PR, other folks have also been working on Regions in the What I like about the timing of your PR alongside the above-mentioned PR is that the repo now has a concrete example of what it looks like when there are side-by-side system-dependent implementations of Region in the repo. Here are some highlights: First, there is some work to conditionally include the uffd version: Second, there are some fiddly things that take place to run the tests — once for mmap and once for userfaultfd: Finally, the repo has taken on a new CI (CircleCI) so that it has Linux containers that are privileged enough to use the userfaultfd mechanism. Certainly, this concrete example isn't ideal. But it is something to look at and may spark insights. You asked really good questions. To paraphrase, “How does something like this really want to be exported?” Here’s one way that’s done now in a Fastly repo:
Now that the |
|
This PR was closed as a byproduct of deleting the branch named |
The patch is relatively easy; what's not clear to me is how something
like this really wants to be exported. Should
sysdepsexport aMappedRegionthat would be backed byVirtualAllocon Windows?Should
sysdepsjust export everything with their platform names andlet
lucet-runtimeuse conditionals to choose what to export?This patch punts on those questions for now; we're just trying to move
the obvious system-dependent code into
sysdeps.