Skip to content

Preparestmt use LRU Map instead default map #7435

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

Merged
merged 22 commits into from
Apr 25, 2025
Merged

Conversation

Yaccc
Copy link
Contributor

@Yaccc Yaccc commented Apr 24, 2025

GROM default map may be cause memory leak

What did this pull request do?

PrepareStmt support use lrumap instead default Map
PreparedStmtDB stmt change to interface to defined maps

type StmtStore interface {
	Get(key string) (*Stmt, bool)
	Set(key string, value *Stmt)
	Delete(key string)
	AllMap() map[string]*Stmt
}
func NewPreparedStmtDB(connPool ConnPool, prepareStmtLruConfig *PrepareStmtLruConfig) *PreparedStmtDB {
	return &PreparedStmtDB{
		ConnPool: connPool,
		Stmts: func() StmtStore {
			var stmts StmtStore
			if prepareStmtLruConfig != nil && prepareStmtLruConfig.Open {
				lru := &LruStmtStore{}
				lru.NewLru(prepareStmtLruConfig.Size, prepareStmtLruConfig.TTL)
				stmts = lru
			} else {
				stmts = &DefaultStmtStore{}
			}
			return stmts
		}(),
		Mux: &sync.RWMutex{},
	}
}
  1. original maps use defaultStmtStore,lur maps use LruStmtStore
  2. PrepareStmtLruConfig init lru size and expired time
  3. size must lt zero ande default ttl is
const noEvictionTTL = time.Hour * 24 * 365 * 10
const DEFAULT_MAX_SIZE = (1 << 63) - 1
const DEFAULT_TTL = time.Hour * 24
	// PrepareStmt cache support LRU expired
	PrepareStmtMaxSize int
	PrepareStmtTTL     time.Duration

User Case Description

TestPreparedStmtLruFromTransaction check lru active

conn, ok := tx.ConnPool.(*gorm.PreparedStmtDB)
	lens := len(conn.Stmts.AllMap())
	if lens == 0 {
		t.Fatalf("lru should not be empty")
	}
	time.Sleep(time.Second * 40)
	AssertEqual(t, ok, true)
	AssertEqual(t, len(conn.Stmts.AllMap()), 0)

gorm.go Outdated
@@ -65,6 +69,11 @@ type Config struct {
callbacks *callbacks
cacheStore *sync.Map
}
type PrepareStmtLruConfig struct {
Copy link
Member

Choose a reason for hiding this comment

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

This is no longer needed?

Copy link
Contributor Author

@Yaccc Yaccc Apr 24, 2025

Choose a reason for hiding this comment

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

already deleted

gorm.go Outdated
@@ -268,7 +277,7 @@ func (db *DB) Session(config *Session) *DB {
if v, ok := db.cacheStore.Load(preparedStmtDBKey); ok {
preparedStmt = v.(*PreparedStmtDB)
} else {
preparedStmt = NewPreparedStmtDB(db.ConnPool)
preparedStmt = NewPreparedStmtDB(db.ConnPool, db.Config.PrepareStmtMaxSize, db.Config.PrepareStmtTTL)
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to allow session-level configuration? For example, configure it here: https://github.com/go-gorm/gorm/blob/master/gorm.go#L107

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if config.PrepareStmt {
		preparedStmt := NewPreparedStmtDB(db.ConnPool, config.PrepareStmtMaxSize, config.PrepareStmtTTL)
		db.cacheStore.Store(preparedStmtDBKey, preparedStmt)
		db.ConnPool = preparedStmt
	}

session default use static config

prepare_stmt.go Outdated
Mux *sync.RWMutex
ConnPool
}

func NewPreparedStmtDB(connPool ConnPool) *PreparedStmtDB {
const DEFAULT_MAX_SIZE = (1 << 63) - 1
Copy link
Member

Choose a reason for hiding this comment

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

Could we relocate the majority of the following code to an internal package, such as internal/stmt_store, to prevent the exposure of additional structs or methods?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cause circular dependency

@Yaccc Yaccc changed the title Preparestmt use lur instead default map Preparestmt use LRU Map instead default map Apr 24, 2025
@jinzhu jinzhu merged commit a827495 into master Apr 25, 2025
23 of 41 checks passed
@jinzhu jinzhu deleted the feature/sup_lru_prepareStmt branch April 25, 2025 08:22
@iTanken
Copy link
Contributor

iTanken commented Apr 27, 2025

Build failed in 32bit environment

a827495#r155931475

GOARCH=386 go test -timeout 20m -v ./...
  shell: /usr/bin/bash -e {0}
# gorm.io/gorm/internal/stmt_store
Error: 
../../../go/pkg/mod/gorm.io/[email protected]/internal/stmt_store/stmt_store.go:99:10: 
cannot use defaultMaxSize (untyped int constant 9223372036854775807) as int value in assignment (overflows)

iTanken added a commit to iTanken/gorm that referenced this pull request Apr 27, 2025
@andig
Copy link

andig commented May 3, 2025

We've just hit this as well in #7442. A .1 release would be highly appreciated.

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.

4 participants