ryobg / JContainers

JSON-based data structures for Papyrus - the TESV Skyrim SE scripting language
MIT License
107 stars 23 forks source link

Patched luajit compile with gc64 flag to fix 2GB address limitation t… #118

Open rfortier opened 2 months ago

rfortier commented 2 months ago

…hat is hit on large builds, or modest builds with Skyrim Together Reborn

rfortier commented 2 months ago

The PR finishes the conversion of JContainers64.dll to the x64 address model. This entails adding "gc64" to the build instructions for luajit to remove the restriction that luajit could only allocate memory from the first 2GB of the virtual address space.

64-bit address space support is considered beta in the current luajit version 2.1.0-beta3. Might want more than a patch bump to the version, or even a beta release first . I've seen zero issues is testing, but JContainers is a heavily used mod. A test build for Skyrim SE 1.6.1170 is available on the author's release page. Please try it out and post any issues here or as an issue on my page.

The fix entails 2 parts. The first is a change to lua_module.cpp to check for the failure of luaL_newstate() and retry every second until it works. Without this check, when no memory below 2GB is available on large builds, you get a CTD from reopen_if_closed() when loading a save. With this fix it won't crash, but it can take several minutes before a big enough chunk of memory is available below 2GB to continue.

You could still encounter a crash later on other allocations that aren't protected, though.

The complete fix involved compiling luajit with the gc64 flag to remove the restriction that allocated memory had to be below the first 2GB.

The problem is particularly bad if you use the Skyrim Together Reborn mod, which changes the memory layout to load it's own logic. A modest build with STR, JContainers, AH Hotkeys (requires JContainers) and a big inventory is enough to provoke the problem (which is why I hunted it down and fixed it).

ryobg commented 2 months ago

Thanks for the proposal. I have no intention to make a release just for that for now. Please, do test meanwhile.

rfortier commented 2 months ago

I should have been clearer.

The "wait for it to work" patch is the minimal intervention to get something that doesn't crash out of the gate. But if you accept the more invasive change to adopt the x64 address model, the wait is always zero (it always works the first time); I just left the message in to prove that.

Without the x64 change, if a timeout is added to the wait loop, the game will just crash immediately. So, that's not an option. Best that could be done is a more visible error message. Since I don't know how to build a pop-up dialog, that would need some research.

The x64 change certainly should get some shakedown / beta soak time before being released, I agree. Since you don't want to post a release right now, would you mind if I linked my "beta" build in the PR comments? I can also update readmes to ask people to comment.

Long term, I expect the trend to be that longer mod lists will cause more and more "random" issues with JContainers if it doesn't move to x64.

Rich

On Tue, Jul 30, 2024 at 11:49 AM ryobg @.***> wrote:

Thanks for the proposal. I have no intention to make a release just for that for now. Please, do test meanwhile.

  • waiting for several minutes to proceed does not sound good at all
  • personally, I would prefer to have some kind of time limit on that waiting

— Reply to this email directly, view it on GitHub https://github.com/ryobg/JContainers/pull/118#issuecomment-2258672996, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAM744DQ5JDH56DQQ4AEN4LZO6Y2DAVCNFSM6AAAAABLS6A7CGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENJYGY3TEOJZGY . You are receiving this because you authored the thread.Message ID: @.***>

ryobg commented 2 months ago

would you mind if I linked my "beta" build in the PR comments?

Nope, go ahead.

if a timeout is added to the wait loop, the game will just crash immediately.

I meant something along the lines, that loop to have an end condition. Currently it is wait forever until it is done.

rfortier commented 2 months ago

Thanks.

There are a couple of options:

  1. With gc64 enabled, it should never happen. So just log a message and let it crash if it does.
  2. Keep the loop and an option to compile WITHOUT gc64, but log a message when the wait starts, wait no more than ~3minutes. If it times out log a message and let it crash.

Or pick a shorter timeout, but something seems to happen at about the 2:30 mark. I don't know if base skyrim frees a chunk of memory or it's one of my mods, but that's how long it takes to not crash with the 2GB memory model.

rfortier commented 2 months ago

As requested, updated the "wait for memory" loop to timeout. Also added explicit logging to the JContainers64.log to tell people it is waiting, why it is waiting, and why it crashes when it times out.