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

[rfortier]

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

[rfortier]

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]

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

[rfortier]

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 <#118 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAM744DQ5JDH56DQQ4AEN4LZO6Y2DAVCNFSM6AAAAABLS6A7CGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENJYGY3TEOJZGY> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[ryobg]

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]

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]

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.

[rfortier]

We're coming up on a year since this PR, and Bethesda still hasn't pushed an update that will require a rebuild. I'd like to make it easier for folks to get at the fix for big or Skyrim Together builds by putting it up on Nexus (it's MIT License, but it's more courteous to work with the author(s)).

So I have a couple of suggestions. First, I'm going to update the PR with a build setup that generates both versions, then create a FOMOD that offers some explanation and a choice to install the original or x64 model. Or maybe just packages with your current version so it is definitively unchanged.

Then, you could offer that in the "additional files" section of your Nexus mod so you get download credits, or I could put it up as an alternative; your choice.

Your thoughts on the proposal?

[ryobg]

This PR looks good to me in general and I can make a release:

• Have you built enough confidence in testing that patch?
• A minor inconvenience is that the code currently waits always at least for 1 second before proceeding, despite knowing the first result already.

[rfortier]

The original intent was to add the break so it wouldn't, but it didn't seem to matter for something that happens once at startup. If there are real-world cases that actually do repeatedly open containers, I can update.

To your other question, the forked version has been bundled in Skyrim Together and SkyrimSE mod packs that collectively have at least O(1000), maybe O(10K) downloads. Zero bug reports. So yes, I'm confident it does what it says.

To be ultra-conservative, though, there are plenty of other uses cases for JContainers than the ones I built the PR for. 64-bit luajit still says it is "beta," though the mileage we've gotten on it says it is pretty solid. If paranoid there's still some bug in 64-bit luajit bug that isn't in 32-bit, the conservative approach would be to build both versions, leave 32-bit as the default with a note to use the 64-bit version if you get the error. The ONLY downside of this is people who get random crashes in 32-bit JContainers (that happen later and don't get the startup error) getting frustrated.

I wouldn't suggest that ultra-conservative approach but for this fact: almost every mod build of SkyrimSE needs JContainers. Mod packs keep getting bigger, and eventually pretty much every mod pack of size is going to need the 64-bit version. With that dichotomy and a reputation to protect, some skepticism of this random, intrusive change being tossed over the wall is at least understandable if not warranted. Publishing both versions isn't crazy.

So that's 2 potential improvements; remove the delay, dual-build support.

[ryobg]

Okay, thank you for the elaborate answer. When you fix that 1 sec update, I will make a release and we will see how it goes.

[rfortier]

Should be all set, other than I'm not sure what AppVeyor is complaining about.

I squashed the commits since it was getting too messy to review. I also, just to be certain, built a 32-bit test version to make sure it still worked too, and got this:

reopen_if_closed() luajit initial memory allocation failed, retrying for up to 3m. This only happens when LUA is compiled in 32bit mode. Or if you are legitimately out of memory, rare with modern computers

reopen_if_closed() delayed 24 seconds trying to allocate memory

[rfortier]

Just noticed one more thing: almost certainly Fixes #122

The jerky slowdown reported is common when you have the problem the PR addresses.

[ryobg]

Thanks. It should stay version 13 though? It was never released.
Anyway, that CI issue looks like failing here:

JContainers/tools/build_boost.bat

Line 23 in 27500de

"%~dp0\7za.exe" rn "%~dp0\boost.7z" boost_1_67_0 boost

which most likely means that the 7za.exe tool cannot handle a new archive format? It might work with newer https://7-zip.org/a/7zr.exe, but frankly, I'm not willing to spent anymore time on this now.

[rfortier]

I tried to do that, but I wanted a different version tag so people would know it (the fork) was updated.

I could not find the place the build number was being set, though. If you can point that out, I can set the build number instead of the minor version number.

[rfortier]

Answering my own question, I looked into Appveyor, your AppVeyor account generates the build number so there is little control.
So I'll set it back to .13, make up a (small) build number tag, and ask you make sure the build number is at least as large.

Thanks!

[rfortier]

Updated. I "pretended" the build number was 2, so as long as the AppVeyor build is >= 3 everything stays unambiguously that your version is newest.