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

Fix storage encoding #3130

Merged
merged 5 commits into from
Feb 10, 2024
Merged

Fix storage encoding #3130

merged 5 commits into from
Feb 10, 2024

Conversation

shargon
Copy link
Member

@shargon shargon commented Feb 8, 2024

Description

We wrote the id in the OS format, but we always read it in LittleEndian

Id = BinaryPrimitives.ReadInt32LittleEndian(cache);

Type of change

  • Bug fix (non-breaking change which fixes an issue)

@shargon shargon added the bug Used to tag confirmed bugs label Feb 8, 2024
@shargon shargon requested a review from Jim8y February 8, 2024 18:05
@shargon shargon added the critical Issues (bugs) that need to be fixed ASAP label Feb 8, 2024
@shargon shargon marked this pull request as draft February 8, 2024 18:11
@@ -29,8 +30,22 @@ public class KeyBuilder
/// <param name="prefix">The prefix of the key.</param>
public KeyBuilder(int id, byte prefix)
{
Add(id);
Copy link
Member Author

Choose a reason for hiding this comment

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

@Jim8y The bug was here, it was stored in the OS format, but it was readed in LittleEndian, in our test is the same, but in a BigEndian machine, it will fault.

Copy link
Member

Choose a reason for hiding this comment

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

Its still wrong. You need to check the machine. Use

if (BitConverter.IsLittleEndian)
    Id = BinaryPrimitives.ReadInt32LittleEndian(cache);
else
    Id = BinaryPrimitives.ReadInt32BigEndian(cache);

@shargon shargon marked this pull request as ready for review February 8, 2024 18:29
@cschuchardt88
Copy link
Member

cschuchardt88 commented Feb 8, 2024

Every machine OS is different. Some use little and some use Big you need to do an OS check.

Copy link
Member

@cschuchardt88 cschuchardt88 left a comment

Choose a reason for hiding this comment

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

Need more tests. @superboyiii needs to check states after this is finished.

@shargon
Copy link
Member Author

shargon commented Feb 9, 2024

Every machine OS is different. Some use little and some use Big you need to do an OS check.

Why? We need to write always in the same way, that's why I removed the unsafe methods

@shargon
Copy link
Member Author

shargon commented Feb 9, 2024

Need more tests. @superboyiii needs to check states after this is finished.

In big endian machine the states will change, but it becomes to be the same as little endian.

Note: no one use it in BigEndian, because it didn't work.

@cschuchardt88
Copy link
Member

cschuchardt88 commented Feb 9, 2024

Need more tests. @superboyiii needs to check states after this is finished.

In big endian machine the states will change, but it becomes to be the same as little endian.

Note: no one use it in BigEndian, because it didn't work.

The reason it didn't work because the OS stores in BigEndian. So you have to write it in the same way. If you do a check if the OS is Big or Little you can use the right one. Instead of forcing one on a developer.

if (BitConverter.IsLittleEndian)
    Id = BinaryPrimitives.ReadInt32LittleEndian(cache);
else
    Id = BinaryPrimitives.ReadInt32BigEndian(cache);

Also if the OS is big it will always write the data to disk or memory as big even if its written in little.

Copy link
Contributor

@roman-khimov roman-khimov left a comment

Choose a reason for hiding this comment

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

The fix itself is correct, maybe we can improve Add and constructor documentation though to mention endianness.

Is LevelDB binary format endian-agnostic, btw?

@shargon
Copy link
Member Author

shargon commented Feb 9, 2024

Instead of forcing one on a developer.

We want deterministic storage, no sense to store in different format in different os.

@cschuchardt88
Copy link
Member

cschuchardt88 commented Feb 9, 2024

Also if the OS is big it will always write the data to disk or memory as big even if its written in little.

So when you go to read it would be in big again? I'm pretty sure. I don't have machine to check. Machine won't use odd address spaces. ONLY even address spaces. Its a hardware thing. But maybe the OS sets a bit or something for if it was written in little or big. I'm not 100% sure on that though. I do know i had problems before. Where you would write it as little and when you read it, the data would be in Big again.

@Jim8y
Copy link
Contributor

Jim8y commented Feb 10, 2024

How about banning BigEndian machine once for all. Now its the world of little endian. Kill the bigendian from neo.

If none of you can not even find a Bigendian machine to test, what is the debating means here.

Just little endian everywhere.

@Jim8y
Copy link
Contributor

Jim8y commented Feb 10, 2024

if (BitConverter.IsLittleEndian){throw exception "sorry, we dont like bigendian"}

@cschuchardt88
Copy link
Member

Nevermind after checking dotnet does it no matter what.

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static void WriteInt64BigEndian(Span<byte> destination, long value)
{
    if (BitConverter.IsLittleEndian)
    {
        long tmp = ReverseEndianness(value);
        MemoryMarshal.Write(destination, ref tmp);
    }
    else
    {
        MemoryMarshal.Write(destination, ref value);
    }
}

@shargon shargon merged commit b4dadcb into master Feb 10, 2024
1 of 2 checks passed
@shargon shargon deleted the fix-storage-enconding branch February 10, 2024 13:35
@roman-khimov roman-khimov added this to the v3.7.0 milestone Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Used to tag confirmed bugs critical Issues (bugs) that need to be fixed ASAP waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants