prepacked_cache: fix byte-accounting for evicted float matrices#374
Open
EylonKrause wants to merge 1 commit into
Open
prepacked_cache: fix byte-accounting for evicted float matrices#374EylonKrause wants to merge 1 commit into
EylonKrause wants to merge 1 commit into
Conversation
AllocateBuffers only allocates and counts a `sums` buffer for non-floating-point matrices; float matrices never allocate one, so a float insertion only adds DataBytes to buffers_bytes_. EjectOne, however, unconditionally subtracted DataBytes + SumsBytes. A float matrix's sums_type still reports a nonzero element size, so SumsBytes is nonzero and each float eviction over-subtracted it, letting buffers_bytes_ drift below true usage (possibly negative) so the cache silently overshoots max_buffers_bytes_. Subtract SumsBytes only for non-floating-point matrices, mirroring AllocateBuffers.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
PrepackedCachetracks total cached bytes inbuffers_bytes_to enforcemax_buffers_bytes_. Insertion and eviction account for thesumsbuffer asymmetrically for floating-point matrices, so the running total drifts.Root cause
AllocateBuffers(prepacked_cache.cc:29-39) allocates and counts asumsbuffer only for non-floating-point matrices — a float matrix adds justDataBytestobuffers_bytes_(Get, line 100). ButEjectOne(line 124) unconditionally subtractedDataBytes + SumsBytes.A float matrix's
sums_typestill reports a nonzero element size: it is set toType::Create<SumsType>()withSumsType = Scalarfor floating-point scalars (create_trmul_params.h:62-69), i.e.float(size 4). SoSumsBytes = layout.cols * sums_type.size > 0(mat.h:436-439) even though nosumsbuffer was allocated. Each float eviction therefore over-subtractscols * 4, lettingbuffers_bytes_drift below true usage (possibly negative) so the cache silently overshoots its cap.Fix
Subtract
SumsBytesonly for non-floating-point matrices, mirroringAllocateBuffers.Verification
Pure accounting logic; reproduced with a standalone model of insert/evict (float matrix, data=1000, cols*4=256):
The existing
prepacked_cache_test.ccdoes not catch this: its dummy float PEMat leavessums_typeat the default (size 0), and no test evicts a float matrix. Happy to add a regression test that evicts a float matrix.