Sunday, December 25, 2016

Wow, 15 years of Halo

Wow. Just realized that it's been 15 years now since I first got an Xbox and with it, Halo. I think it was bundled with a few other games too (Munch's Oddysee, an NFL game, Project Gotham Racing, and DoA3 I think), but I'm focusing on Halo today.

I think I actually got the console+games bundle before Xmas, but still in the month of December. I wouldn't have been able to really play it until winter break started anyway. I remember looking at the box thinking "hmmm, this looks cool...I wonder if it will be anything like Jet Force Gemini? I loved that game"

GoldenEye and Jet Force were the bread and butter of my N64 experience. Around that same time I would have probably been playing Driver 2. Earlier in 2001 I had moved from a different state and with that event I branched out to more single player games since I didn't have my old friends to play co-op with in legends like GoldenEye.

Anyway, I was looking at Halo's DVD case thinking "hmmm...those things [Grunts] kinda resemble the Ants in Jet Force. Why am I not playing yet??". So I finished setting up the Xbox and inserted the game. Splash screens roll by, main menus are briefly hit, and finally, a new campaign is started.

With the game's first cutscene I pretty quickly forgot the "is this like Jet Force" question/mindset. I was pretty amazed at what I was seeing, given that everything else I had played before was on an N64 or PS1. While I did play PC games too, the last interesting title I had played before then was a Star Wars Sith Lords game or Thief Demo from an EGM disc.

However, Halo's cinematics were just amazing and looked pretty. It had all my attention. I played through the first mission. More! Played through the second mission. Moar! Played through the third- "wait...are there no boss fights?". I realized that in the back of my mind I still had some hopes/expectations of what I experienced in Jet Force to appear in this game. One of those things being boss fights. The realization, for a moment, felt weird: "No boss fights?". But that quickly faded. "No boss fights!" It was back to shooting what I now knew as Grunts and Elites.

Now, if you're reading this you've more than likely played the first Halo. After all, it's been released and re-released four times now on four different platforms. So unless you are some PlayStation exclusive nerf herder, you probably already know that midway through the game a new foe is introduced: The Flood. At the time of course, none of us knew or anticipated this reveal.

Let me first paint a better picture of the environment I was playing this game in: I was in North East America, it was winter, cold (probably snowy), middle of the night, and I was in the basement of the house I lived in (practically another floor, since it was carpeted, mini bar, and with a sliding glass door to the the outside). Oh and the house was basically built into the side of a hill with thick woods. In a town where your neighbors were a hike away.

So yeah, I was already feeling quite isolated and susceptible to things too spooky. Then the game goes "here's The Flood, have fun!". My adrenaline was flowing. It was awesome. It probably helped that I was still middle school age too. No game or movie really hit me to such great affect after that. Well, expect maybe Signs. Only because I had scarred myself to the concept of "little green men" back in the 90s with the abduction/UFO shows that aired on TLC (you know...back when it didn't just air reality garbage) and Discovery Channel, plus due to some other events. I want to believe.

I'm fairly certain I finished the game in one sitting/night. While I had finished the game, I was by no means finished with it. I replayed it over and over. I would go to a friend's house and use GameSpy to tunnel mulitplayer matches. I discovered warthog jumping (warthog jumping revisited also needs mentioning).

It wasn't until I was in high school that the Action Replay came out for Xbox. Back in my N64 days I was a big GameShark user. I'd go online, find codes for games, then randomly start changing parts of the codes in hopes of getting something even cooler/different. I seem to recall the GameShark also coming with a VHS that explained basic hexadecimal, among other things. Sadly, I soon realized that the Action Replay was only a device for transferring game saves to/from the console and PC. I couldn't use it to modify a game's memory on the Xbox. But that wasn't the end of that.

Not wanting to let the investment go to waste, I used the device to transfer a Halo game save to the PC and opened it in none other than Notepad. At first anyway. The game used binary game saves, literally memcpy'ing the game state memory (always allocated at a fixed address) and flushing to disc. Before I would venture on to use Hex Workshop to view the bytes of the file, I scrolled what all I could read of the game save in Notepad. To my surprise, I saw help strings relating to the game's scripting engine (which was LISP inspired at the time, these days Halo and Destiny use Lua). This was due to how the engine "freed" datum in the game state memory. It would fill the datum blocks with random bytes from the executable itself. I didn't realize this was the reason until many years later when working on Halo Custom Edition.

All of this would eventually lead me to the Halo modding community and many, many other reverse engineering hijinks that would require their own post. It's kinda incredible to think that it's been 15 years now, longer than I had been alive by the time I first played Halo.

Incredible enough to make me take the time and write this blog post anyway. Perhaps I'll try firing up the Master Chief Collection and try playing Halo 1 again, this Xmas day? I say 'try' twice, because I'm uncertain if that game will function properly. It's too bad no one will do a technical post-mortem on that uber game. I'll probably get two or three missions finished before the nostalgia wears off and I realize I have better things to do with my time these days anyway :)

