Reserve capacity for regular file artifacts
This commit is contained in:
@@ -116,10 +116,14 @@ and after opening the file. It copies content into an artifact, records
|
|||||||
metadata and an opaque content-equality value, and applies a content-type
|
metadata and an opaque content-equality value, and applies a content-type
|
||||||
fallback.
|
fallback.
|
||||||
|
|
||||||
Regular files are read synchronously in bounded chunks. Cancellation is
|
Regular files are read synchronously in bounded chunks. The opened file's size
|
||||||
checked before opening, before and after every read, and before publishing the
|
is only an initial-capacity hint, capped at 1 MiB; reads continue to EOF, and
|
||||||
artifact, so a canceled read never publishes partial content. The ordinary
|
the returned body determines artifact size and hash. If an overreported hint
|
||||||
reader does not detach file reads into background goroutines.
|
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
|
||||||
|
canceled read never publishes partial content. The ordinary reader does not
|
||||||
|
detach file reads into background goroutines.
|
||||||
|
|
||||||
This ordinary reader does not implement an inbound HTTP security boundary. In
|
This ordinary reader does not implement an inbound HTTP security boundary. In
|
||||||
particular, it does not constrain files to an application root or impose an
|
particular, it does not constrain files to an application root or impose an
|
||||||
|
|||||||
@@ -2,7 +2,7 @@
|
|||||||
|
|
||||||
Implement the remaining local work in the [performance roadmap](performance.md)
|
Implement the remaining local work in the [performance roadmap](performance.md)
|
||||||
in order. Each sequential stage is scoped for one Luna coding-agent prompt.
|
in order. Each sequential stage is scoped for one Luna coding-agent prompt.
|
||||||
Stages 1–3 are complete; Stages 4–14 remain pending. Update status and evidence
|
Stages 1–4 are complete; Stages 5–14 remain pending. Update status and evidence
|
||||||
as each work unit lands.
|
as each work unit lands.
|
||||||
The completed inspection plan is preserved at
|
The completed inspection plan is preserved at
|
||||||
`481e2d4:docs/roadmap/implementation.md` in Git history.
|
`481e2d4:docs/roadmap/implementation.md` in Git history.
|
||||||
@@ -178,6 +178,12 @@ without `-race`. Different request inputs must remain independent.
|
|||||||
|
|
||||||
## Stage 4 — Reserve A Useful Initial File-Read Capacity
|
## Stage 4 — Reserve A Useful Initial File-Read Capacity
|
||||||
|
|
||||||
|
**Status: Complete (2026-10-09).** File reads now use the opened regular
|
||||||
|
file's size only as a capped initial-capacity hint, continue reading to EOF,
|
||||||
|
and compact a completed body when a stale hint leaves disproportionate unused
|
||||||
|
capacity. The exact-size and overreported-size benchmark comparisons are
|
||||||
|
recorded below.
|
||||||
|
|
||||||
**Outcome:** Ordinary file reads avoid early geometric buffer growth without
|
**Outcome:** Ordinary file reads avoid early geometric buffer growth without
|
||||||
trusting metadata as the amount of content to return.
|
trusting metadata as the amount of content to return.
|
||||||
|
|
||||||
@@ -683,6 +689,61 @@ including exact message/session output, cancellation, artifact-text lifecycle,
|
|||||||
and a new concurrent-render case that verifies independent request variables,
|
and a new concurrent-render case that verifies independent request variables,
|
||||||
artifact text, and session IDs against shared compiled templates.
|
artifact text, and session IDs against shared compiled templates.
|
||||||
|
|
||||||
|
### Artifact File-Read Capacity Comparison
|
||||||
|
|
||||||
|
The benchmark-only fixture was committed as `996295d` (`Add artifact file-read
|
||||||
|
benchmarks`) before the reader change. Before measurements use that revision;
|
||||||
|
after measurements use the same fixture after the bounded reservation and
|
||||||
|
post-read compaction were implemented. The real-file cases contain 4 KiB,
|
||||||
|
1 MiB, and 4 MiB of repeated `x` bytes. The retained case reads a real 256 KiB
|
||||||
|
file whose opened-file seam reports 16 MiB, retaining four results. Fixture
|
||||||
|
creation and MIME lookup are outside timing. Environment: Linux/amd64, Intel
|
||||||
|
Core i5-7260U @ 2.20 GHz, benchmark parallelism 4, Go 1.26.8, ConfigKit
|
||||||
|
`v0.5.0`, JSON Schema `v6.0.2`, and backend modules `v3.20260930.0`.
|
||||||
|
|
||||||
|
Command for both runs (five 500 ms samples per benchmark, without the race
|
||||||
|
detector):
|
||||||
|
|
||||||
|
```sh
|
||||||
|
GOTOOLCHAIN=local GOWORK=off GOPROXY=off GONOPROXY=none GOSUMDB=off \
|
||||||
|
go test -mod=readonly -run '^$' \
|
||||||
|
-bench '^(BenchmarkArtifactFileRead|BenchmarkRetainedArtifactFileRead)$' \
|
||||||
|
-benchmem -benchtime=500ms -count=5 ./internal/artifact
|
||||||
|
```
|
||||||
|
|
||||||
|
Times list all five `ns/op` samples in command order, followed by median and
|
||||||
|
range. Allocation figures are `B/op; allocs/op`.
|
||||||
|
|
||||||
|
| File-read case | Before `ns/op` (median; range) | Before allocations | After `ns/op` (median; range) | After allocations |
|
||||||
|
| --- | --- | --- | --- | --- |
|
||||||
|
| 4 KiB | 37904, 38759, 37073, 36437, 36930 (37073; 36437–38759) | 70513–70514; 12 | 37222, 37414, 36521, 37867, 39988 (37414; 36521–39988) | 70513; 12 |
|
||||||
|
| 1 MiB | 3576064, 4010278, 3919714, 3918643, 3844238 (3918643; 3576064–4010278) | 4752589–4752677; 23–24 | 3105775, 3055909, 3160450, 3268202, 3079892 (3105775; 3055909–3268202) | 1115066–1115090; 12 |
|
||||||
|
| 4 MiB | 16818098, 15977605, 16465923, 15753739, 15589001 (15977605; 15589001–16818098) | 20244106–20244130; 33 | 17060165, 16462547, 15995322, 17206316, 16180077 (16462547; 15995322–17206316) | 21177936–21178008; 24–25 |
|
||||||
|
|
||||||
|
The 1 MiB case reduced cumulative allocation from about 4.75 MB to 1.12 MB
|
||||||
|
and from 23–24 to 12 allocations, with a lower median time. For 4 MiB, the
|
||||||
|
reservation reduced allocation count from 33 to 24–25 but increased cumulative
|
||||||
|
allocated bytes by about 0.93 MB; timing ranges overlap. The small-file case
|
||||||
|
was effectively unchanged. These results support the specified capped hint as
|
||||||
|
a way to avoid repeated growth for ordinary 1 MiB reads, while showing that
|
||||||
|
the 1 MiB initial reserve is not a cumulative-byte win for every larger file.
|
||||||
|
|
||||||
|
| Overreported-size retained workload | Before | After |
|
||||||
|
| --- | --- | --- |
|
||||||
|
| `ns/op` (median; range) | 1144696, 1073206, 1060135, 1059339, 1073194 (1073194; 1059339–1144696) | 1188822, 1201670, 1199592, 1175553, 1173590 (1188822; 1173590–1201670) |
|
||||||
|
| `B/op; allocs/op` | 779427–779463; 18 | 1377557–1377574; 17 |
|
||||||
|
| Retained heap (`retained-B`, median; range) | 1082272, 1082496, 1082944, 1082384, 1082496 (1082496; 1082272–1082944) | 1049728, 1049392, 1049280, 1049504, 1049168 (1049392; 1049168–1049728) |
|
||||||
|
|
||||||
|
For the stale 16 MiB hint and 256 KiB actual body, the bounded reserve adds
|
||||||
|
about 0.6 MB of cumulative allocation and roughly 0.1 ms to the read, but the
|
||||||
|
post-read compaction prevents it from increasing live retained storage; the
|
||||||
|
four-result retained-heap median is about 33 KiB lower than the baseline. This
|
||||||
|
is intentionally a stale-metadata path, not a normal exact-size read. The
|
||||||
|
ordinary reader still derives returned bytes, size, and hash from content read
|
||||||
|
through EOF. Focused verification passed: `go test -mod=readonly
|
||||||
|
./internal/artifact`; the existing cancellation and nonregular-file checks
|
||||||
|
remain in the passing suite.
|
||||||
|
|
||||||
## Open Questions
|
## Open Questions
|
||||||
|
|
||||||
No consumer-policy decisions remain open. File reservation and encoding error
|
No consumer-policy decisions remain open. File reservation and encoding error
|
||||||
|
|||||||
@@ -21,6 +21,7 @@ var (
|
|||||||
)
|
)
|
||||||
|
|
||||||
const fileReadChunkSize = 64 * 1024
|
const fileReadChunkSize = 64 * 1024
|
||||||
|
const fileReadInitialCapacityLimit = 1 << 20
|
||||||
|
|
||||||
// Reader resolves artifact references into actual artifacts.
|
// Reader resolves artifact references into actual artifacts.
|
||||||
type Reader interface {
|
type Reader interface {
|
||||||
@@ -133,7 +134,11 @@ func readFileArtifact(ctx context.Context, path string, open func(string) (artif
|
|||||||
return nil, fmt.Errorf("%w: %s", ErrUnsupportedFile, path)
|
return nil, fmt.Errorf("%w: %s", ErrUnsupportedFile, path)
|
||||||
}
|
}
|
||||||
|
|
||||||
data := make([]byte, 0)
|
capacity := 0
|
||||||
|
if size := openedInfo.Size(); size > 0 {
|
||||||
|
capacity = int(min(size, int64(fileReadInitialCapacityLimit)))
|
||||||
|
}
|
||||||
|
data := make([]byte, 0, capacity)
|
||||||
chunk := make([]byte, fileReadChunkSize)
|
chunk := make([]byte, fileReadChunkSize)
|
||||||
for {
|
for {
|
||||||
if err := ctx.Err(); err != nil {
|
if err := ctx.Err(); err != nil {
|
||||||
@@ -153,6 +158,13 @@ func readFileArtifact(ctx context.Context, path string, open func(string) (artif
|
|||||||
return nil, fmt.Errorf("failed to read file %s: %w", path, readErr)
|
return nil, fmt.Errorf("failed to read file %s: %w", path, readErr)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
if len(data) > 0 && 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))
|
||||||
|
copy(compact, data)
|
||||||
|
data = compact
|
||||||
|
}
|
||||||
|
|
||||||
contentType := mime.TypeByExtension(filepath.Ext(path))
|
contentType := mime.TypeByExtension(filepath.Ext(path))
|
||||||
if contentType == "" {
|
if contentType == "" {
|
||||||
|
|||||||
@@ -3,7 +3,9 @@ package artifact
|
|||||||
import (
|
import (
|
||||||
"bytes"
|
"bytes"
|
||||||
"context"
|
"context"
|
||||||
|
"crypto/sha256"
|
||||||
"errors"
|
"errors"
|
||||||
|
"fmt"
|
||||||
"os"
|
"os"
|
||||||
"path/filepath"
|
"path/filepath"
|
||||||
"strings"
|
"strings"
|
||||||
@@ -232,6 +234,60 @@ func TestFileReaderFailuresAndMetadata(t *testing.T) {
|
|||||||
})
|
})
|
||||||
}
|
}
|
||||||
|
|
||||||
|
func TestFileReaderUsesOpenedSizeOnlyAsACapacityHint(t *testing.T) {
|
||||||
|
tests := []struct {
|
||||||
|
name string
|
||||||
|
body []byte
|
||||||
|
reportedSize int64
|
||||||
|
}{
|
||||||
|
{name: "overreported size", body: []byte("actual file content"), reportedSize: 128},
|
||||||
|
{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)},
|
||||||
|
}
|
||||||
|
|
||||||
|
for _, tc := range tests {
|
||||||
|
t.Run(tc.name, func(t *testing.T) {
|
||||||
|
filePath := filepath.Join(t.TempDir(), "artifact.txt")
|
||||||
|
if err := os.WriteFile(filePath, tc.body, 0o600); err != nil {
|
||||||
|
t.Fatal(err)
|
||||||
|
}
|
||||||
|
reader := &fileReader{open: func(path string) (artifactFile, error) {
|
||||||
|
file, err := os.Open(path)
|
||||||
|
if err != nil {
|
||||||
|
return nil, err
|
||||||
|
}
|
||||||
|
info, err := file.Stat()
|
||||||
|
if err != nil {
|
||||||
|
file.Close()
|
||||||
|
return nil, err
|
||||||
|
}
|
||||||
|
return &reportedInfoFile{
|
||||||
|
artifactFile: file,
|
||||||
|
info: reportedFileSizeInfo{FileInfo: info, size: tc.reportedSize},
|
||||||
|
}, nil
|
||||||
|
}}
|
||||||
|
|
||||||
|
artifact, err := reader.Read(context.Background(), domain.ArtifactRef{
|
||||||
|
Type: domain.ArtifactRefFile,
|
||||||
|
URI: filePath,
|
||||||
|
})
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("read artifact: %v", err)
|
||||||
|
}
|
||||||
|
if !bytes.Equal(artifact.Body, tc.body) {
|
||||||
|
t.Fatalf("artifact body length=%d, want %d", len(artifact.Body), len(tc.body))
|
||||||
|
}
|
||||||
|
if artifact.Size != int64(len(tc.body)) {
|
||||||
|
t.Fatalf("artifact size=%d, want %d", artifact.Size, len(tc.body))
|
||||||
|
}
|
||||||
|
wantHash := fmt.Sprintf("%x", sha256.Sum256(tc.body))
|
||||||
|
if artifact.Hash != wantHash {
|
||||||
|
t.Fatalf("artifact hash=%q, want %q", artifact.Hash, wantHash)
|
||||||
|
}
|
||||||
|
})
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
func TestFileReaderCancelsAfterReadProgress(t *testing.T) {
|
func TestFileReaderCancelsAfterReadProgress(t *testing.T) {
|
||||||
filePath := filepath.Join(t.TempDir(), "artifact.bin")
|
filePath := filepath.Join(t.TempDir(), "artifact.bin")
|
||||||
content := bytes.Repeat([]byte("x"), fileReadChunkSize*2)
|
content := bytes.Repeat([]byte("x"), fileReadChunkSize*2)
|
||||||
@@ -268,6 +324,13 @@ func (f *reportedInfoFile) Stat() (os.FileInfo, error) {
|
|||||||
return f.info, nil
|
return f.info, nil
|
||||||
}
|
}
|
||||||
|
|
||||||
|
type reportedFileSizeInfo struct {
|
||||||
|
os.FileInfo
|
||||||
|
size int64
|
||||||
|
}
|
||||||
|
|
||||||
|
func (info reportedFileSizeInfo) Size() int64 { return info.size }
|
||||||
|
|
||||||
type cancelAfterProgressFile struct {
|
type cancelAfterProgressFile struct {
|
||||||
artifactFile
|
artifactFile
|
||||||
cancel context.CancelFunc
|
cancel context.CancelFunc
|
||||||
|
|||||||
Reference in New Issue
Block a user