[WIP] Libnethack shared #140

tscmoo · 2020-12-07T02:26:33Z

I'm creating this draft PR as things appear to be fully functional, but there are at minimum some cleanups necessary, and some more functionality I'd like to add.

This introduces a "shared" mode to the libnethack approach. It is Linux-only, and implements manual loading of the shared object file to avoid the need to copy the library anywhere and to better isolate the environment.
It is enabled by default on Linux and will fall back to the existing (dlopen) behavior on other platforms.
I called it shared as the shared object is actually shared between instances. Yey.

Some advantages:

never creates or writes to any files on the filesystem. NetHack is tricked into using temporary files in memory when it wants to write to something. Thus it doesn't leak anything even in the event of a crash, and instances are guaranteed not to inferere with each other (not even the leaderboard is shared). chdir is also supressed.
Should perform better, though this remains to be tested.
- Resets should be much faster as they now involve little more than a 130kb memcpy.
- Copies of the library are still done in memory, but they are copy-on-write mmapped copies - this allows them to share large portions of read-only instruction data, which in turn will be shared in the instruction cache in the CPU.
Some functions such as fork, popen, exit will currently throw an error, but my intention is for them to just end the episode. This allows us to continue even if nethack panics or quits for some reason.
It will be possible to track memory resources to detect leaks or force cleanup.

tscmoo · 2020-12-07T02:28:15Z

nle/nethack/nethack.py

-        dlpath = os.path.join(self._vardir, "libnethack.so")
-        shutil.copyfile(DLPATH, dlpath)
+          # Create a HACKDIR for us.
+          if os.getenv("SLURM_JOBID"):


This slurm stuff probably doesn't belong here and I'll most likely remove it from this PR

heiner

I'm not sure I can review this more than superficially.

Would be interested if this releves any lock contention issue the dlopen/dlclose cycles produced?

nle/nethack/nethack.py

heiner · 2020-12-07T11:22:39Z

nle/nethack/nethack.py

-            os.close(os.open(os.path.join(self._vardir, fn), os.O_CREAT))
-        os.mkdir(os.path.join(self._vardir, "save"))
+        if _pynethack.supports_shared():
+          # "shared" mode does some hacky things to enable using a


This mix of 2-letter and 4-letter indentation is weeeiiiirdd

Yes, it is. I apologize. I come from a world of 2 indentation. Formatting will be cleaned up.

sys/unix/nledl.c

sys/unix/nleshared.cc

heiner · 2020-12-07T16:30:46Z

sys/unix/nleshared.cc

