-
Notifications
You must be signed in to change notification settings - Fork 804
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
ByteBuffer
uses ArrayPool
to prevent LOH allocations
#11660
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the approach, and it seems great to not constantly reallocate those large buffers.
ByteBuffer
uses ArrayPool to prevent LOH allocations
ByteBuffer
uses ArrayPool to prevent LOH allocationsByteBuffer
uses ArrayPool
to prevent LOH allocations
eng/Versions.props
Outdated
@@ -75,6 +75,7 @@ | |||
https://pkgs.dev.azure.com/azure-public/vside/_packaging/vs-impl/nuget/v3/index.json; | |||
</RestoreSources> | |||
<!-- System.* packages --> | |||
<SystemRuntimeCompilerServicesUnsafeVersion>4.7.1</SystemRuntimeCompilerServicesUnsafeVersion> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@KevinRansom you may not like this, but the changes I made with the ByteBuffer, is calling something from System.Runtime.CompilerServices.Unsafe, which has problems with binding redirects. Best way to resolve this is including the specific version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks goods, I wonder why we have so many different sizes for allocation buffers, 100000, 50000, 20000 and 10000. Meaningful names might help me understand why each size was chosen, Of course it might not too.
src/fsharp/QuotationPickler.fs
Outdated
let phase2data = (stringTab, phase1bytes) | ||
|
||
let st2 = | ||
{ os = ByteBuffer.Create(100000, useArrayPool = true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
possibly a constant for 100000, it's used several places in this code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no idea why these values were chosen. This was here before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, just give it a name. pickle_object_buffersize or somesuch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the literals.
src/fsharp/absil/ilwrite.fs
Outdated
{ emitTailcalls=emitTailcalls | ||
deterministic = deterministic | ||
showTimes=showTimes | ||
ilg = ilg | ||
desiredMetadataVersion=desiredMetadataVersion | ||
requiredDataFixups= requiredDataFixups | ||
requiredStringFixups = [] | ||
codeChunks=ByteBuffer.Create 40000 | ||
codeChunks=ByteBuffer.Create(40000, useArrayPool = true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps constant for 40000
src/fsharp/absil/ilwrite.fs
Outdated
@@ -3230,7 +3241,7 @@ let writeILMetadataAndCode (generatePdb, desiredMetadataVersion, ilg, emitTailca | |||
codedBigness 2 TableNames.AssemblyRef || | |||
codedBigness 2 TableNames.TypeRef | |||
|
|||
let tablesBuf = ByteBuffer.Create 20000 | |||
use tablesBuf = ByteBuffer.Create(20000, useArrayPool = true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps a name and a constant for 20000
src/fsharp/absil/ilwrite.fs
Outdated
@@ -3309,7 +3320,7 @@ let writeILMetadataAndCode (generatePdb, desiredMetadataVersion, ilg, emitTailca | |||
reportTime showTimes "Layout Metadata" | |||
|
|||
let metadata, guidStart = | |||
let mdbuf = ByteBuffer.Create 500000 | |||
use mdbuf = ByteBuffer.Create(500000, useArrayPool = true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps a name and a constant for 50000?
Addresses this issue: #7982 |
note that azdo is down for us, so there's no point in trying to re-trigger CI |
We allocate a lot on the LOH when serializing FSharp metadata. This PR should help.