Release unused file buffers for empty artifacts
This commit is contained in:
@@ -131,9 +131,10 @@ fallback.
|
||||
Regular files are read synchronously in bounded chunks. The opened file's size
|
||||
is only an initial-capacity hint, capped at 1 MiB; reads continue to EOF, and
|
||||
the returned body determines artifact size and hash. If an overreported hint
|
||||
leaves more than twice the actual body length unused, the completed body is
|
||||
copied to release the excess backing storage. Cancellation is checked before
|
||||
opening, before and after every read, and before publishing the artifact, so a
|
||||
leaves unused capacity greater than the actual body length, the completed body
|
||||
is copied to release the excess backing storage, including when the body is
|
||||
empty. Cancellation is checked before opening, before and after every read,
|
||||
and before publishing the artifact, so a
|
||||
canceled read never publishes partial content. The ordinary reader does not
|
||||
detach file reads into background goroutines.
|
||||
|
||||
|
||||
@@ -1,7 +1,6 @@
|
||||
# Implementation Review Remediation
|
||||
|
||||
Status: One computational-efficiency finding awaits remediation. Earlier
|
||||
findings below remain resolved.
|
||||
Status: All recorded findings have been resolved in separate remediation units.
|
||||
|
||||
## Computational Efficiency Review
|
||||
|
||||
@@ -10,7 +9,16 @@ Review baseline: `8a5c0de`, compared with planning commit `3fde1f6`, the
|
||||
|
||||
### Finding 1 — Release A Stale File Reservation For An Empty Body (P3)
|
||||
|
||||
**Status: Open.**
|
||||
**Status: Resolved.** Completed empty bodies now release stale file-size
|
||||
reservations using the existing compaction path. The stale-size table covers
|
||||
empty content, metadata, and hashes; the retained-artifact benchmark includes
|
||||
an empty-body fixture without enforcing capacity or allocation thresholds.
|
||||
Three offline, one-iteration samples for four retained empty artifacts changed
|
||||
from 4,194,928–4,194,976 retained bytes to 624–640 bytes. Initial reservation
|
||||
allocations still occur; this fix reduces retained storage, not cumulative
|
||||
allocation. The nonempty benchmark remains near 1 MiB for four 256 KiB bodies.
|
||||
Artifact package race tests and vet passed, and the internal documentation
|
||||
now accurately describes the compaction condition.
|
||||
|
||||
Location: [file reader](../../internal/artifact/reader.go), lines 137–166.
|
||||
|
||||
|
||||
@@ -158,7 +158,7 @@ func readFileArtifact(ctx context.Context, path string, open func(string) (artif
|
||||
return nil, fmt.Errorf("failed to read file %s: %w", path, readErr)
|
||||
}
|
||||
}
|
||||
if len(data) > 0 && cap(data)-len(data) > len(data) {
|
||||
if cap(data)-len(data) > len(data) {
|
||||
// A stale size hint should not make a small completed artifact retain a
|
||||
// disproportionately large backing array.
|
||||
compact := make([]byte, len(data))
|
||||
|
||||
@@ -42,7 +42,20 @@ func BenchmarkArtifactFileRead(b *testing.B) {
|
||||
// BenchmarkRetainedArtifactFileRead measures live artifact bodies when an
|
||||
// opened file reports a size larger than the content that reaches EOF.
|
||||
func BenchmarkRetainedArtifactFileRead(b *testing.B) {
|
||||
body := bytes.Repeat([]byte("x"), 256<<10)
|
||||
for _, fixture := range []struct {
|
||||
name string
|
||||
body []byte
|
||||
}{
|
||||
{name: "small", body: bytes.Repeat([]byte("x"), 256<<10)},
|
||||
{name: "empty", body: []byte{}},
|
||||
} {
|
||||
b.Run(fixture.name, func(b *testing.B) {
|
||||
benchmarkRetainedArtifactFileRead(b, fixture.body)
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func benchmarkRetainedArtifactFileRead(b *testing.B, body []byte) {
|
||||
path := filepath.Join(b.TempDir(), "overreported.txt")
|
||||
if err := os.WriteFile(path, body, 0o600); err != nil {
|
||||
b.Fatal(err)
|
||||
|
||||
@@ -241,6 +241,7 @@ func TestFileReaderUsesOpenedSizeOnlyAsACapacityHint(t *testing.T) {
|
||||
reportedSize int64
|
||||
}{
|
||||
{name: "overreported size", body: []byte("actual file content"), reportedSize: 128},
|
||||
{name: "overreported size with empty content", body: []byte{}, reportedSize: 16 << 20},
|
||||
{name: "underreported size", body: bytes.Repeat([]byte("x"), fileReadChunkSize+17), reportedSize: 1},
|
||||
{name: "very large hint with small content", body: []byte("small actual content"), reportedSize: int64(^uint64(0) >> 1)},
|
||||
}
|
||||
@@ -280,6 +281,9 @@ func TestFileReaderUsesOpenedSizeOnlyAsACapacityHint(t *testing.T) {
|
||||
if artifact.Size != int64(len(tc.body)) {
|
||||
t.Fatalf("artifact size=%d, want %d", artifact.Size, len(tc.body))
|
||||
}
|
||||
if artifact.Name != filepath.Base(filePath) || artifact.URI != filePath || artifact.ContentType == "" {
|
||||
t.Fatalf("unexpected artifact metadata: name=%q URI=%q content type=%q", artifact.Name, artifact.URI, artifact.ContentType)
|
||||
}
|
||||
wantHash := fmt.Sprintf("%x", sha256.Sum256(tc.body))
|
||||
if artifact.Hash != wantHash {
|
||||
t.Fatalf("artifact hash=%q, want %q", artifact.Hash, wantHash)
|
||||
|
||||
Reference in New Issue
Block a user