Improve performance of map thumbnail loading#985
Conversation
|
Nightly build for this pull request:
|
I think there was a way to specify the specific subset of sections and keys to parse already, was it not used in this case? |
|
@Metadorius It is partially used here. Not in this class, but where the preview extractor is called in Map.cs. It could be optimized to be slightly tighter, but it is unlikely that it'd be as fast as the custom reader. |
|
I am thinking preserving the old code (slow, good readability) as a virtual class method, and let the new code (fast, bad readability) act as a class method that override it. In this way we can retain the old code as a "reference implementation" |
There was a problem hiding this comment.
Pull request overview
This PR speeds up non-immediate map thumbnail extraction by avoiding full INI parsing and instead scanning the map file once to read only [Preview] Size and [PreviewPack] values.
Changes:
- Added
FastMapPreviewExtractorthat scans map files line-by-line and extracts preview payload + size. - Introduced
IMapPreviewExtractorand refactoredMapPreviewExtractorinto an instantiable base class to share decompress/bitmap creation logic. - Switched
Map.GetNonImmediatePreviewImage()to use the fast extractor.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| DXMainClient/Domain/Multiplayer/MapPreviewExtractor.cs | Refactored to instance-based API and exposed core helpers for reuse by the fast extractor. |
| DXMainClient/Domain/Multiplayer/Map.cs | Uses FastMapPreviewExtractor for non-immediate preview extraction. |
| DXMainClient/Domain/Multiplayer/IMapPreviewExtractor.cs | New interface for preview extraction. |
| DXMainClient/Domain/Multiplayer/FastMapPreviewExtractor.cs | New single-pass scanner implementation for preview extraction. |
Comments suppressed due to low confidence (1)
DXMainClient/Domain/Multiplayer/MapPreviewExtractor.cs:116
- DecompressPreviewData creates an LzoStream inside a loop but never disposes it. This can retain buffers and file/stream resources longer than needed. Wrap the LzoStream (and underlying MemoryStream, if appropriate) in a using statement (or using var) to ensure timely disposal.
protected byte[] DecompressPreviewData(byte[] dataSource, int decompressedDataSize, out string errorMessage)
{
try
{
byte[] dataDest = new byte[decompressedDataSize];
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ordering Agent-Logs-Url: https://github.com/SadPencil/xna-cncnet-client/sessions/8bb70b8a-dc8d-4ba7-9c82-526c7a265b43 Co-authored-by: SadPencil <11227602+SadPencil@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
DXMainClient/Domain/Multiplayer/MapPreviewExtractor.cs:137
DecompressPreviewDatadeclares a non-nullbyte[]return, but it returnsnullon error paths. Since this method is nowprotectedand consumed by nullable-enabled code, the non-null contract is misleading. Update the return type (anderrorMessageout parameter) to reflect nullability, or add a nullable-enabled region for these members.
protected byte[] DecompressPreviewData(byte[] dataSource, int decompressedDataSize, out string errorMessage)
{
try
{
byte[] dataDest = new byte[decompressedDataSize];
int readBytes = 0, writtenBytes = 0;
while (true)
{
if (readBytes >= dataSource.Length)
break;
ushort sizeCompressed = BinaryPrimitives.ReadUInt16LittleEndian(dataSource.AsSpan(readBytes));
readBytes += 2;
ushort sizeUncompressed = BinaryPrimitives.ReadUInt16LittleEndian(dataSource.AsSpan(readBytes));
readBytes += 2;
if (sizeCompressed == 0 || sizeUncompressed == 0)
break;
if (readBytes + sizeCompressed > dataSource.Length ||
writtenBytes + sizeUncompressed > dataDest.Length)
{
errorMessage = "Preview data does not match preview size or the data is corrupted, unable to extract preview.";
return null;
}
DXMainClient/Domain/Multiplayer/MapPreviewExtractor.cs:172
CreatePreviewBitmapFromImageDatadeclares a non-nullImagereturn, but it returnsnullon validation/errors. WithFastMapPreviewExtractorcalling this from a nullable-enabled file, the signature is misleading and can mask null-safety issues. Change the return type (anderrorMessageout parameter) to nullable, or add a nullable-enabled region for these members.
protected Image CreatePreviewBitmapFromImageData(int width, int height, byte[] imageData, out string errorMessage)
{
const int pixelFormatBitCount = 24;
const int pixelFormatByteCount = pixelFormatBitCount / 8;
if (imageData.Length != width * height * pixelFormatByteCount)
{
errorMessage = "Provided preview image dimensions do not match preview image data length.";
return null;
}

Replace the IniFile based preview loading with a single-pass line scanner that collects only [Preview] Size and [PreviewPack] values, avoiding the overhead of parsing and allocating unrelated sections.
Tested on ~2500 maps without issue. Speed is improved greatly - maps that would take a second or two before are now <100ms.
If we'd prefer not to reinvent INI loading and want to stick with Rampastring.Tools instead, then we can look at making some changes to the INI loading by adding a dictionary. It speeds it up, although not as good as this and I wasn't a huge fan of the methodology.