Switching to using memfd for input data #990

quantum5 · 2022-01-30T05:23:16Z

Breaking changes:

All interactors using _interact_with_process

This closes #835.

codecov-commenter · 2022-01-30T05:30:03Z

Codecov Report

Attention: Patch coverage is 87.79528% with 31 lines in your changes missing coverage. Please review.

Project coverage is 82.59%. Comparing base (b199763) to head (709c6db).
Report is 10 commits behind head on master.

Files with missing lines	Patch %	Lines
dmoj/cptbox/lazy_bytes.py	69.23%	16 Missing ⚠️
dmoj/cptbox/utils.py	87.34%	10 Missing ⚠️
dmoj/cptbox/isolate.py	0.00%	2 Missing ⚠️
dmoj/executors/shell_executor.py	0.00%	2 Missing ⚠️
dmoj/problem.py	97.36%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #990      +/-   ##
==========================================
+ Coverage   82.37%   82.59%   +0.21%     
==========================================
  Files         143      146       +3     
  Lines        5442     5675     +233     
==========================================
+ Hits         4483     4687     +204     
- Misses        959      988      +29

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Riolku · 2022-03-05T02:25:19Z

dmoj/cptbox/utils.py

-        super().__init__(memory_fd_create(), 'r+')
+    _name: Optional[str] = None
+
+    def __init__(self, prefill: Optional[bytes] = None, seal=False) -> None:


Maybe one or both of these should be required kwargs. I'm thinking the second should. What is the difference between prefilling with nothing, and passing None?

Made keyword arguments required.

dmoj/cptbox/utils.py

Riolku · 2022-03-05T05:02:15Z

dmoj/cptbox/utils.py

+            if e.errno == errno.ENOSYS:
+                # FreeBSD
+                self.seek(0, os.SEEK_SET)
+                return


I'm not quite sure what this does. Does it deserve more of a comment?

dmoj/problem.py

Riolku · 2022-03-05T05:23:06Z

dmoj/cptbox/helper.cpp

