Skip to content

Patch conditional compilation #9

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 4 commits into from
Feb 5, 2018
Merged

Conversation

arekbulski
Copy link
Member

@arekbulski arekbulski commented Jan 22, 2018

Most important change: in project settings, a flag similiar to DEBUG and TRACE gets computed for IsLittleEndian. The code actually compiles conditionally on it. Better performance.

Someone needs to run test suite on it, I dont have it set up.

@LogicAndTrick
Copy link
Collaborator

A few questions:

  • What happens if the user pulls in the pre-compiled DLL as a Nuget package if their architecture differs from the build machine?
  • What happens if the user adds the uncompiled .cs files to their own project and they don't have the LITTLEENDIAN flag set up on their project?

@arekbulski
Copy link
Member Author

  • I have no idea how NuGet works. Impossible for me to say.
  • That would cause a bug. I will fix the code so it detects this situation.

@GreyCat
Copy link
Member

GreyCat commented Jan 23, 2018

That's probably not a matter of how NuGet works, but how .NET assemblies work. As far as I understand, .NET assemblies are essentially CPU-neutral (setting compiler target for x64, x86 or arm doesn't seem to affect anything?), and CIL-to-machine code compilation is done during installation / loading of the DLL. Thus probably we can't do that in compile time.

@arekbulski
Copy link
Member Author

We can adjust the code so that it has 3 branches, little big and default (current code). NuGet would use default but the user could take advantage of this feature if he adjusts his .csproj.

@GreyCat
Copy link
Member

GreyCat commented Jan 23, 2018

I have very vague idea of how .NET assemblies work, so it is really up to you guys to decide. If I'm right about the fact that they are essentially architecture-indepedent, then I'd vote to stick with current run-time detection routines, and, indeed, put alternative versions in the branches.

@arekbulski
Copy link
Member Author

I have a very vague idea how assemblies compile as well, but as far as I understand it, even platform-independant code does not keep multiple versions per compile symbols. Ergo, it prunes the code during (pre)compilation.

I will fix it.

Copy link
Contributor

@Arlorean Arlorean left a comment

Choose a reason for hiding this comment

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

Instead of conditional compilation if we cache the bool locally on the KaitaiStream class:

static readonly bool IsLittleEndian = BitConverter.IsLittleEndian;

Then use that in ReadBytesNormalisedLittle/BigEndian then the JIT compiler optimizes the Array.Reverse out when it doesn't apply. That way we get one nuget dll but optimal performance on all platforms.
Here is ReadBytesNormalisedLittleEndian() on my machine, no if check or Array.Reverse():

           byte[] bytes = ReadBytes(count);
038A0F50  mov         eax,edx  
038A0F52  sar         eax,1Fh  
038A0F55  push        eax  
038A0F56  push        edx  
038A0F57  call        038A0D90 
038A0F5C  ret 

@arekbulski arekbulski self-assigned this Jan 27, 2018
@arekbulski arekbulski force-pushed the patch-conditionalcompilation branch from 021331a to 632bde8 Compare February 4, 2018 14:34
@arekbulski
Copy link
Member Author

@Arlorean
Could you check last commit and confirm its RTM, please?

Copy link
Contributor

@Arlorean Arlorean left a comment

Choose a reason for hiding this comment

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

This looks perfect!

@GreyCat GreyCat merged commit e8bb20a into master Feb 5, 2018
@arekbulski arekbulski deleted the patch-conditionalcompilation branch February 5, 2018 16:23
pluskal pushed a commit to pluskal/kaitai_struct_csharp_runtime that referenced this pull request Oct 1, 2020
Resolve "Add CancellationToken support."

Closes kaitai-io#9

See merge request marta/kaitai_struct_csharp_runtime!19
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants