fNbt icon indicating copy to clipboard operation
fNbt copied to clipboard

Remove the need for unsafe code

Open AjaxGb opened this issue 8 years ago • 5 comments

Unsafe code was only used in three places in the NbtBinaryWriter class: once each to cast floats to uint bits and doubles to ulong bits, and once to use this method. I have changed the casting to make use of struct member overlap, and there is an almost identical method that uses safe code.

These changes should make compiling easier (they did for me).

AjaxGb avatar Mar 15 '16 18:03 AjaxGb

Looks good! I'll check for performance impact of these changes, and merge it if there are no problems.

mstefarov avatar Mar 27 '16 23:03 mstefarov

Alright, I did some profiling. Changes to Write(float) and Write(double) don't cause any problems, but changes to Write(string) cause a 25% increase in CPU time, as well as doubling the amount of heap allocation. That added value.ToCharArray() call is pretty expensive.

Under-the-hood in the .NET Framework, EncoderNLS.GetBytes(Char[]...) simply proxies to EncoderNLS.GetBytes(Char*...) anyway. Is there really a good reason not to stick to the current, "unsafe" but efficient implementation of NbtBinaryWriter?

mstefarov avatar Mar 28 '16 04:03 mstefarov

fNbt Issue 20 profiling results in dotTrace

Here are the profiling results by the way, before/after.

mstefarov avatar Mar 28 '16 05:03 mstefarov

Nah, that sounds reasonable.

AjaxGb avatar Mar 28 '16 06:03 AjaxGb

FYI, I just forked the project and had absolutely no problems getting it to compile.

NiclasOlofsson avatar Oct 01 '16 20:10 NiclasOlofsson