mirror of
https://github.com/navidrome/navidrome.git
synced 2026-05-03 06:51:16 +00:00
perf: reduce hot-path heap escapes from value-param pointer aliasing (#5342)
* perf(subsonic): keep album/mediafile params on stack in response helpers Two helpers were forcing their entire value parameter onto the heap via pointer-to-field aliasing, adding one full-struct heap allocation per response item on hot Subsonic endpoints (search3, getAlbumList2, etc.). - childFromMediaFile assigned &mf.BirthTime to the returned Child, pulling the whole ~1KB model.MediaFile to the heap on every call. - buildDiscSubtitles passed &a.UpdatedAt to NewArtworkID inside a loop, pulling the whole model.Album to the heap on every album with discs. Both now copy the time.Time to a stack-local and use gg.P / &local so only the small time.Time escapes. Verified via go build -gcflags=-m=2: moved to heap: mf and moved to heap: a are gone at these sites. * perf(metadata): avoid per-track closure allocations in PID computation createGetPID was a factory that returned nested closures capturing mf model.MediaFile (~992 bytes) by reference. Since it is called three times per track during scans (trackPID, albumID, artistID), every track triggered the allocation of three closures plus a heap copy of the full MediaFile. Refactor the body into package-level functions (computePID, getPIDAttr) that take hash as an explicit parameter and the inner slice.Map callback to an indexed for loop, removing the closure-capture of mf entirely. trackPID/albumID/artistID now call computePID directly. The tiny createGetPID wrapper was kept only for tests; move the closure-building into the test file so production has no dead API. Verified via go build -gcflags=-m=2 on model/metadata: no "moved to heap: mf" anywhere in persistent_ids.go, and the callers in map_mediafile.go / map_participants.go no longer heap-promote their MediaFile argument.
This commit is contained in:
parent
9b0bfc606b
commit
ab2f1b45de
@ -12,54 +12,41 @@ import (
|
|||||||
"github.com/navidrome/navidrome/model"
|
"github.com/navidrome/navidrome/model"
|
||||||
"github.com/navidrome/navidrome/model/id"
|
"github.com/navidrome/navidrome/model/id"
|
||||||
"github.com/navidrome/navidrome/utils"
|
"github.com/navidrome/navidrome/utils"
|
||||||
"github.com/navidrome/navidrome/utils/slice"
|
|
||||||
"github.com/navidrome/navidrome/utils/str"
|
"github.com/navidrome/navidrome/utils/str"
|
||||||
)
|
)
|
||||||
|
|
||||||
type hashFunc = func(...string) string
|
type hashFunc = func(...string) string
|
||||||
|
|
||||||
// createGetPID returns a function that calculates the persistent ID for a given spec, getting the referenced values from the metadata
|
// computePID calculates the persistent ID for a given spec. The spec is a
|
||||||
// The spec is a pipe-separated list of fields, where each field is a comma-separated list of attributes
|
// pipe-separated list of fields, where each field is a comma-separated list of
|
||||||
// Attributes can be either tags or some processed values like folder, albumid, albumartistid, etc.
|
// attributes. Attributes can be either tags or processed values like folder,
|
||||||
// For each field, it gets all its attributes values and concatenates them, then hashes the result.
|
// albumid, albumartistid, etc. For each field, it gets all its attribute values
|
||||||
// If a field is empty, it is skipped and the function looks for the next field.
|
// and concatenates them, then hashes the result. If a field is empty, it is
|
||||||
type getPIDFunc = func(mf model.MediaFile, md Metadata, spec string, prependLibId bool) string
|
// skipped and the function looks for the next field.
|
||||||
|
//
|
||||||
func createGetPID(hash hashFunc) getPIDFunc {
|
// Taking hash as a parameter (instead of closing over it in a factory) keeps
|
||||||
var getPID getPIDFunc
|
// mf on the stack: closing over mf would force the whole ~1KB MediaFile to the
|
||||||
getAttr := func(mf model.MediaFile, md Metadata, attr string, prependLibId bool, spec string) string {
|
// heap on every call.
|
||||||
attr = strings.TrimSpace(strings.ToLower(attr))
|
func computePID(mf model.MediaFile, md Metadata, spec string, prependLibId bool, hash hashFunc) string {
|
||||||
switch attr {
|
switch spec {
|
||||||
case "albumid":
|
case "track_legacy":
|
||||||
if spec == conf.Server.PID.Album {
|
return legacyTrackID(mf, prependLibId)
|
||||||
log.Error("Recursive PID definition detected, ignoring `albumid`", "spec", spec)
|
case "album_legacy":
|
||||||
return ""
|
return legacyAlbumID(mf, md, prependLibId)
|
||||||
}
|
}
|
||||||
return getPID(mf, md, conf.Server.PID.Album, prependLibId)
|
|
||||||
case "folder":
|
|
||||||
return filepath.Dir(mf.Path)
|
|
||||||
case "albumartistid":
|
|
||||||
return hash(str.Clear(strings.ToLower(mf.AlbumArtist)))
|
|
||||||
case "title":
|
|
||||||
return mf.Title
|
|
||||||
case "album":
|
|
||||||
return str.Clear(strings.ToLower(md.String(model.TagAlbum)))
|
|
||||||
}
|
|
||||||
return md.String(model.TagName(attr))
|
|
||||||
}
|
|
||||||
getPID = func(mf model.MediaFile, md Metadata, spec string, prependLibId bool) string {
|
|
||||||
pid := ""
|
pid := ""
|
||||||
fields := strings.SplitSeq(spec, "|")
|
fields := strings.SplitSeq(spec, "|")
|
||||||
for field := range fields {
|
for field := range fields {
|
||||||
attributes := strings.Split(field, ",")
|
attributes := strings.Split(field, ",")
|
||||||
|
values := make([]string, len(attributes))
|
||||||
hasValue := false
|
hasValue := false
|
||||||
values := slice.Map(attributes, func(attr string) string {
|
for i, attr := range attributes {
|
||||||
v := getAttr(mf, md, attr, prependLibId, spec)
|
v := getPIDAttr(mf, md, attr, prependLibId, spec, hash)
|
||||||
if v != "" {
|
if v != "" {
|
||||||
hasValue = true
|
hasValue = true
|
||||||
}
|
}
|
||||||
return v
|
values[i] = v
|
||||||
})
|
}
|
||||||
if hasValue {
|
if hasValue {
|
||||||
pid += strings.Join(values, "\\")
|
pid += strings.Join(values, "\\")
|
||||||
break
|
break
|
||||||
@ -71,29 +58,39 @@ func createGetPID(hash hashFunc) getPIDFunc {
|
|||||||
return hash(pid)
|
return hash(pid)
|
||||||
}
|
}
|
||||||
|
|
||||||
return func(mf model.MediaFile, md Metadata, spec string, prependLibId bool) string {
|
func getPIDAttr(mf model.MediaFile, md Metadata, attr string, prependLibId bool, spec string, hash hashFunc) string {
|
||||||
switch spec {
|
attr = strings.TrimSpace(strings.ToLower(attr))
|
||||||
case "track_legacy":
|
switch attr {
|
||||||
return legacyTrackID(mf, prependLibId)
|
case "albumid":
|
||||||
case "album_legacy":
|
if spec == conf.Server.PID.Album {
|
||||||
return legacyAlbumID(mf, md, prependLibId)
|
log.Error("Recursive PID definition detected, ignoring `albumid`", "spec", spec)
|
||||||
|
return ""
|
||||||
}
|
}
|
||||||
return getPID(mf, md, spec, prependLibId)
|
return computePID(mf, md, conf.Server.PID.Album, prependLibId, hash)
|
||||||
|
case "folder":
|
||||||
|
return filepath.Dir(mf.Path)
|
||||||
|
case "albumartistid":
|
||||||
|
return hash(str.Clear(strings.ToLower(mf.AlbumArtist)))
|
||||||
|
case "title":
|
||||||
|
return mf.Title
|
||||||
|
case "album":
|
||||||
|
return str.Clear(strings.ToLower(md.String(model.TagAlbum)))
|
||||||
}
|
}
|
||||||
|
return md.String(model.TagName(attr))
|
||||||
}
|
}
|
||||||
|
|
||||||
func (md Metadata) trackPID(mf model.MediaFile) string {
|
func (md Metadata) trackPID(mf model.MediaFile) string {
|
||||||
return createGetPID(id.NewHash)(mf, md, conf.Server.PID.Track, true)
|
return computePID(mf, md, conf.Server.PID.Track, true, id.NewHash)
|
||||||
}
|
}
|
||||||
|
|
||||||
func (md Metadata) albumID(mf model.MediaFile, pidConf string) string {
|
func (md Metadata) albumID(mf model.MediaFile, pidConf string) string {
|
||||||
return createGetPID(id.NewHash)(mf, md, pidConf, true)
|
return computePID(mf, md, pidConf, true, id.NewHash)
|
||||||
}
|
}
|
||||||
|
|
||||||
// BFR Must be configurable?
|
// BFR Must be configurable?
|
||||||
func (md Metadata) artistID(name string) string {
|
func (md Metadata) artistID(name string) string {
|
||||||
mf := model.MediaFile{AlbumArtist: name}
|
mf := model.MediaFile{AlbumArtist: name}
|
||||||
return createGetPID(id.NewHash)(mf, md, "albumartistid", false)
|
return computePID(mf, md, "albumartistid", false, id.NewHash)
|
||||||
}
|
}
|
||||||
|
|
||||||
func (md Metadata) mapTrackTitle() string {
|
func (md Metadata) mapTrackTitle() string {
|
||||||
|
|||||||
@ -15,12 +15,13 @@ var _ = Describe("getPID", func() {
|
|||||||
md Metadata
|
md Metadata
|
||||||
mf model.MediaFile
|
mf model.MediaFile
|
||||||
sum hashFunc
|
sum hashFunc
|
||||||
getPID getPIDFunc
|
|
||||||
)
|
)
|
||||||
|
getPID := func(mf model.MediaFile, md Metadata, spec string, prependLibId bool) string {
|
||||||
|
return computePID(mf, md, spec, prependLibId, sum)
|
||||||
|
}
|
||||||
|
|
||||||
BeforeEach(func() {
|
BeforeEach(func() {
|
||||||
sum = func(s ...string) string { return "(" + strings.Join(s, ",") + ")" }
|
sum = func(s ...string) string { return "(" + strings.Join(s, ",") + ")" }
|
||||||
getPID = createGetPID(sum)
|
|
||||||
})
|
})
|
||||||
|
|
||||||
Context("attributes are tags", func() {
|
Context("attributes are tags", func() {
|
||||||
|
|||||||
@ -217,7 +217,7 @@ func childFromMediaFile(ctx context.Context, mf model.MediaFile) responses.Child
|
|||||||
child.Path = fakePath(mf)
|
child.Path = fakePath(mf)
|
||||||
}
|
}
|
||||||
child.DiscNumber = int32(mf.DiscNumber)
|
child.DiscNumber = int32(mf.DiscNumber)
|
||||||
child.Created = &mf.BirthTime
|
child.Created = P(mf.BirthTime)
|
||||||
child.AlbumId = mf.AlbumID
|
child.AlbumId = mf.AlbumID
|
||||||
child.ArtistId = mf.ArtistID
|
child.ArtistId = mf.ArtistID
|
||||||
child.Type = "music"
|
child.Type = "music"
|
||||||
@ -407,9 +407,12 @@ func buildDiscSubtitles(a model.Album) []responses.DiscTitle {
|
|||||||
return nil
|
return nil
|
||||||
}
|
}
|
||||||
var discTitles []responses.DiscTitle
|
var discTitles []responses.DiscTitle
|
||||||
|
// Hoist UpdatedAt to a single stack-local so &updatedAt doesn't force the
|
||||||
|
// whole model.Album parameter onto the heap.
|
||||||
|
updatedAt := a.UpdatedAt
|
||||||
for num, title := range a.Discs {
|
for num, title := range a.Discs {
|
||||||
artID := model.NewArtworkID(model.KindDiscArtwork,
|
artID := model.NewArtworkID(model.KindDiscArtwork,
|
||||||
model.DiscArtworkID(a.ID, num), &a.UpdatedAt)
|
model.DiscArtworkID(a.ID, num), &updatedAt)
|
||||||
discTitles = append(discTitles, responses.DiscTitle{
|
discTitles = append(discTitles, responses.DiscTitle{
|
||||||
Disc: int32(num),
|
Disc: int32(num),
|
||||||
Title: title,
|
Title: title,
|
||||||
|
|||||||
Loading…
x
Reference in New Issue
Block a user