Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added config option to set size of transcode cache and cadence to enf… #520

Merged
merged 6 commits into from
Sep 15, 2024

Conversation

brian-doherty
Copy link
Contributor

…orce that sizing via ejection.

@brian-doherty
Copy link
Contributor Author

Resolves #501

@brian-doherty
Copy link
Contributor Author

@sentriz Would love to get your feedback on this when you get a minute. Thanks.

@sentriz
Copy link
Owner

sentriz commented Jul 12, 2024

hiya Brian sorry for the delay . will hopefully take a look shortly

@zpuskas
Copy link
Contributor

zpuskas commented Jul 16, 2024

@brian-doherty Could you please also update contrib/config accordingly with this new option?

@brian-doherty
Copy link
Contributor Author

@brian-doherty Could you please also update contrib/config accordingly with this new option?

Done. Lint and test are running now but will pass.

@brian-doherty
Copy link
Contributor Author

@sentriz I know you're busy but I would love you to have a look at this PR when you get a sec.

Copy link
Owner

@sentriz sentriz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good! just a couple things

@@ -64,6 +68,41 @@ func (t *CachingTranscoder) Transcode(ctx context.Context, profile Profile, in s
return nil
}

func (t *CachingTranscoder) CacheEject() {
Copy link
Owner

@sentriz sentriz Sep 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could return an error, which we can just log in main.go

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

var files []file
var total int64 = 0

_ = filepath.Walk(t.cachePath, func(path string, info os.FileInfo, err error) error {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use the new (and faster i belive) filepath.WalkDir here?

and return an error too

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

curFile := files[0]
files = files[1:]
total -= curFile.info.Size()
unlock := t.locks.Lock(curFile.path)
Copy link
Owner

@sentriz sentriz Sep 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the t.locks is a keyedMutex but the keys are profile strings (ffmpeg commands with args and paths) not paths. so this has little effect i think

maybe we should have a global clean lock which the normal transcode thing respects too?

something like

type CachingTranscoder struct {
    ...
    locks      keyedMutex
    cleanLock  sync.RWMutex
}

...

func (t *CachingTranscoder) Transcode(...) error {
    ...
    
    t.cleanLock.RLock()
    defer t.cleanLock.RUnlock()
    
    ...
    
    key := cacheKey(name, args)
    unlock := t.locks.Lock(key)
    defer unlock()
}

func (t *CachingTranscoder) CacheEject() error {
    t.cleanLock.Lock()
    defer t.cleanLock.Unlock()

    ...
}

that way the cleaning process locks the entire cache, while multiple goroutines looking to read the cache work fine, just not while we're cleaning

wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call and my apologies. Done.

locks keyedMutex
}

var _ Transcoder = (*CachingTranscoder)(nil)

func NewCachingTranscoder(t Transcoder, cachePath string) *CachingTranscoder {
return &CachingTranscoder{transcoder: t, cachePath: cachePath}
func NewCachingTranscoder(t Transcoder, cachePath string, limitMb int) *CachingTranscoder {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks megabits to me. how about a big B for bytes

Suggested change
func NewCachingTranscoder(t Transcoder, cachePath string, limitMb int) *CachingTranscoder {
func NewCachingTranscoder(t Transcoder, cachePath string, limitMB int) *CachingTranscoder {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@brian-doherty
Copy link
Contributor Author

@sentriz All comments addressed. See what you think and thanks!

cmd/gonic/gonic Outdated
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like this was committed accidentally

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad. Fixed.

@sentriz
Copy link
Owner

sentriz commented Sep 15, 2024

looks great, thanks! sorry for the delay

@sentriz sentriz merged commit bcb613c into sentriz:master Sep 15, 2024
1 check passed
@brian-doherty brian-doherty deleted the cacheeject branch September 15, 2024 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants