Increase mmf file sizes based on emperical data for roslyn itself. #73108
Conversation
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
|
|
||
| /// <summary> | ||
| /// The size in bytes of a memory mapped file created to store multiple temporary objects. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// <para>This value was arbitrarily chosen and appears to work well. Can be changed if data suggests | ||
| /// something better.</para> | ||
| /// <para>This value (8mb) creates roughly 35 memory mapped files (around 300MB) to store the contents of all of |
There was a problem hiding this comment.
It is not the 32. Each large block we allocate is currently 4mb. This doubles that to halve the number of total Mmfs we need for Roslyn itself
There was a problem hiding this comment.
Knowing nothing about this code, is there a reason that the MultiFileBlockSize is increasing?
Yes. It's basically saying: we'd like each large block to hold at least 32 large files (or a lot more of we're question working with small files.
It's a balance of not wanting to preallocate something huge, or something that grows is massive chunks (like, say 256 mb at a time), while also keeping it handle count low and not creating an excessive number of actual memory mapped files.
These numbers strike a good balance. Growing 8mb at a time is fine, and we can still pack most files into those chunks
There was a problem hiding this comment.
There are two additional risks here:
-
We use bump-pointer allocation to fit new data into these blocks. The larger the item allowed, the larger the potential gap of unused space is at the end of a segment. By doubling the block size at the same time as doubling the threshold, I'm hoping these balance out.
-
We do not use compaction as part of our "GC strategy" for these blocks. If any single segment within the block is retained, the entire block will not be eligible for GC. If after running VS for a long period of time, you observe a significant number of MMF blocks of size 128KiB < size < 256KiB, this would indicate a potential problem with this change because many of those blocks could have increased in size to 8MiB (by being the last segment to retain a block).
Another indicator of the same problem would be a significant number of 4MiB MMF blocks being retained by only a single segment of size 0 < size < 128KiB. Prior to this change, these segments would only end up retaining 4MiB, but this change would cause them to retain 8MiB.
There was a problem hiding this comment.
We use bump-pointer allocation to fit new data into these blocks. The larger the item allowed, the larger the potential gap of unused space is at the end of a segment. By doubling the block size at the same time as doubling the threshold, I'm hoping these balance out.
I didn't see any problematic gaps measuring this. And, as you said, both are doubled. So the expected %-loss should stay the same.
Prior to this change, these segments would only end up retaining 4MiB, but this change would cause them to retain 8MiB.
Both of thse are so small as to be irrelevant afaict. There is no realistic usage pattern i can think of either that would lead to excessive patterns being a problem with teh new numbers that wouldn't have already been a problem with the old ones.
| @@ -65,33 +66,24 @@ internal sealed partial class TemporaryStorageService : ITemporaryStorageService | |||
|
|
|||
| /// <summary> | |||
| /// The most recent memory mapped file for creating multiple storage units. It will be used via bump-pointer | |||
| /// allocation until space is no longer available in it. | |||
| /// allocation until space is no longer available in it. Access should be synchronized on <see cref="_gate"/> | |||
There was a problem hiding this comment.
📝 The old form of this comment was preferred.
| /// <para>Access should be synchronized on <see cref="_gate"/>.</para> | ||
| /// </remarks> | ||
| /// <summary>The name of the current memory mapped file for multiple storage units. Access should be synchronized on | ||
| /// <see cref="_gate"/></summary> |
There was a problem hiding this comment.
📝 The access condition is not part of the description of this field. The old form of this comment was preferred.
|
@jasonmalinowski For review when you get back. |
No description provided.