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

lru: use weak pointers in LRU stack #49

Merged
merged 3 commits into from
Feb 26, 2025
Merged

Conversation

dfinkel
Copy link

@dfinkel dfinkel commented Feb 11, 2025

  • lru: use weak pointers in LRU stack
    Galaxycache isn't always the most friendly to the Go Garbage Collector.
    By keeping a whole pile of pointers in a linked list that the GC needs
    to scan in its mark phase, we're compromising the tail latency of any
    processes that maintain large numbers of objects in their caches.

    Fortunately, Go 1.24 provides weak pointers, so as long as we keep an
    "owning" reference in the cache's map, we can make all next/previous
    pointers weak pointers and avoid a whole bunch of extra GC work in its
    mark phase. (note: this optimization requires that the weak pointers be
    at the end of the struct so the GC can skip scanning them if there are
    pointers in the value -- if there aren't, then the GC can skip the
    linked list entries entirely -- not likely unless one has integer keys)

  • actions: add go 1.24 to the matrix

Galaxycache isn't always the most friendly to the Go Garbage Collector.
By keeping a whole pile of pointers in a linked list that the GC needs
to scan in its mark phase, we're compromising the tail latency of any
processes that maintain large numbers of objects in their caches.

Fortunately, Go 1.24 provides weak pointers, so as long as we keep an
"owning" reference in the cache's map, we can make all next/previous
pointers weak pointers and avoid a whole bunch of extra GC work in its
mark phase. (note: this optimization requires that the weak pointers be
at the end of the struct so the GC can skip scanning them if there are
pointers in the value -- if there aren't, then the GC can skip the
linked list entries entirely -- not likely unless one has integer keys)
@dfinkel dfinkel force-pushed the weakptr_lru_stack_1.24 branch from 55a1651 to f9cf640 Compare February 11, 2025 20:39
Copy link
Member

@arodland arodland left a comment

Choose a reason for hiding this comment

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

