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

SBE decoding for composite types inefficiency #599

Open
kdoherty2 opened this issue Oct 9, 2018 · 6 comments
Open

SBE decoding for composite types inefficiency #599

kdoherty2 opened this issue Oct 9, 2018 · 6 comments

Comments

@kdoherty2
Copy link

kdoherty2 commented Oct 9, 2018

SBE for composite types generates full scale wrappers with all the nested fields.
For example from the generated code for delegate:

Here is a message with composite field common:

class MsgOrderNew
{
private:
    OrderCommon m_common;

public:
    OrderCommon &common()  
    {       
        m_common.wrap(m_buffer, m_offset + 0, m_actingVersion, m_bufferLength);
        return m_common;     
    }
};

class OrderCommon
{ 
    private:
     char *m_buffer;
     std::uint64_t m_bufferLength;
     std::uint64_t m_offset;
     std::uint64_t m_actingVersion; ……. 
};

So for every nested composite field (even if this composite contain only one primitive field) you get extra 32 bytes in SBE wrapper.

GCC can’t optimize the memory layout. All these fields must be initialized.
Also in a majority of cases GCC can’t inline the whole chain of calls down to individual atomic field within the message.

@VSol-ll
Copy link

VSol-ll commented Oct 11, 2018

Here is a piece of code to play with: https://godbolt.org/z/S8x7Rw.
It contains commented line at the very bottom -- un-commenting will lead to bloating the generated code. Also it contains macro ALL_METHODS_ATTRIB which can be modified to [[gnu::always_inline]].

As a part of this issue we can discuss the following optimizations:

  1. validate the buffer size at reset() and don't do this in nested wrappers. Remove the m_bufferLength field.
  2. merge m_buffer and m_offset -- as far as I understand, wrapper should never access binary data before and after its part.
  3. remove m_activeVersion from nested wrapper -- user can validate it on its own if needed.
    As a result the only required member field for the wrapper is m_buffer.

Maybe there're some SBE features which are not compatible with this approach. But usual C++ way is not paying for things you don't use.

Another part could be adding configurable attributes to methods.

@tmontgomery
Copy link
Contributor

wrt having configurable attributes to methods, please submit a different issue for adding that. I think that is useful and we can track it separately.

For composites, I think we can do better. Here are my thoughts.

  • offset can be removed. Also, removing the offset() method.
  • actingVersion might be removable. The current SBE spec does not allow adding fields to composites during versioning. I am unsure about 2.0, though. @mjpt777 does 2.0 plan to add the ability to extend composites?
  • bufferLength can be made conditional and only used when bounds checking is on. However, it is currently needed for nested types (composites, sets, enums, etc.) due to how the types may (or may not) be used in a nested way or directly in a message.

@tmontgomery
Copy link
Contributor

One option for preserving bounds checking, but removing the need to keep bufferLength is to add a specific wrap mechanism for MessageHeader types that will do bounds checking, but to remove bounds checking from the normal path where it is somewhat redundant.

@daswass
Copy link

daswass commented Jul 23, 2020

Has there been any though on this topic since the last post? Any plans to make changes or resume the discussion?

@tmontgomery
Copy link
Contributor

Nothing we've had time to look into.

@mjpt777
Copy link
Contributor

mjpt777 commented Jul 24, 2020

Given the constructors we now have on composites and bit sets we could not keep them as members and return them by value rather than by reference. This could optimise and inline better locally.

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

No branches or pull requests

5 participants