Fixed LOH byte array allocations in the IDE from metadata #7971
Conversation
TIHan
changed the title
|
CI failed not as a result of this change, it's due to this commit: d68942f |
|
Great! Thank you! |
|
This is ready. |
There was a problem hiding this comment.
I think the core implementation could be an interface like this: https://gist.github.com/cartermp/e3b3d20f587ecf4cc5e40a06653e3a54
Mostly I feel more comfortable exposing an interface instead of an abstract class in a public contract, and since it's basically just an interface anyways that would feel appropriate. The main downside is that the extension methods have to be in a module.
|
I prefer interfaces if everything was self contained in the file and nothing else would use it. Since we are using |
There was a problem hiding this comment.
Looks great! Error handling does not following the guidelines here though: https://docs.microsoft.com/en-us/dotnet/fsharp/style-guide/conventions#use-exceptions-when-errors-cannot-be-represented-with-types
There was a problem hiding this comment.
If you take care of the Exceptions, @cartermp pointed out, this is good.
|
Gotcha, will do. Thanks for the feedback! |
|
@cartermp I think this is ready with the feedback changes. |
* Fixed LOH byte array allocations in the IDE. Unified memory handling for IL and FSharp metadata. * minor cleanup * minor cleanup * rename EmitBytes to EmitByteMemory * Fixed ByteArrayMemory * fixing build * Fixing build again * Better ReadInt32/ReadUInt16 * ILResourceLocation.Local should use a ReadOnlyByteMemory * Able to have a ReadOnlyStream * Using OutOfRangeException according to feedback
When doing a find all references in VS on a
booltype in the VisualFSharp.sln, byte arrays are the top LOH allocations, mainly around FSharp metadata.This PR should resolve some of it by using memory mapped files. It also unifies the memory handling of both IL and FSharp metadata with the
ByteMemorytype.A
ByteMemorycan be backed by in-memory or memory mapped file data.Let's see if CI passes. :)