Thursday, April 14, 2016

One simple change removed heap allocations from our generic IO extensions

The more I use generics, the more I miss C++ templates (we'll just ignore the horrendous compile errors we can get with them when not using Clang).

While I was profiling our Unity project the other day I was wondering why, exactly, parts of our serialization extensions were popping up in our per-frame heap allocations report. While we don't do much serialization ops in client code, the server nodes do, so I felt it worthwhile to investigate.

I normally only profile in a non-development "release" build (although, I'm not certain Unity compiles assemblies with the /optimze+ flag). So by default I'm not seeing file/line info for everything since Unity doesn't give the option to force include the .mdb files (which our profiler tries to use when tracing allocations and other bits).

After making another build, this time with "Development Build" checked and after modifying the .pdb path in Unity's prebaked executable, I was up and running our memory profiled builds with full file/line info for managed and unmanaged code. The reports were showing that at line 330 of our extensions file was to blame for the curious heap allocations.

But how can I easily fix this near the end of a milestone and without going and changing a bunch of callsites? You can't specialize generic code based on the constraints alone, so I couldn't have two Read<T> methods which only differed from "where T:class/struct". I really didn't want to go and make, plus reeducate others to use, a ReadValue and ReadReference method when our Write<T> extensions don't require such verbose code. A few other suggestions were thrown at me, but I wasn't biting.

But wait...what about default arguments? What if I instead added a parameter based on T that uses its default value? This would allow me to avoid having to update a huge number of callsites, but what about still having to allocate reference objects? Turns out checking null is enough for them, while value types don't compare equally to null, so we can avoid the obj = new T() statement altogether for value types and use default arguments to avoid any other code fixup!


Yeah, I hate it when people post code in picture form too, so I put up the bits on gist.github too.

There can be a hidden cost here, but I factor anything GC related to be the biggest, nondeterministic, hidden cost. Specifically, the larger the T value type is, the more code can get generated to deal with copy-by-value semantics for the stack and return value. Although it is possible the runtime or native compiler could work some magic under the hood to use a reference to a stack value. It all depends! I'm not positive what Unity's Mono (IL2CPP would be easier to figure out) is doing under the hood for JIT'd code, but I do know we're no longer generating unexpected temporary garbage in our serialization code!

Tuesday, April 12, 2016

List.Reverse calls Array.Reverse, which may or may not box

You'd expect List<T>.Reverse to not box (ie, allocate memory on the managed heap) when you're using structs, right? Well, you would be wrong. And you'd expect List<T>.Reverse to not box when using basic builtin types like short, byte, right? Sometimes, you may be right.

This is the Array.Reverse implementation in Unity's Mono (some branch of 2.x). It won't box values when the array (List<T> uses an array internally) is an int[] or double[], but will on everything else (see line 1261).

This is the Array.Reverse implementation in the latest version of Mono. It won't box arrays of builtin types, but your enums and custom structs will still result in boxed values.

This is the Array.Reverse implementation in MS.NET's reference source. At the time of this writing, it will try to call the externally defined TrySZReverse function before going on to just box all teh things. Apparently, according to this API issue, that function is a fast-path for reversing primitive types (but your structs, and I'd imagine your enums, don't fall into that category). That API issue is up for review, so it could be that MS.NET will have sane generic Reverse'ing in the future with no boxing!

So yes, you'd expect that your .NET runtimes wouldn't box when performing List<T>.Reverse or Array.Reverse<T> when T is a value type, but you'd mostly be wrong most of the time.

Leave the boxing to Mike Tyson and Rocky!

Be mindful of Dictionary's keyed with enums. Also, the power of per-frame GC analysis!

If you’re going to use a Dictionary with enums in Unity, or any value types rather*, ensure you provide a custom (hopefully static) IEqualityComparer instance for it to use, instead of letting it use the default implementation. I’m not %100 sure about IL2CPP, but in Mono the default comparer (when used with value types) causes a separate heap allocation every time you do ContainsKey or use the Dictionary indexer (you should also probably ask yourself why you’re using ContainsKey/get_Item, instead of TryGetValue, too). The size of each BoehmGC heap allocation, not factoring for things like alignment, is (sizeof(void*) * 2) + sizeof(enum-underlying-type), which in our case was a byte so on 32-bit machines each allocation was 9 bytes.
* You have no choice when it comes to AOT environments like iOS when it comes to structs as keys
At my day job on an unannounced project, we had a particular enum to describe a set of hard coded things (let's call the enum HardCodedThing). Then a certain set of our source game data (let's call the class ProtoSomething) had two dictionaries that were keyed by this enum for various reasons. In our game's sim we have a method, we'll call it UpdateSomethings(), which does a lot of HardCodedThing indexing. The code is ran when a sim is deserialized or when some other state is derived from the sim.

For the past week or so, I've been adding per-frame memory analysis to the custom memory tools that I worked with Rich Geldreich to implement. I marked two custom events (strings which the game runtime sends to the memory profiler at specific points for later reference) as the start and end frames (or rather, the frames which the events took place in). I was interested, well worried, as to why we had so many GC allocations in general when just idling in the game. It turns out our sanity checks for Animator components having parameters before trying to set them was generating garbage (detailed later). It also just so happened that the sim state deriving I mentioned earlier was taking place shortly before the marked last custom event, so heap allocations for the HardCodedThing enum were bubbling up.

The fix to avoid all these allocations was to simply add a Comparer that is then passed in to our Dictionary<HardCodedThing, TValue> instances. The implementation, using the disguised typenames used in this article, can be found here.

Adding the per-frame analysis utilities to our tool has been EXTREMELY helpful (I found some other GC-heavy APIs specific to Unity that I'm sure we're not the only ones ignorant of...should blog about them soon). We're not stuck scratching our heads by what's making the stock Unity Profiler show "GC Allocations per Frame: 9999 / 12 KB" or some general nonsense. We really don't even bother using Unity's Memory profiler. Reason being is that it's just a snapshot. Snapshots have limited use. It's also far easier and quicker to make a Standalone build vs a device build, since their tool is IL2CPP only. We instead have a transaction log of all Boehm/Mono memory related operations from the start of the process until the very end. Coupled with our custom events setup we can create near-deterministic transaction logs over multiple runs. In theory, we could probably even setup a BVT to ensure we're not leaking managed memory or churning GC memory more than we already expect to.

And when/if we do, we have a boat load of information at our disposal to diagnose problems. Part of the per-frame analysis was to first spit out a CSV file mapping types seen across a set of frames to how many allocations and frees they were associated with, along with their average garbage (if alloc:free is near 1:1, you have garbage heavy code using that type somewhere!). You can find an excerpt of the CSV report here. I mentioned that we had code sanity checking Animators for parameters we are trying to set earlier, and here you can see a glimpse of how they were showing up in our reports.

With this broad report, the next thing I added was a 'blame list' report. The blame list details, per type, the sets of backtraces which are spawning these allocations. We then sort and break down these backtraces by their allocations, so the biggest offenders of garbage are at the top. You can find an excerpt of the blame list report here.

Perf matters. Memory usage matters. Tools matter. While the studio I work at probably won't open source our memory tool's full pipeline anytime soon for various reasons (although our Mono modifications can be found here), I'm hoping to use this blog to publicly speak more about bad/good patterns we see/learn from experience and verified using the tools we've developed.

Sunday, February 21, 2016

Exposing the Unity3D profiler window settings

Lately, I've been working on writing reverse engineering the internals of Unity's Profiler APIs and Window in hopes of doing my own viewer. This is part of the reason for the lack of new IL2CPP articles (although I do have some content I really want to cover, especially regarding virtual calls). Anyway, I want my own profile viewer.

A viewer that isn't limited to IMGUI patterns.

A viewer that isn't slow as hell (to the point of being unusable) when expanding CPU hierarchy nodes that are so many levels deep, especially in deep profiling (go jump off a bridge if you really think I have time to manage manual Sample traces, especially in code which is Unity agnostic, and the iteration times involved).

A viewer that can work on custom offline trace files. Which, maybe could provide ETL steps to spit out a .DTP version that is compatible with dotTrace. OK, so their formats don't appear to be open or documented, but you get the idea. One could even hack together a tool based off this Windows Heap profiler. Anything is possible when you have the raw data!

A viewer that is open source. Because one size does not fit all. If it did, I wouldn't be writing this blog post or reverse engineering the profiler!

Anyway, good progress is being made. I managed to get many of my LINQ utils working in the .NET framework used in Unity. Even uncovered some use cases that I didn't solve already in the process. It also helps that not all of the types used in the ProfilerWindow/APIs are marked internal. I expect to have some raw tools ready by early next month (probably in time for another round of profiling work at my day job).

But in the meantime, I'm serving appetizers!

Customizing the Profiler Window view

Yesterday, I happened to stumble across this programmer's tweet where he uses the PreferenceItem attribute to add custom Edit->Preferences options. Pretty cool! I then remembered how earlier I found (via ILSpy) that many of the Profiler Window's column's visibilities can be tuned by some EditorPrefs settings. Not long after, I was able to get the following settings working:


I can finally tune just how much BS is being displayed in the Profiler! Kind of a big deal, since I'm pretty sure part of the blame for slow down in the Profiler CPU Hierarchy is due to all the information it has to format into std::strings, which are then marshaled to C#, then processed in the window's IMGUI drawing code.

You can find the code for the editor here. You of course use it at your own risk. I will not be responsible for any strange states you may get your Profiler window into using it. On Windows, you could manually undo any craziness by regedit'ing the raw EditorPrefs key/values (just delete them).

It should be noted, however, that you can't dynamically add/remove columns with the Profiler window open. Before opening it, you must set your desired columns.

Also, a note about the code: I have a USE_SYSTEM_BITVECTOR macro. For public consumption, you'll want to keep this defined as it will cause the code to use the .NET BitVector32, instead of mine (which can do much more).

There's also some incomplete Chart prefs editors, in case anyone is that interested (I'm not, but did as much for completeness' sake).

Hope this helps others manage their...managed code profiling!


Sunday, January 3, 2016

Seeing Sharp C++: The love/hate relationship with Enums

Enums are great. They give you an identifier to either an explicit value (Apples = 1, Oranges = 2), or a compiler derived value. They're something you can easily refactor. They're quasi-type-safe. They're just great.

Except when it comes to generics. And IL2CPP.

In a project I'm currently working on, I make use of a custom BitVector32 struct for cases like object flags storage, etc. For the project I've added some additional methods to interop with it using an Enum via generics, where each member in a given Enum maps to a bit index:

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
#region Access via Enum
private bool ValidateBit<TEnum>(TEnum bit, int bitIndex)
 where TEnum : struct, IComparable, IFormattable, IConvertible
{
 if (bitIndex < 0 || bitIndex >= this.Length)
 {
  Assert("Enum member {0} with value {1} is out of range for indexing",
   bit, bitIndex);
  return false;
 }

 return true;
}

public bool Test<TEnum>(TEnum bit)
 where TEnum : struct, IComparable, IFormattable, IConvertible
{
 int bitIndex = bit.ToInt32(null);
 if (!ValidateBit(bit, bitIndex))
  return false;

 return Bitwise.Flags.Test(mWord, ((uint)1) << bitIndex);
}

public void Set<TEnum>(TEnum bit, bool value)
 where TEnum : struct, IComparable, IFormattable, IConvertible
{
 int bitIndex = bit.ToInt32(null);
 if (!ValidateBit(bit, bitIndex))
  return;

 var flag = ((uint)1) << bitIndex;

 Bitwise.Flags.Modify(value, ref mWord, flag);
}
#endregion

The "where" clause uses as many Enum-specific constraints as possible since whatever lightbulbs behind the language thought it was a bright idea to not allow "where T : enum" (but you can in IL and F#). I mean, what the hell? But I digress, this isn't a Unity problem.

The IConvertible interface gives us the power of ToInt32(), which is needed to generically 'interpret' the TEnum value "bit" as a bit index. For the rest of this article I'm just going to focus on the Set method (line 25). Let's inspect the IL and how IL2CPP (as of 5.3.1p1) transforms it:

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
.method public hidebysig 
 instance void Set<valuetype .ctor ([mscorlib]System.ValueType, [mscorlib]System.IComparable, [mscorlib]System.IFormattable, [mscorlib]System.IConvertible) TEnum> (
  !!TEnum bit,
  bool 'value'
 ) cil managed 
{
 // Method begins at RVA 0x159f8
 // Code size 51 (0x33)
 .maxstack 3
 .locals init (
  [0] int32,
  [1] uint32
 )

 IL_0000: ldarga.s bit
 IL_0002: ldnull
 IL_0003: constrained. !!TEnum
 IL_0009: callvirt instance int32 [mscorlib]System.IConvertible::ToInt32(class [mscorlib]System.IFormatProvider)
 IL_000e: stloc.0
 IL_000f: ldarg.0
 IL_0010: ldarg.1
 IL_0011: ldloc.0
 IL_0012: call instance bool BitVector32::ValidateBit<!!TEnum>(!!0, int32)
 IL_0017: brtrue IL_001d

 IL_001c: ret

 IL_001d: ldc.i4.1
 IL_001e: ldloc.0
 IL_001f: ldc.i4.s 31
 IL_0021: and
 IL_0022: shl
 IL_0023: stloc.1
 IL_0024: ldarg.2
 IL_0025: ldarg.0
 IL_0026: ldflda uint32 BitVector32::mWord
 IL_002b: ldloc.1
 IL_002c: call bool Bitwise.Flags::Modify(bool, uint32&, uint32)
 IL_0031: pop
 IL_0032: ret
} // end of method BitVector32::Set

Here's the IL2CPP of Set instanced with an unimportant enum named MyEnum ('KM00' marks my own comments, not the compiler):
UPDATE: A fix for the double boxing described below is in the pipes on Unity's end. It is now in as of 5.3.1p4, "Removed an unnecessary Box used to null check before calling a virtual method". 

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
// generic sharing
#define IL2CPP_RGCTX_DATA(rgctxVar,index) (InitializedTypeInfo(rgctxVar[index].klass))
#define IL2CPP_RGCTX_METHOD_INFO(rgctxVar,index) (rgctxVar[index].method)

// System.Void BitVector32::Set<MyEnum>(TEnum,System.Boolean)
// System.Void BitVector32::Set<MyEnum>(TEnum,System.Boolean) // [sic] KM00: why is this written twice?
extern TypeInfo* IConvertible_t1162873557_0_il2cpp_TypeInfo_var;
extern const uint32_t BitVector32_Set_TisMyEnum_t1322059486_0_m_97831884_0_MetadataUsageId;
extern "C"  void BitVector32_Set_TisMyEnum_t1322059486_0_m_97831884_0_gshared (BitVector32_t_84621378_0 * __this, int32_t ___bit, bool ___value, const MethodInfo* method)
{
 static bool s_Il2CppMethodIntialized;
 if (!s_Il2CppMethodIntialized)
 {
  il2cpp_codegen_initialize_method (BitVector32_Set_TisMyEnum_t1322059486_0_m_97831884_0_MetadataUsageId);
  s_Il2CppMethodIntialized = true;
 }
 int32_t V_0 = 0;
 uint32_t V_1 = 0;
 {
  // KM00: RGCTX stands for Runtime Generic Context
  // Index 0 in the RGCTX data should be a TypeInfo pointer to MyEnum.
  // So this should be trying to box ___bit to a full MyEnum object (including vtable, etc)
  // And yes, this is resulting in double the garbage: Null checked Box, followed by another Box for the actual ToInt32 call.
  NullCheck((Object_t *)Box(IL2CPP_RGCTX_DATA(method->rgctx_data, 0), (&___bit)));
  // KM00: the actual ToInt32() call
  int32_t L_0 = (int32_t)InterfaceFuncInvoker1< int32_t, Object_t * >::Invoke(7 /* System.Int32 System.IConvertible::ToInt32(System.IFormatProvider) */, IConvertible_t1162873557_0_il2cpp_TypeInfo_var, (Object_t *)Box(IL2CPP_RGCTX_DATA(method->rgctx_data, 0), (&___bit)), (Object_t *)NULL);
  V_0 = (int32_t)L_0;
  int32_t L_1 = ___bit;
  int32_t L_2 = V_0;
  // KM00: this is the ValidateBit() call
  bool L_3 = ((  bool (*) (BitVector32_t_84621378_0 *, int32_t, int32_t, const MethodInfo*))IL2CPP_RGCTX_METHOD_INFO(method->rgctx_data, 1)->method)((BitVector32_t_84621378_0 *)__this, (int32_t)L_1, (int32_t)L_2, /*hidden argument*/IL2CPP_RGCTX_METHOD_INFO(method->rgctx_data, 1));
  if (L_3)
  {
   // KM00: If ValidateBit() didn't fail
   goto IL_001d;
  }
 }
 {
  return;
 }

IL_001d:
 {
  int32_t L_4 = V_0;
  V_1 = (uint32_t)((int32_t)((int32_t)1<<(int32_t)((int32_t)((int32_t)L_4&(int32_t)((int32_t)31)))));
  bool L_5 = ___value;
  uint32_t* L_6 = (uint32_t*)&(__this->___mWord_3);
  uint32_t L_7 = V_1;
  Flags_Modify_m_912810781_0(NULL /*static, unused*/, (bool)L_5, (uint32_t*)L_6, (uint32_t)L_7, /*hidden argument*/NULL);
  return;
 }
}

Essentially what is happening here is that IL2CPP is missing a possible optimization of just treating the ___bit as itself: an int32_t value. Instead it goes through the full plumbing of Enum's IConvertible implementation, which 'requires' a virtual call which itself requires a complete Il2CppObject (vtable, etc).
Line 24 in the above snippet looks an awful lot like bad IL2CPP codegen. It's a bit painful to trace without source (hint hint!), but it appears to be generated in IL2CPP's MethodBodyWriter.CallMethod, specially where it invokes MethodBodyWriter.WriteNullCheckForInvocationIfNeeded. They should probably cache args[0] in a stack variable, then null check that, and then assign args[0] to that stack variable to avoid more boxing and garbage than needed.
As it turns out, calling ToInt32() on an enum value will always result in a GC operation in Mono, as it calls the get_value method which returns an Object. Internally, this is implemented in Mono via ves_icall_System_Enum_get_value. It essentially boxes the enum value to an instance of the underlying-type (int, long, etc) and does a memcpy on the field data. From there it would end up calling the IConvertible on the underlying type (in this case, Int32).

Final thoughts

No matter what your runtime, Unity's super-old Mono or IL2CPP (MS .NET can be more forgiving), you're probably running into more overhead than you realize with Enums and value types in general. Just because you use generics and constrain your type parameters to given interfaces doesn't mean that Mono or IL2CPP will properly optimize the output using that knowledge (in the event types are structs or otherwise sealed).

If C# had more relaxed forms of 'generics' as with C++'s templates, then I could have simply done an 'int bitIndex = (int)bit' cast, with the cast being resolved at template instantiation. The goal of these generic methods was to abstract the need for these casts from client code, but it looks like I'll need to roll back their usage in game loop code.

It also appears interface method calls on value types, as of 5.3.1p1, can result in an extra GC alloc and box operation. IL2CPP has come a long, long way over the past year but it obviously still has lots of room for improvement. For Unity and the Unity community's sake, more developers should inspect their IL2CPP code to not only ensure they're not generating funky IL, but that Unity itself isn't generating funky C++!

One day we'll be able to have nice things. Like enums and generics. Until then I'm just going to byte the bullet and explicitly cast them to integers :(

Saturday, January 2, 2016

IL2CPP codegen bugs as of Unity 5.3.1.p1

While researching the changes made in Patch 5.3.1p1 ("IL2CPP: Optimize method prologues for code size and incremental builds"), I came across both a codegen bug and filegen oversight.

Codegen bug

For IL2CPP codegen, Unity uses StreamWriter. Said class inherits from TextWriter, which exposes a NewLine property. Viewing Unity's mscorlib assembly on Windows in ILSpy shows the default value of NewLine is "\r\n", which is the line ending Windows uses. However, Unity prefers Unix-style line endings. Which wouldn't be a problem if they would either set or override (say, in a UnityStreamWriter class) NewLine to just be "\n"! But they don't.

For IL2CPP's CodeWriter class, it gracefully uses Unix-style since they wrap an internal StreamWriter object and expose WriteLine methods that just call the Write method. However, there are multiple instances across the IL2CPP assembly where StreamWriter is used directly without having NewLine set to Unix-style line endings.

Good Newlines
Bad Newlines
I wouldn't be surprised if there are other places in Unity's managed code where this is an issue. Perhaps Unity's test cases should include checks to ensure there are no carriage returns prior to new lines.

Filegen oversight

While looking through my project's IL2CPP I noticed that enums had method decl headers, like "mscorlib_System_AttributeTargets_59449993MethodDeclarations.h". However, IL2CPP doesn't actually spit out anything (of interest) in these headers for enums! For a project I'm working on IL2CPP spits out almost 13k header and source files. 512 of those are meaningless MethodDeclations for enums.

Should probably check type != enum too