Diff between the non-weak and weak linked-list implementations
--- lru/typed_ll.go	2025-02-11 21:50:13.119602204 +0000
+++ lru/typed_ll_weak.go	2025-02-11 21:50:13.119602204 +0000
@@ -1,7 +1,7 @@
-//go:build go1.18 && !go1.24
+//go:build go1.24
 
 /*
-Copyright 2022 Vimeo Inc.
+Copyright 2025 Vimeo Inc.
 
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.
@@ -18,7 +18,10 @@
 
 package lru
 
-import "fmt"
+import (
+	"fmt"
+	"weak"
+)
 
 // Default-disable paranoid checks so they get compiled out.
 // If this const is renamed/moved/updated make sure to update the sed
@@ -27,23 +30,25 @@
 
 // LinkedList using generics to reduce the number of heap objects
 // Used for the LRU stack in TypedCache
+// This implementation switches to using weak pointers when using go 1.24+ so
+// the GC can skip scanning the linked list elements themselves.
 type linkedList[T any] struct {
-	head *llElem[T]
-	tail *llElem[T]
+	head weak.Pointer[llElem[T]]
+	tail weak.Pointer[llElem[T]]
 	size int
 }
 
 type llElem[T any] struct {
-	next, prev *llElem[T]
 	value      T
+	next, prev weak.Pointer[llElem[T]]
 }
 
 func (l *llElem[T]) Next() *llElem[T] {
-	return l.next
+	return l.next.Value()
 }
 
 func (l *llElem[T]) Prev() *llElem[T] {
-	return l.prev
+	return l.prev.Value()
 }
 
 func (l *linkedList[T]) PushFront(val T) *llElem[T] {
@@ -53,16 +58,17 @@
 	}
 	elem := llElem[T]{
 		next:  l.head,
-		prev:  nil, // first element
+		prev:  weak.Pointer[llElem[T]]{}, // first element
 		value: val,
 	}
-	if l.head != nil {
-		l.head.prev = &elem
+	weakElem := weak.Make(&elem)
+	if lHead := l.head.Value(); lHead != nil {
+		lHead.prev = weakElem
 	}
-	if l.tail == nil {
-		l.tail = &elem
+	if lTail := l.tail.Value(); lTail == nil {
+		l.tail = weakElem
 	}
-	l.head = &elem
+	l.head = weakElem
 	l.size++
 
 	return &elem
@@ -77,49 +83,52 @@
 		defer l.checkHeadTail()
 	}
 
-	if l.head == e {
+	extHead := l.head.Value()
+
+	if extHead == e {
 		// nothing to do
 		return
 	}
+	eWeak := weak.Make(e)
 
-	if e.next != nil {
+	if eNext := e.next.Value(); eNext != nil {
 		// update the previous pointer on the next element
-		e.next.prev = e.prev
+		eNext.prev = e.prev
 	}
-	if e.prev != nil {
-		e.prev.next = e.next
+	if ePrev := e.prev.Value(); ePrev != nil {
+		ePrev.next = e.next
 	}
-	if l.head != nil {
-		l.head.prev = e
+	if lHead := l.head.Value(); lHead != nil {
+		lHead.prev = eWeak
 	}
 
-	if l.tail == e {
+	if lTail := l.tail.Value(); lTail == e {
 		l.tail = e.prev
 	}
 	e.next = l.head
-	l.head = e
-	e.prev = nil
+	l.head = eWeak
+	e.prev = weak.Pointer[llElem[T]]{}
 }
 
 func (l *linkedList[T]) checkHeadTail() {
 	if !paranoidLL {
 		return
 	}
-	if (l.head != nil) != (l.tail != nil) {
+	if (l.head.Value() != nil) != (l.tail.Value() != nil) {
 		panic(fmt.Sprintf("invariant failure; nilness mismatch: head: %+v; tail: %+v (size %d)",
 			l.head, l.tail, l.size))
 	}
 
-	if l.size > 0 && (l.head == nil || l.tail == nil) {
+	if l.size > 0 && (l.head.Value() == nil || l.tail.Value() == nil) {
 		panic(fmt.Sprintf("invariant failure; head and/or tail nil with %d size: head: %+v; tail: %+v",
 			l.size, l.head, l.tail))
 	}
 
-	if l.head != nil && (l.head.prev != nil || (l.head.next == nil && l.size != 1)) {
+	if lHead := l.head.Value(); lHead != nil && (lHead.prev.Value() != nil || (lHead.next.Value() == nil && l.size != 1)) {
 		panic(fmt.Sprintf("invariant failure; head next/prev invalid with %d size: head: %+v; tail: %+v",
 			l.size, l.head, l.tail))
 	}
-	if l.tail != nil && ((l.tail.prev == nil && l.size != 1) || l.tail.next != nil) {
+	if lTail := l.tail.Value(); lTail != nil && ((lTail.prev.Value() == nil && l.size != 1) || lTail.next.Value() != nil) {
 		panic(fmt.Sprintf("invariant failure; tail next/prev invalid with %d size: head: %+v; tail: %+v",
 			l.size, l.head, l.tail))
 	}
@@ -133,19 +142,19 @@
 		l.checkHeadTail()
 		defer l.checkHeadTail()
 	}
-	if l.tail == e {
+	if l.tail.Value() == e {
 		l.tail = e.prev
 	}
-	if l.head == e {
+	if l.head.Value() == e {
 		l.head = e.next
 	}
 
-	if e.next != nil {
+	if eNext := e.next.Value(); eNext != nil {
 		// update the previous pointer on the next element
-		e.next.prev = e.prev
+		eNext.prev = e.prev
 	}
-	if e.prev != nil {
-		e.prev.next = e.next
+	if ePrev := e.prev.Value(); ePrev != nil {
+		ePrev.next = e.next
 	}
 	l.size--
 }
@@ -155,9 +164,9 @@
 }
 
 func (l *linkedList[T]) Front() *llElem[T] {
-	return l.head
+	return l.head.Value()
 }
 
 func (l *linkedList[T]) Back() *llElem[T] {
-	return l.tail
+	return l.tail.Value()
 }
which is basically a straightforward replacement. Looks good to me, let's see how happy it makes the GC.

@dfinkel dfinkel merged commit 7051bd0 into master Feb 26, 2025
30 checks passed
@dfinkel dfinkel deleted the weakptr_lru_stack_1.24 branch February 26, 2025 20:50
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