@@ -306,7 +306,7 @@ int memory_fd_create(void) {
 #ifdef __FreeBSD__


Actually, is this function called on FreeBSD anymore? Are you creating the tempfile in Python instead?

Let's keep this around for now, since I'd rather this function work on all platforms, as the detection logic for the FreeBSD case is now different. If FreeBSD implements /proc/[pid]/fd some day, this will magically work.

Riolku · 2022-03-05T05:24:55Z

dmoj/cptbox/utils.py

+        try:
+            os.dup2(new_fd, fd)
+        finally:
+            os.close(new_fd)


Maybe we could use a comment about why this dup is needed. Also, why isn't it implemented in the C function?

C code is just that much more painful to maintain.

This changes the permissions of the backing fd from RW to RO. A comment seems fine.

Factored it to a function with an obvious name.

Riolku · 2022-03-05T05:27:04Z

dmoj/executors/shell_executor.py

@@ -21,10 +21,10 @@ def get_fs(self):
    def get_allowed_syscalls(self):
        return super().get_allowed_syscalls() + ['fork', 'waitpid', 'wait4']

-    def get_security(self, launch_kwargs=None):
+    def get_security(self, launch_kwargs=None, extra_fs=None):


Not that this is a direct result of this PR, but maybe this should be converted to **kwargs instead?

We can do that later.

Xyene · 2024-12-29T20:53:31Z

dmoj/cptbox/utils.py

+    _name: Optional[str] = None
+
+    def __init__(self, prefill: Optional[bytes] = None, seal=False) -> None:
+        if FREEBSD:


I think we either want to branch on some sort of "expected size" here, or figure out something to make the memory backing this file be more likely to be swapped out.

If the input is 5 GiB large, we don't want to buffer all of it in memory.

Leaving this up to the caller for now. If the caller desires, it can use NamedFileIO.

Xyene · 2024-12-29T20:55:47Z

dmoj/cptbox/utils.py

+                self._name = f.name
+                super().__init__(os.dup(f.fileno()), 'r+')
+        else:
+            super().__init__(memory_fd_create(), 'r+')


I think these branches should be a try for memory_fd_create, and a fallback for using a disk-based file. Feels more straightforward than special-casing FreeBSD here (and only here; seal is implemented in an OS-agnostic way).

Changed to have both NamedFileIO and MemoryIO, with the latter being an alias of the former on FreeBSD.

Xyene · 2024-12-29T20:57:44Z

dmoj/cptbox/utils.py

+        try:
+            os.dup2(new_fd, fd)
+        finally:
+            os.close(new_fd)


This changes the permissions of the backing fd from RW to RO. A comment seems fine.

Xyene · 2024-12-29T20:59:23Z

dmoj/cptbox/utils.py

+    def close(self) -> None:
+        super().close()
+        if self._name:
+            os.unlink(self._name)


I would think that we could do this up-front (don't pass delete=False). It's still possible to write to an orphaned file. We can drop self._name and have to_path generate the procfs path unconditionally.

The resulting file can't be reopened from another process on FreeBSD. Made this more explicit in the new version.

Xyene · 2024-12-29T21:03:38Z

dmoj/cptbox/utils.py


 from dmoj.cptbox._cptbox import memory_fd_create, memory_fd_seal
+from dmoj.cptbox.tracer import FREEBSD


 class MemoryIO(io.FileIO):


Where/how do we end up deleting the memory backing these objects once they're no longer necessary to grade a case?

It's closed when MemoryIO.close is called or it's garbage collected, since FileIO constructor takes ownership of the file descriptor unless closefd=False is passed.

dmoj/cptbox/utils.py

Xyene · 2024-12-30T03:22:38Z

dmoj/cptbox/utils.py

+
+    def to_bytes(self) -> bytes:
+        try:
+            with mmap.mmap(self.fileno(), 0, access=mmap.ACCESS_READ) as f:


How often do we expect this will be called? Should we madvise(..., MADV_SEQUENTIAL) here?

Not very often, it's mostly for compatibility with old checkers etc.

Sounds like "very often" to me, but happy to punt on this. I worry we'll hit issues with gigabyte-sized generator inputs that also have checkers, since this doubles the memory requirement.

It's less of a problem than it looks. In the standard grader, we pass this magic to checkers: judge_input=LazyBytes(case.input_data). We only pay for this if the checker actually reads judge_input.

dmoj/cptbox/utils.py

quantum5 force-pushed the memfd-judge branch 11 times, most recently from e30f7b1 to 66437a7 Compare January 30, 2022 21:11

quantum5 marked this pull request as ready for review January 30, 2022 21:20

quantum5 requested review from kiritofeng, Xyene and WallE256 January 30, 2022 21:27

Riolku reviewed Mar 5, 2022

View reviewed changes

Xyene mentioned this pull request Dec 7, 2024

Unstable test timing results #1190

Open

Add streaming line-ending normalization utility

f3dac23

quantum5 force-pushed the memfd-judge branch 2 times, most recently from 8aa122f to a903955 Compare December 28, 2024 04:23

Offer input data as memfd in standard judge

3824ea2

quantum5 force-pushed the memfd-judge branch from a903955 to 3111bc5 Compare December 28, 2024 04:34

quantum5 added 4 commits December 28, 2024 00:05

Allow passing memfd to bridged interactors

6144b60

Use LazyBytes to avoid converting memfd to bytes unless needed

c88ef56

Fix memfd_create emulation bug on FreeBSD

cbb0f87

Allow passing memfd to bridged checkers

8f4ba33

quantum5 force-pushed the memfd-judge branch from 3111bc5 to 7a9da0e Compare December 28, 2024 05:06

Xyene reviewed Dec 29, 2024

View reviewed changes

quantum5 force-pushed the memfd-judge branch 3 times, most recently from 023b22b to ea93c96 Compare December 30, 2024 03:27

Xyene reviewed Dec 30, 2024

View reviewed changes

dmoj/cptbox/utils.py Show resolved Hide resolved

quantum5 force-pushed the memfd-judge branch from ea93c96 to 17a551c Compare December 30, 2024 03:57

Implement graceful memfd fallback for FreeBSD

52948c7

quantum5 force-pushed the memfd-judge branch from 17a551c to 52948c7 Compare December 30, 2024 03:58

Implement generator writing to memfd

77bf381

quantum5 force-pushed the memfd-judge branch from 0ad1b83 to 77bf381 Compare December 30, 2024 05:05

Rename MemoryIO *_fd to *_io

709c6db

Xyene approved these changes Dec 30, 2024

View reviewed changes

quantum5 merged commit 315ede5 into master Dec 30, 2024
19 checks passed

quantum5 deleted the memfd-judge branch December 30, 2024 05:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switching to using memfd for input data #990

Switching to using memfd for input data #990

quantum5 commented Jan 30, 2022 •

edited

Loading

codecov-commenter commented Jan 30, 2022 •

edited by codecov bot

Loading

Riolku Mar 5, 2022

quantum5 Dec 30, 2024

Riolku Mar 5, 2022

Riolku Mar 5, 2022

quantum5 Dec 30, 2024

Riolku Mar 5, 2022

quantum5 Mar 11, 2022

Xyene Dec 29, 2024

quantum5 Dec 30, 2024

Riolku Mar 5, 2022

quantum5 Dec 30, 2024

Xyene Dec 29, 2024

quantum5 Dec 30, 2024

Xyene Dec 29, 2024

quantum5 Dec 30, 2024

Xyene Dec 29, 2024

Xyene Dec 29, 2024

quantum5 Dec 30, 2024

Xyene Dec 29, 2024

quantum5 Dec 30, 2024

Xyene Dec 30, 2024

quantum5 Dec 30, 2024

Xyene Dec 30, 2024

quantum5 Dec 30, 2024

		@@ -306,7 +306,7 @@ int memory_fd_create(void) {
		#ifdef __FreeBSD__

Switching to using memfd for input data #990

Switching to using memfd for input data #990

Conversation

quantum5 commented Jan 30, 2022 • edited Loading

codecov-commenter commented Jan 30, 2022 • edited by codecov bot Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quantum5 commented Jan 30, 2022 •

edited

Loading

codecov-commenter commented Jan 30, 2022 •

edited by codecov bot

Loading