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

Investigate potential alignment issue #140

Open
bjornharrtell opened this issue Sep 5, 2021 · 6 comments
Open

Investigate potential alignment issue #140

bjornharrtell opened this issue Sep 5, 2021 · 6 comments

Comments

@bjornharrtell
Copy link
Member

See dvidelabs/flatcc#210 (comment).

It is likely that this disallows zero-copy reading of invidivual buffers from a stream. I think we circumvent it everwhere now by copying buffers out from the stream into a new chunk of aligned memory but this is not optimal.

I believe it should be possible to tail pad the buffers without breaking backward compatibility. However, I would need to bump the format version a "patch" level to enforce it so that reader implementations can take advantage of it. And I think existing reader implementations doesn't have the foresight to read higher "patch" levels of the format so that should be introduced ASAP.

@bjornharrtell bjornharrtell self-assigned this Sep 5, 2021
@bjornharrtell
Copy link
Member Author

Discussed today and proper flatbuffers are always aligned to the minimum stored scalar in the buffer (size prefixed or not). There is a bug in flatcc in that it does not adhere to this.

But the problem remains due to flatgeobuf schemas having only 8 byte scalars in vectors and they can be left unset in which case the buffers can be aligned to 4 bytes instead. To avoid this in the C++ implementation of flatbuffers one can force min alignment to the desired size by fbb.TrackMinAlign(8).

@pka
Copy link
Member

pka commented Sep 7, 2021

Can't find this setting in Rust https://docs.rs/flatbuffers/. So this should be tested in a Rust writer implementation.

@bjornharrtell
Copy link
Member Author

@pka probably deserves a missing feature issue at https://github.com/google/flatbuffers. I will check if it's the same situation with TS, Java and C# implementations. I currently struggle with the C implementation in general but that's another story...

@bjornharrtell
Copy link
Member Author

Also notable is that the GDAL implementation was made with forsight on format patch levels. It only verifies the first four bytes of the magicbytes. I've already made the specification change noted here https://github.com/flatgeobuf/flatgeobuf#specification and will when I get time ensure that my reference implementations do not get stuck on patch level 0.

@bjornharrtell
Copy link
Member Author

Just to be clear - this does not cause issues with any of existing implementations AFAIK. It can potentially cause alignment related issue if a reading implementation attempt to read a feature straight from an offset in flatgeobuf in memory.

@bjornharrtell
Copy link
Member Author

With #146 all impl. are lenient on patch level but only C++ can produce aligned (patch level 1) versions so this will have to remain open.

@bjornharrtell bjornharrtell removed their assignment May 1, 2024
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

2 participants