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

[WIP] luawrapper: work with luajit on ARM64 #6512

Closed
wants to merge 8 commits into from

Conversation

Habbie
Copy link
Member

@Habbie Habbie commented Apr 17, 2018

Short description

Lua lightuserdata on ARM64 with LuaJIT is limited to addresses that can fit in 47 bits. This PR removes all usage of lua_pushlightuserdata from our copy of Luawrapper. The ValueInRegistry and plain function pointer changes are mostly harmless; the type index changes might have some performance impact.

This makes the dnsdist test suite pass on Linux/ARM64.

PRing this now for reviews and comments. If it turns out the type index changes have real performance impact, I will consider making that a compile time choice.

When force applied to upstream Luawrapper, this breaks compilation of the custom_types, movable and metatables test suite components. Fixed in Revert "no longer need this"

TODO

  • deal with the last remaining lua_pushlightuserdata call (in template<> struct LuaContext::Pusher<const std::type_info*>) - either make sure we never call it, or fix it. Just removing that template causes the upstream LuaWrapper failure mentioned above. I checked this box after adding an assert to that call, proving we do not rely on it.
  • update comments
  • check performance impact of these changes

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled and tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)
  • checked that this code was merged to master

@Habbie
Copy link
Member Author

Habbie commented Apr 19, 2018

Travis and Buildbot pass this as of 5a01fb3

@Habbie
Copy link
Member Author

Habbie commented Apr 20, 2018

This is ready for review, and, as far as I'm concerned, merge to master after some squashing.

Copy link
Member

@rgacogne rgacogne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good. Am I understanding correctly that our current FFI API will not work on ARM64 or anything else that uses more than 47 bits of addresses?

@Habbie
Copy link
Member Author

Habbie commented Apr 20, 2018

@Habbie
Copy link
Member Author

Habbie commented Apr 20, 2018

Code looks good. Am I understanding correctly that our current FFI API will not work on ARM64 or anything else that uses more than 47 bits of addresses?

No, it's only lightuserdata that is affected by the 47 bit limit, as far as I can tell.

@rgacogne
Copy link
Member

No, it's only lightuserdata that is affected by the 47 bit limit, as far as I can tell.

But isn't that what we use for our pdns_ffi_param type, to maximize performance and because I couldn't get FFI to work when the type was wrapped by LuaWrapper?

@Habbie
Copy link
Member Author

Habbie commented Apr 20, 2018

But isn't that what we use for our pdns_ffi_param type, to maximize performance and because I couldn't get FFI to work when the type was wrapped by LuaWrapper?

Oh, yes. We'll have to look into that.

@rgacogne
Copy link
Member

Benchmarks look good, and as far as I understand it, it won't break the existing recursor FFI API on x86_64 so I'd be happy to merge this once you are done with it. I think you mentioned wanting to do some squashing first?

@Habbie
Copy link
Member Author

Habbie commented Apr 25, 2018

But isn't that what we use for our pdns_ffi_param type, to maximize performance and because I couldn't get FFI to work when the type was wrapped by LuaWrapper?

Confirmed broken with or without this branch.

@Habbie
Copy link
Member Author

Habbie commented Apr 30, 2018

Benchmarks look good, and as far as I understand it, it won't break the existing recursor FFI API on x86_64 so I'd be happy to merge this once you are done with it. I think you mentioned wanting to do some squashing first?

The one ARM64 user we talked to is happy for now, without this merged (but with this sitting here!). I'd like to test/think about performance some more before we put this in.

@ahrex
Copy link

ahrex commented Sep 15, 2018

@Habbie, is there anything we can do to assist? I'm working with ARM64 at the moment, and I would prefer to use this patch over swapping LuaJIT for Lua wholesale.

Thanks!

@Habbie
Copy link
Member Author

Habbie commented Sep 18, 2019

@Habbie Habbie closed this Sep 18, 2019
@Habbie Habbie mentioned this pull request Sep 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants