WebP - Reduce the allocations in lossless encoding #2546
Conversation
JimBobSquarePants
requested review from
brianpopow,
tocsoft,
antonfirsov and
dlemstra
|
|
||
| OptimizeHistogramSymbols(clusterMappings, numClusters, mapTmp, histogramSymbols); | ||
| } | ||
|
|
||
| float x = quality / 100.0f; | ||
| float x = quality / 100F; |
There was a problem hiding this comment.
| float x = quality / 100F; | |
| float x = quality * 0.01F; |
Division is more expensive than multiplication.
There was a problem hiding this comment.
I'd need to check again if this is on a hot path.
| if (p.CostDiff >= 0.0d) | ||
| HistoListUpdatePair(histograms[p.Idx1], histograms[p.Idx2], stats, bitsEntropy, 0D, p); | ||
|
|
||
| if (p.CostDiff >= 0D) |
There was a problem hiding this comment.
Re: 0D before the literals were lowercase which is IMO more readable (0d <-> 0D).
Anymway it should be consistent throughout the code-base.
There was a problem hiding this comment.
I have an irrational dislike for lowercase suffix type identifiers for F, D, L, and M. I cannot explain why as I use u at times.
| { | ||
| this.bufferOwner = bufferOwner; | ||
| this.buffer = buffer; | ||
| this.bufferHandle = this.buffer.Pin(); |
There was a problem hiding this comment.
I know I'm the author of the code, but I think this needs further restructuring. The problem is that individual pinning of the sub-buffers forces the original big IMemoryOwner<uint> (which is in fact an UnmanagedBuffer<T> : MemoryManager<T>) to increase the big buffers ref count Vp8LHistogram.Count times, then decrease it on each dispose. This is unnecessary fake bookeeping that only adds a risk of introducing memory leaks. We should instead let Vp8HistogramSet do the pinning, so we take an uint* in the constructor.
For Vp8HistogramSet members this would turn Dispose into a NOP and DisposeAt(idx) to an unnecessary method. For better clarity, I would split Vp8LHistogram into multiple types so Dispose is only defined on instances which are actually disposable.
abstract class Vp8LHistogram
{
// Hide the ctr
protected Vp8LHistogram(uint* basePointer);
}
sealed class OwnedVp8LHistogram : Vp8LHistogram, IDisposable
{
// Static Create methods
}
class Vp8LHistogramSet
{
// Use the hidden ctr within Vp8LHistogramSet code only.
private class MemberHistogram : Vp8LHistogram { }
// !!! Delete DisposeAt()
}I can push this change later, after finishing higher priority contributions.
There was a problem hiding this comment.
This is actually trickier than it looks. Pinning the buffer to pass the pointer is proving troublesome.
There was a problem hiding this comment.
Figured it out.
| // Copyright (c) Six Labors. | ||
| // Licensed under the Six Labors Split License. | ||
|
|
||
| #nullable disable |
There was a problem hiding this comment.
Given that List<Vp8LHistogram> items is in fact a List<Vp8LHistogram?> items, it would be nice to enable this.
There was a problem hiding this comment.
Yeah, I only disabled it and make the items generic type nullable as I didn't want to introduce additional checks in callers.
| bool[] bufRle = new bool[maxNumSymbols]; | ||
| Span<HuffmanTree> huffTree = stackalloc HuffmanTree[3 * maxNumSymbols]; |
Co-authored-by: Anton Firszov <[email protected]>
|
@antonfirsov I want to get this merged so it's not blocking #2569 None of the changes here represent security enhancements so there's nothing to backport. Any future ones can be manually handled. |
Prerequisites
Description
This PR refactors the
Vp8LHistogramtype to use pooled allocations. It also introduces the concept of aVp8LHistogramSettype to allow allocating a single buffer for a collection.Benchmark indicates a 20ms speedup for lossless encoding while using less than half the original allocated memory.
Main
PR