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

Support for user aligned types #38

Closed
krisr opened this issue Oct 24, 2014 · 9 comments
Closed

Support for user aligned types #38

krisr opened this issue Oct 24, 2014 · 9 comments

Comments

@krisr
Copy link
Contributor

krisr commented Oct 24, 2014

I would like to patch LuaIntf to support user aligned types. At the moment, the alignment of types is ignored as a result of the way placement new works. User aligned types are common in games where SIMD instructions are useful. I worked around this for a while by patching LuaIntf to allocate user aligned types directly via new, and just store their pointers in the user data. However, I think it would probably be better to use something like alignof to dynamically allocate extra memory and offset it so as to avoid the additional memory allocation.

@krisr
Copy link
Contributor Author

krisr commented Oct 24, 2014

Ha, this ended up being really simple to fix cleanly:
#39

@SteveKChiu
Copy link
Owner

Cool, the solution has been merged

@krisr
Copy link
Contributor Author

krisr commented Oct 25, 2014

@SteveKChiu

I should have mentioned that my fix only works if the user also remembers to define LUAI_USER_ALIGNMENT_T to a type that is at least as large as alignof(T). I thought about ways to avoid this, but they all require reserving extra space in the m_data array so that the data can be properly aligned for T inside CppObjectValue, but some people may only want to do that when alignof(T) > alignof(L_Umaxalign) to save space. It's possible to reserve the space conditionally at compile time, however that would require that L_Umaxalign be available which would require inclusion of lua's llimits.h which is not included by default when you include LUA :(.

As a result, I think for now at least, my solution is probably sufficient for most use cases. Most people do not need alignment beyond 16bytes, so it's probably ok for them to have to require that all lua user data be aligned to 16bytes by defining LUAI_USER_ALIGNMENT_T. It would be great if LuaIntf figured all this out automatically, but it's probably not worth the additional complexity of requiring users to include llimits.h. Do you agree? If not, I'll happily change the code to assume llimits.h is included and automatically adjust the alignment. If you agree with the current behavior, I'll document it in the README.

@SteveKChiu
Copy link
Owner

I think update the document is good enough, and 16 bytes alignment should be fine for most cases.

@SteveKChiu SteveKChiu reopened this Oct 26, 2014
@SteveKChiu
Copy link
Owner

On second thought, I think the runtime solution is better, and I don't think L_Umaxalign is needed at all.

To my understanding sizeof(L_Umaxalign) is likely == 8 in most system, I am not sure where is 16 bytes alignment comes from, but it is probably due to sizeof (userdata header). Anyway, increase LUAI_USER_ALIGNMENT_T has far more overhead than CppObjectValue, and add the difficulty to explain the issue.

Here is what I plan to do, I need your suggestion:

template <typename T>
class CppObjectValue : public CppObject
{
private:
    CppObjectValue()
    {
        if (PADDING > 0) {
            uintptr_t align = reinterpret_cast<uintptr_t>(&m_data[0]) % alignof(T);
            if (align > 0) align = alignof(T) - align;
            assert(align < PADDING);
            m_data[sizeof(m_data) - 1] = (unsigned char)align;
        }
    }
...
    virtual void* objectPtr() override
    {
        if (PADDING == 0) {
            return &m_data[0];
        } else {
            return &m_data[0] + m_data[sizeof(m_data) - 1];
        }
    }

private:
    static constexpr int PADDING = alignof(T) <= alignof(void*) ? 0 : alignof(T) - alignof(void*) + 1;
    unsigned char m_data[sizeof(T) + PADDING];
};

edit: PADDING and align calculation, see the next comment

@SteveKChiu
Copy link
Owner

The PADDING definition should use alignof(void_), I forget to take virtual class into account. I am sure the userdata should be at least alignas(double), but with virtual class the m_data might not be alignas(double) but it should always at least alignas(void_)

    static constexpr int PADDING = alignof(T) <= alignof(void*) ? 0 : alignof(T) - alignof(void*) + 1;

@krisr
Copy link
Contributor Author

krisr commented Oct 26, 2014

Hey Steve, yes that sounds right. Your code is very close to one solution I played around with (except I was using sizeof(L_Umaxalign) and std::align). L_Umaxalign is defined as sizeof(union { double u; void *s; long l; }), so I agree it should be 8 bytes on most systems.

I'm not sure I see why you need the " + 1" in your PADDING expression?

If you haven't already, I will happily test this change since my app requires it.

@SteveKChiu
Copy link
Owner

The +1 PADDING is to store the padding offset, so the objectPtr() can get the aligned pointer by:

return &m_data[0] + m_data[sizeof(m_data) - 1];

And yes, please test this implementation, thanks.

SteveKChiu added a commit that referenced this issue Oct 27, 2014
@krisr
Copy link
Contributor Author

krisr commented Oct 27, 2014

The change works well on my machine. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants