Adopt Turso-inspired performance patterns#490
Conversation
…che) - Implement batch insertion methods in `CodebaseDb` (`insert_chunks_batch`, `insert_symbols_batch`, `insert_embeddings_batch`) using `libsql` transactions. - Implement lazy schema initialization in `CodebaseDb` and `ConversationsDb` using `tokio::sync::OnceCell` to defer table creation until first use. - Refactor `ConnectionManager` to use `libsql` and `moka::future::Cache` for a unified LRU connection pool with a capacity of 10 and 30-minute idle timeout. - Optimize `populate_fts` in `CodebaseDb` by using native async `execute_batch` instead of redundant `spawn_blocking`. - Standardize on `libsql` driver across the codebase module. - Add unit tests for batch operations and verify existing database tests.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Warning Review limit reached
More reviews will be available in 60 minutes. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request transitions the codebase connection management from r2d2 and rusqlite to libsql and moka::future::Cache, introduces lazy schema initialization via OnceCell across database managers, and adds batch insertion methods for symbols, chunks, and embeddings. Feedback on these changes includes addressing a potential race condition in get_connection by using try_get_with for atomic initialization, optimizing batch insertion methods by preparing SQL statements outside of loops, and ensuring populate_fts triggers schema initialization to prevent runtime failures.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| pub async fn get_connection(&self, project_id: &str, project_root: &str) -> Result<Arc<Connection>> { | ||
| if let Some(conn) = self.conns.get(project_id).await { | ||
| return Ok(conn); | ||
| } | ||
|
|
||
| let db_path = if project_id == "memory" { | ||
| PathBuf::from(project_root).join("xavier_memory.db") | ||
| } else if project_id == "vec_store" { | ||
| PathBuf::from(project_root).join("vec-store.sqlite3") | ||
| } else if project_id == "metrics" { | ||
| PathBuf::from(project_root).join("metrics.db") | ||
| } else if project_id.starts_with("conv_") { | ||
| super::conversations_db::ConversationsDb::db_path(project_id.strip_prefix("conv_").unwrap()) | ||
| } else { | ||
| if let Some(mut pool) = self.pools.get_mut(project_id) { | ||
| pool.activated_at = Instant::now(); | ||
| } | ||
| PathBuf::from(project_root).join(".xavier").join("codebase.db") | ||
| }; | ||
|
|
||
| if let Some(parent) = db_path.parent() { | ||
| std::fs::create_dir_all(parent) | ||
| .with_context(|| format!("failed to create parent dir for {:?}", db_path))?; | ||
| } | ||
| Ok(()) | ||
|
|
||
| let path_str = db_path.to_string_lossy().to_string(); | ||
| let db = libsql::Builder::new_local(&path_str) | ||
| .build() | ||
| .await | ||
| .with_context(|| format!("failed to open database at {}", db_path.display()))?; | ||
|
|
||
| let conn = db.connect().context("failed to connect to libSQL database")?; | ||
|
|
||
| // Apply Pragmas | ||
| conn.execute_batch( | ||
| "PRAGMA journal_mode = WAL; | ||
| PRAGMA synchronous = NORMAL; | ||
| PRAGMA foreign_keys = ON; | ||
| PRAGMA busy_timeout = 5000;", | ||
| ).await.context("failed to set PRAGMAs")?; | ||
|
|
||
| let conn_arc = Arc::new(conn); | ||
| self.conns.insert(project_id.to_string(), conn_arc.clone()).await; | ||
| Ok(conn_arc) | ||
| } |
There was a problem hiding this comment.
Under high concurrency, multiple tasks calling get_connection for the same project_id simultaneously can cause a race condition where multiple connections are opened and overwrite each other in the cache. Using try_get_with from moka::future::Cache ensures atomic connection initialization.
pub async fn get_connection(&self, project_id: &str, project_root: &str) -> Result<Arc<Connection>> {
self.conns
.try_get_with(project_id.to_string(), async {
let db_path = if project_id == "memory" {
PathBuf::from(project_root).join("xavier_memory.db")
} else if project_id == "vec_store" {
PathBuf::from(project_root).join("vec-store.sqlite3")
} else if project_id == "metrics" {
PathBuf::from(project_root).join("metrics.db")
} else if project_id.starts_with("conv_") {
super::conversations_db::ConversationsDb::db_path(project_id.strip_prefix("conv_").unwrap())
} else {
PathBuf::from(project_root).join(".xavier").join("codebase.db")
};
if let Some(parent) = db_path.parent() {
std::fs::create_dir_all(parent)
.with_context(|| format!("failed to create parent dir for {:?}", db_path))?;
}
let path_str = db_path.to_string_lossy().to_string();
let db = libsql::Builder::new_local(&path_str)
.build()
.await
.with_context(|| format!("failed to open database at {}", db_path.display()))?;
let conn = db.connect().context("failed to connect to libSQL database")?;
// Apply Pragmas
conn.execute_batch(
"PRAGMA journal_mode = WAL;
PRAGMA synchronous = NORMAL;
PRAGMA foreign_keys = ON;
PRAGMA busy_timeout = 5000;",
).await.context("failed to set PRAGMAs")?;
Ok(Arc::new(conn))
})
.await
.map_err(|e| anyhow::anyhow!("failed to get connection: {}", e))
}| pub async fn insert_symbols_batch(&self, symbols: &[SymbolInput]) -> Result<()> { | ||
| self.ensure_schema().await?; | ||
| let tx = self.conn.transaction().await.context("failed to start transaction")?; | ||
| for s in symbols { | ||
| tx.execute( | ||
| "INSERT OR REPLACE INTO symbols | ||
| (id, name, kind, file_path, line_start, line_end, signature, visibility, doc_comment, language, module_path, complexity) | ||
| VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9, ?10, ?11, ?12)", | ||
| params![ | ||
| s.id.clone(), s.name.clone(), s.kind.clone(), s.file_path.clone(), | ||
| s.line_start, s.line_end, s.signature.clone(), s.visibility.clone(), | ||
| s.doc_comment.clone(), s.language.clone(), s.module_path.clone(), s.complexity | ||
| ], | ||
| ).await.context("failed to insert symbol in batch")?; | ||
| } | ||
| tx.commit().await.context("failed to commit transaction")?; | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Executing tx.execute in a loop compiles the SQL query on every iteration. Preparing the statement once outside the loop is much more efficient for batch inserts.
| pub async fn insert_symbols_batch(&self, symbols: &[SymbolInput]) -> Result<()> { | |
| self.ensure_schema().await?; | |
| let tx = self.conn.transaction().await.context("failed to start transaction")?; | |
| for s in symbols { | |
| tx.execute( | |
| "INSERT OR REPLACE INTO symbols | |
| (id, name, kind, file_path, line_start, line_end, signature, visibility, doc_comment, language, module_path, complexity) | |
| VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9, ?10, ?11, ?12)", | |
| params![ | |
| s.id.clone(), s.name.clone(), s.kind.clone(), s.file_path.clone(), | |
| s.line_start, s.line_end, s.signature.clone(), s.visibility.clone(), | |
| s.doc_comment.clone(), s.language.clone(), s.module_path.clone(), s.complexity | |
| ], | |
| ).await.context("failed to insert symbol in batch")?; | |
| } | |
| tx.commit().await.context("failed to commit transaction")?; | |
| Ok(()) | |
| } | |
| pub async fn insert_symbols_batch(&self, symbols: &[SymbolInput]) -> Result<()> { | |
| self.ensure_schema().await?; | |
| let tx = self.conn.transaction().await.context("failed to start transaction")?; | |
| let mut stmt = tx.prepare( | |
| "INSERT OR REPLACE INTO symbols | |
| (id, name, kind, file_path, line_start, line_end, signature, visibility, doc_comment, language, module_path, complexity) | |
| VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9, ?10, ?11, ?12)" | |
| ).await.context("failed to prepare statement")?; | |
| for s in symbols { | |
| stmt.execute(params![ | |
| s.id.clone(), s.name.clone(), s.kind.clone(), s.file_path.clone(), | |
| s.line_start, s.line_end, s.signature.clone(), s.visibility.clone(), | |
| s.doc_comment.clone(), s.language.clone(), s.module_path.clone(), s.complexity | |
| ]).await.context("failed to insert symbol in batch")?; | |
| } | |
| tx.commit().await.context("failed to commit transaction")?; | |
| Ok(()) | |
| } |
| /// Insert multiple code chunks in a single transaction. | ||
| pub async fn insert_chunks_batch(&self, chunks: &[ChunkInput]) -> Result<()> { | ||
| self.ensure_schema().await?; | ||
| let tx = self.conn.transaction().await.context("failed to start transaction")?; | ||
| for c in chunks { | ||
| tx.execute( | ||
| "INSERT OR REPLACE INTO code_chunks (id, path, content, language, symbol_id, tokens) | ||
| VALUES (?1, ?2, ?3, ?4, ?5, ?6)", | ||
| params![c.id.clone(), c.path.clone(), c.content.clone(), c.language.clone(), c.symbol_id.clone(), c.tokens], | ||
| ).await.context("failed to insert chunk in batch")?; | ||
| } | ||
| tx.commit().await.context("failed to commit transaction")?; | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Preparing the statement once outside the loop is much more efficient than calling tx.execute repeatedly for each chunk.
| /// Insert multiple code chunks in a single transaction. | |
| pub async fn insert_chunks_batch(&self, chunks: &[ChunkInput]) -> Result<()> { | |
| self.ensure_schema().await?; | |
| let tx = self.conn.transaction().await.context("failed to start transaction")?; | |
| for c in chunks { | |
| tx.execute( | |
| "INSERT OR REPLACE INTO code_chunks (id, path, content, language, symbol_id, tokens) | |
| VALUES (?1, ?2, ?3, ?4, ?5, ?6)", | |
| params![c.id.clone(), c.path.clone(), c.content.clone(), c.language.clone(), c.symbol_id.clone(), c.tokens], | |
| ).await.context("failed to insert chunk in batch")?; | |
| } | |
| tx.commit().await.context("failed to commit transaction")?; | |
| Ok(()) | |
| } | |
| /// Insert multiple code chunks in a single transaction. | |
| pub async fn insert_chunks_batch(&self, chunks: &[ChunkInput]) -> Result<()> { | |
| self.ensure_schema().await?; | |
| let tx = self.conn.transaction().await.context("failed to start transaction")?; | |
| let mut stmt = tx.prepare( | |
| "INSERT OR REPLACE INTO code_chunks (id, path, content, language, symbol_id, tokens) | |
| VALUES (?1, ?2, ?3, ?4, ?5, ?6)" | |
| ).await.context("failed to prepare statement")?; | |
| for c in chunks { | |
| stmt.execute(params![ | |
| c.id.clone(), c.path.clone(), c.content.clone(), c.language.clone(), c.symbol_id.clone(), c.tokens | |
| ]).await.context("failed to insert chunk in batch")?; | |
| } | |
| tx.commit().await.context("failed to commit transaction")?; | |
| Ok(()) | |
| } |
| /// Insert multiple embedding vectors in a single transaction. | ||
| pub async fn insert_embeddings_batch(&self, embeddings: &[EmbeddingInput]) -> Result<()> { | ||
| self.ensure_schema().await?; | ||
| let tx = self.conn.transaction().await.context("failed to start transaction")?; | ||
| for e in embeddings { | ||
| let embedding_blob = crate::memory::sqlite_vec_store::vector::serialize_embedding(&e.embedding); | ||
| tx.execute( | ||
| "INSERT INTO code_embeddings (id, embedding) VALUES (?1, ?2)", | ||
| params![e.id.clone(), embedding_blob], | ||
| ).await.context("failed to insert embedding in batch")?; | ||
| } | ||
| tx.commit().await.context("failed to commit transaction")?; | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Preparing the statement once outside the loop is much more efficient than calling tx.execute repeatedly for each embedding.
| /// Insert multiple embedding vectors in a single transaction. | |
| pub async fn insert_embeddings_batch(&self, embeddings: &[EmbeddingInput]) -> Result<()> { | |
| self.ensure_schema().await?; | |
| let tx = self.conn.transaction().await.context("failed to start transaction")?; | |
| for e in embeddings { | |
| let embedding_blob = crate::memory::sqlite_vec_store::vector::serialize_embedding(&e.embedding); | |
| tx.execute( | |
| "INSERT INTO code_embeddings (id, embedding) VALUES (?1, ?2)", | |
| params![e.id.clone(), embedding_blob], | |
| ).await.context("failed to insert embedding in batch")?; | |
| } | |
| tx.commit().await.context("failed to commit transaction")?; | |
| Ok(()) | |
| } | |
| /// Insert multiple embedding vectors in a single transaction. | |
| pub async fn insert_embeddings_batch(&self, embeddings: &[EmbeddingInput]) -> Result<()> { | |
| self.ensure_schema().await?; | |
| let tx = self.conn.transaction().await.context("failed to start transaction")?; | |
| let mut stmt = tx.prepare( | |
| "INSERT INTO code_embeddings (id, embedding) VALUES (?1, ?2)" | |
| ).await.context("failed to prepare statement")?; | |
| for e in embeddings { | |
| let embedding_blob = crate::memory::sqlite_vec_store::vector::serialize_embedding(&e.embedding); | |
| stmt.execute(params![e.id.clone(), embedding_blob]).await.context("failed to insert embedding in batch")?; | |
| } | |
| tx.commit().await.context("failed to commit transaction")?; | |
| Ok(()) | |
| } |
| /// Populate the FTS index from code chunks. | ||
| pub async fn populate_fts(&self) -> Result<()> { | ||
| let sql = populate_fts_from_chunks_sql(); | ||
| self.conn | ||
| .execute_batch(&sql) | ||
| .await | ||
| .map_err(|e| anyhow::anyhow!("failed to populate FTS: {}", e))?; | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
populate_fts does not call self.ensure_schema().await? before executing the batch SQL. If it is called before any other database operation, it could fail because the tables are not yet created.
/// Populate the FTS index from code chunks.
pub async fn populate_fts(&self) -> Result<()> {
self.ensure_schema().await?;
let sql = populate_fts_from_chunks_sql();
self.conn
.execute_batch(&sql)
.await
.map_err(|e| anyhow::anyhow!("failed to populate FTS: {}", e))?;
Ok(())
}
iberi22
added
jules
|
Closed in favor of #489 - more complete implementation (batch inserts, lazy schema, LRU cache) |
1 participant
Adopted several performance patterns inspired by Turso to improve Xavier's database handling:
CodebaseDb. This significantly speeds up initial indexing of large repositories.CodebaseDbandConversationsDbto usetokio::sync::OnceCellfor schema creation. Tables are now created only when the first database operation is performed, saving resources for inactive projects.ConnectionManagerto usemoka::future::Cache. It now manages up to 10 activelibsqlconnections with a 30-minute idle timeout, providing a proper LRU eviction policy.ConnectionManagerfromrusqlite/r2d2tolibsqlto match the rest of thecodebasemodule, ensuring architectural consistency.populate_ftsto uselibsql's async capabilities directly, avoiding the overhead ofspawn_blockingwhere it was redundant.All changes have been verified with the internal test suite for the
codebasemodule.Fixes #427
PR created automatically by Jules for task 17406721068983009540 started by @iberi22