+    endaddr = 0;
+    size = 0;
+
+    forPh([&](Elf64_Phdr* ph) {;


I have a very vague notion of what this is doing: It's like a reimplementation of parts of Linux dynamic library loading. It interprets the data in the so in ELF format, creates manual copies ("forks"), and gives access to symbols a la dlsym.

I can't claim I understand much beyond that and perhaps a few comments might be good?

I will try to outline the behavior in comments.

heiner · 2020-12-07T16:31:59Z

nle/nethack/nethack.py

@@ -75,13 +75,13 @@ def _set_env_vars(options, hackdir, wizkit=None):
    if wizkit is not None:
        os.environ["WIZKIT"] = wizkit

+_nhinstances = 0

 # TODO: Not thread-safe for many reasons.
 # TODO: On Linux, we could use dlmopen to use different linker namespaces,


We can probably update some of these comments when we merge this.

heiner · 2020-12-07T16:38:47Z

Ah I read the PR description now.

Some functions such as fork, popen, exit will currently throw an error, but my intention is for them to just end the episode. This allows us to continue even if nethack panics or quits for some reason.

If you make them call nethack_exit(int status) that should do it.

As for the not-writing any files approach: I'm not sure to what extend users are currently using the built-in highscore (written to a file, persistent for one instance of NLE). Probably not much, and very probably not enough to warrant doing something different than you are doing here.

If only there were a POSIX way of doing this :P

tscmoo · 2020-12-07T18:30:35Z

If you make them call nethack_exit(int status) that should do it.

Great, thanks.

The records (highscore) file could be made persistent between resets, but is there even anything it could be used for?

If only there were a POSIX way of doing this :P

The other approach to replacing these functions would of course be to do it in nethack, through the system specific configs and defines.

sys/unix/nleshared.cc

heiner

One more question:

It will be possible to track memory resources to detect leaks or force cleanup.

How would you do that? Overriding each of malloc, alloc, posix_memalign, etc?

sys/unix/nleshared.cc

heiner · 2020-12-08T10:41:31Z

The records (highscore) file could be made persistent between resets, but is there even anything it could be used for?

It's not very important; the only effect a persistent highscore has is that the replays contain this relative comparison of the episode that just ended with other episodes. I'm not sure to what extend researchers look at replays and the highscore data in there.

heiner · 2020-12-09T11:14:25Z

sys/unix/nleshared.cc

+    size_t baseaddr = ~0;
+    size_t endaddr = 0;
+
+    forPh([&](Elf64_Phdr* ph) {


I'm assuming this is fast enough to not need the ability to break based on e.g. a return value of the lambda?

This code only runs once in the entire lifespan of the process, so... There's really nothing to be gained by making this stuff faster.
(also, it is very fast yeah)

tscmoo added 6 commits December 7, 2020 02:49

wip shared

c289976

stash

7eaffe5

clean up

80f59d5

fix non-shared path

908f61f

load DT_NEEDED libraries

7fa21bb

Disable rename & unlink

bd5cebe

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 7, 2020

cmake cleanup

a9e1897

tscmoo commented Dec 7, 2020

View reviewed changes

heiner reviewed Dec 7, 2020

View reviewed changes

tscmoo added 2 commits December 8, 2020 00:15

Some abstractin' and refactorin'

37c45c5

Some more

2c281fc

heiner reviewed Dec 8, 2020

View reviewed changes

sys/unix/nleshared.cc Outdated Show resolved Hide resolved

heiner reviewed Dec 8, 2020

View reviewed changes

sys/unix/nleshared.cc Outdated Show resolved Hide resolved

heiner reviewed Dec 9, 2020

View reviewed changes

tscmoo added 5 commits December 10, 2020 19:51

Throw error library could not be opened

60a62c9

clang-format

2de2491

clang-format

8a0c39e

black

cecde27

test_nethack: update so it works with shared libnethack

a77cb20

heiner mentioned this pull request May 6, 2024

TODOs 2024 heiner/nle#6

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Libnethack shared #140

[WIP] Libnethack shared #140

tscmoo commented Dec 7, 2020

tscmoo Dec 7, 2020

heiner left a comment

heiner Dec 7, 2020

tscmoo Dec 7, 2020

heiner Dec 7, 2020

tscmoo Dec 7, 2020

heiner Dec 7, 2020

heiner commented Dec 7, 2020

tscmoo commented Dec 7, 2020 •

edited

Loading

heiner left a comment

heiner commented Dec 8, 2020

heiner Dec 9, 2020

tscmoo Dec 9, 2020 •

edited

Loading

[WIP] Libnethack shared #140

Are you sure you want to change the base?

[WIP] Libnethack shared #140

Conversation

tscmoo commented Dec 7, 2020

tscmoo Dec 7, 2020

Choose a reason for hiding this comment

heiner left a comment

Choose a reason for hiding this comment

heiner Dec 7, 2020

Choose a reason for hiding this comment

tscmoo Dec 7, 2020

Choose a reason for hiding this comment

heiner Dec 7, 2020

Choose a reason for hiding this comment

tscmoo Dec 7, 2020

Choose a reason for hiding this comment

heiner Dec 7, 2020

Choose a reason for hiding this comment

heiner commented Dec 7, 2020

tscmoo commented Dec 7, 2020 • edited Loading

heiner left a comment

Choose a reason for hiding this comment

heiner commented Dec 8, 2020

heiner Dec 9, 2020

Choose a reason for hiding this comment

tscmoo Dec 9, 2020 • edited Loading

Choose a reason for hiding this comment

tscmoo commented Dec 7, 2020 •

edited

Loading

tscmoo Dec 9, 2020 •

edited

Loading