Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

luajit: add version with gc64 enabled #77800

Closed
wants to merge 1 commit into from
Closed

luajit: add version with gc64 enabled #77800

wants to merge 1 commit into from

Conversation

@Quiark
Copy link
Contributor

@Quiark Quiark commented Jan 16, 2020

Motivation for this change

Add a build variant of this which helps resolve some issues in Minetest (package coming in later)

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
@vcunat
Copy link
Member

@vcunat vcunat commented Jan 16, 2020

I haven't investigated GC64 details, but it's like preparing for this upstream change.


Well, I'm a bit doubtful about releases happening from that repo anymore, but that doesn't seem important for this PR... which leads me to: I've been considering adding a package for a fork into nixpkgs and I have been testing it a bit locally for quite some time, but altogether it feels like we'd be adding a bit too many variants there, especially if we wanted to add lua*Packages variants (those are four currently).

@teto
Copy link
Contributor

@teto teto commented Jan 16, 2020

there are a few contenders like https://github.com/raptorjit/raptorjit as well. From what I heard some distribs may replace luajit with a fork but then luajit development may pickup some of the recent changes as well. So maybe wait and see ?

@vcunat
Copy link
Member

@vcunat vcunat commented Jan 16, 2020

raptorjit probably not (generally), as that was cut down to x86_64 only IIRC, for example it won't work on aarch64.

@lblasc
Copy link
Contributor

@lblasc lblasc commented Jan 20, 2020

From my experience enabling LJ_GC64 on beta3 release will cause random failures. Only way to have stable luajit with LJ_GC64 is to use recent 2.1 branch or some of the forks moonjit/OpenResty's luajit2) they have lot of stability patches for both aarch64 and LJ_GC64.

My suggestion is to update luajit_2_1 to latest commit in 2.1 branch (LJ_GC64 is enabled by default). Then we can enable aarch64 builds and solve luajit memory problems (>2G on x86_64).

@vcunat I would love to have moonjit fork as a alternative interpreter available in the tree.

@vcunat
Copy link
Member

@vcunat vcunat commented Jan 20, 2020

At my job we've done some brief testing of 2.1 beta3 on aarch64 (which only supports LJ_GC64 mode IIRC) and it seemed fine*. I believe it's sufficient to have the builds on Hydra, and they do build already (example). Still, I'm sad to hear that reliability isn't good. Luckily, to me aarch64 isn't that important, at least for now.

* Except for the well-known trouble with light userdata rejecting some pointers, but we worked around that somehow.

@vcunat
Copy link
Member

@vcunat vcunat commented Jan 20, 2020

With moonjit, I'm still a bit unsure in what way the integration with OpenResty's luajit2 fork will happen (if at all, perhaps this). I'd hope that we can reduce the number of luajit versions, e.g. to: original 2.0, original 2.1, "new" 2.1 and "new" 2.2 (with incompatible changes).

@lblasc
Copy link
Contributor

@lblasc lblasc commented Jan 20, 2020

I haven't used LuaJIT on aarch64, nothing I can add here except I saw loads of changes/fixes after 2.1-beta3 release. On x86_64 luajit 2.1-beta3 with LJ_GC64 is unusable, it event returns wrong values in the tables when mem usage goes over 2GB. I've experienced this on production setup and it was a big headache, I would like to spare others from this experience :)

It is a bummer that Mike (LuaJIT author) did latest release in 2017, I would very much like recent LuaJIT 2.1 tagged and released in any way. But reality (at least mine :) ) is that there was recent activity on 2.1 branch which pulled all the relevant fixes and using it by default in luajit_2_1 should be safe option, only exception is problem with light userdata in LJ_GC64 mode which @vcunat mentioned, but I would still go with LJ_GC64 enabled and fix packages which have problems with light userdata.

I hope fork situation will crystallize soon.

@vcunat
Copy link
Member

@vcunat vcunat commented Jan 20, 2020

Updating the default from beta3 to latest 2.1 commit would hopefully be safe enough, except for that triggering LJ_GC64 by default, I'm afraid. Well, even 2.0 has seen many fixes since the last release (2.0.5, also May 2017).

I feel like we don't want to have LJ_GC64 on the default luajit 2.1 version yet. I suspect only the most active lua packages have patched their light userdata. For now upstream supports turning it off by -DLUAJIT_DISABLE_GC64. Well, this PR actually started with the intention to test the LJ_GC64 mode separately for now.

I also hope the situation with forks and releases will get clearer soon.

@lblasc
Copy link
Contributor

@lblasc lblasc commented Jan 20, 2020

Maybe enabling LJ_GC64 by default is to radical for now.

I suggest the following plan:

  • update luajit_2_1 to latest 2.1 branch and build it with -DLUAJIT_DISABLE_GC64 by default
  • add flag in luajit_2_1 package E.g. gc64 so packages like Minetest can use LJ_GC64 variant easily
  • no need to have special variant luajit_2_1_gc64 (aarch64 is always LJ_GC64, so lualijt_2_1_gc64 and luajit_2_1 will be the same, it will confuse users, in future we maybe switch LJ_GC64 to default on all supported platforms)

@vcunat what do you think ?

@vcunat
Copy link
Member

@vcunat vcunat commented Jan 20, 2020

Sounds good to me. @Quiark: you didn't have any specific use case for explicit luajit_2_1_gc64, right? One difference is that it would be built on Hydra (x86_64 version with gc64), but this by itself is cheap to build anyway.

@Quiark
Copy link
Contributor Author

@Quiark Quiark commented Jan 21, 2020

OK for me, I didn't know that packages can have options

@vcunat
Copy link
Member

@vcunat vcunat commented Apr 26, 2020

Implementation of that plan: #85483 (so you get notified of this)

@vcunat
Copy link
Member

@vcunat vcunat commented May 1, 2020

Merged that one.

@vcunat vcunat closed this May 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.