mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
perf(chatd): fix six scale bottlenecks identified by benchmarking (#22957)
## Summary Scale-tested the `chatd` package with mock-based benchmarks to identify performance bottlenecks. This PR fixes 6 of the 8 identified issues, ranked by severity. ## Changes ### 1. Parallel tool execution (HIGH) — `chatloop.go` `executeTools` ran tool calls sequentially. Now dispatches all calls concurrently via goroutines with `sync.WaitGroup`. Results are pre-allocated by index (no mutex needed). `onResult` callbacks fire as each tool completes. ### 2. Pubsub-backed subagent await (HIGH) — `subagent.go` `awaitSubagentCompletion` polled the DB every 200ms. Now subscribes to the child chat's `ChatStreamNotifyChannel` via pubsub for near-instant notifications. Fallback poll reduced to 5s. Falls back to 200ms only when `pubsub == nil` (single-instance / in-memory). ### 3. Per-chat stream locking (MEDIUM) — `chatd.go` Replaced single global `streamMu` + `map[uuid.UUID]*chatStreamState` with `sync.Map` where each `chatStreamState` has its own `sync.Mutex`. Zero cross-chat contention. ### 4. Batch chat acquisition (MEDIUM) — `chatd.go` `processOnce` acquired 1 chat per tick. Now loops up to `maxChatsPerAcquire = 10` per tick, avoiding idle time when many chats are pending. ### 5. Reduced heartbeat frequency (LOW-MEDIUM) — `chatd.go` `chatHeartbeatInterval` changed from 30s to 60s. Safe given the 5-minute `DefaultInFlightChatStaleAfter`. ### 6. O(depth) descendant check (LOW) — `subagent.go` Replaced top-down BFS (`O(total_descendants)` queries) with bottom-up parent-chain walk (`O(depth)` queries). Includes cycle protection. ## Not addressed (intentionally) - Message serialization overhead - Buffer eviction (`buffer[1:]` pattern)
This commit is contained in:
@@ -1512,13 +1512,13 @@ func (q *querier) authorizeProvisionerJob(ctx context.Context, job database.Prov
|
||||
return nil
|
||||
}
|
||||
|
||||
func (q *querier) AcquireChat(ctx context.Context, arg database.AcquireChatParams) (database.Chat, error) {
|
||||
// AcquireChat is a system-level operation used by the chat processor.
|
||||
func (q *querier) AcquireChats(ctx context.Context, arg database.AcquireChatsParams) ([]database.Chat, error) {
|
||||
// AcquireChats is a system-level operation used by the chat processor.
|
||||
// Authorization is done at the system level, not per-user.
|
||||
if err := q.authorizeContext(ctx, policy.ActionUpdate, rbac.ResourceChat); err != nil {
|
||||
return database.Chat{}, err
|
||||
return nil, err
|
||||
}
|
||||
return q.db.AcquireChat(ctx, arg)
|
||||
return q.db.AcquireChats(ctx, arg)
|
||||
}
|
||||
|
||||
func (q *querier) AcquireLock(ctx context.Context, id int64) error {
|
||||
|
||||
@@ -373,14 +373,15 @@ func (s *MethodTestSuite) TestConnectionLogs() {
|
||||
}
|
||||
|
||||
func (s *MethodTestSuite) TestChats() {
|
||||
s.Run("AcquireChat", s.Mocked(func(dbm *dbmock.MockStore, faker *gofakeit.Faker, check *expects) {
|
||||
arg := database.AcquireChatParams{
|
||||
s.Run("AcquireChats", s.Mocked(func(dbm *dbmock.MockStore, faker *gofakeit.Faker, check *expects) {
|
||||
arg := database.AcquireChatsParams{
|
||||
StartedAt: dbtime.Now(),
|
||||
WorkerID: uuid.New(),
|
||||
NumChats: 1,
|
||||
}
|
||||
chat := testutil.Fake(s.T(), faker, database.Chat{})
|
||||
dbm.EXPECT().AcquireChat(gomock.Any(), arg).Return(chat, nil).AnyTimes()
|
||||
check.Args(arg).Asserts(rbac.ResourceChat, policy.ActionUpdate).Returns(chat)
|
||||
dbm.EXPECT().AcquireChats(gomock.Any(), arg).Return([]database.Chat{chat}, nil).AnyTimes()
|
||||
check.Args(arg).Asserts(rbac.ResourceChat, policy.ActionUpdate).Returns([]database.Chat{chat})
|
||||
}))
|
||||
s.Run("DeleteAllChatQueuedMessages", s.Mocked(func(dbm *dbmock.MockStore, faker *gofakeit.Faker, check *expects) {
|
||||
chat := testutil.Fake(s.T(), faker, database.Chat{})
|
||||
|
||||
@@ -104,11 +104,11 @@ func (m queryMetricsStore) DeleteOrganization(ctx context.Context, id uuid.UUID)
|
||||
return r0
|
||||
}
|
||||
|
||||
func (m queryMetricsStore) AcquireChat(ctx context.Context, arg database.AcquireChatParams) (database.Chat, error) {
|
||||
func (m queryMetricsStore) AcquireChats(ctx context.Context, arg database.AcquireChatsParams) ([]database.Chat, error) {
|
||||
start := time.Now()
|
||||
r0, r1 := m.s.AcquireChat(ctx, arg)
|
||||
m.queryLatencies.WithLabelValues("AcquireChat").Observe(time.Since(start).Seconds())
|
||||
m.queryCounts.WithLabelValues(httpmw.ExtractHTTPRoute(ctx), httpmw.ExtractHTTPMethod(ctx), "AcquireChat").Inc()
|
||||
r0, r1 := m.s.AcquireChats(ctx, arg)
|
||||
m.queryLatencies.WithLabelValues("AcquireChats").Observe(time.Since(start).Seconds())
|
||||
m.queryCounts.WithLabelValues(httpmw.ExtractHTTPRoute(ctx), httpmw.ExtractHTTPMethod(ctx), "AcquireChats").Inc()
|
||||
return r0, r1
|
||||
}
|
||||
|
||||
|
||||
@@ -44,19 +44,19 @@ func (m *MockStore) EXPECT() *MockStoreMockRecorder {
|
||||
return m.recorder
|
||||
}
|
||||
|
||||
// AcquireChat mocks base method.
|
||||
func (m *MockStore) AcquireChat(ctx context.Context, arg database.AcquireChatParams) (database.Chat, error) {
|
||||
// AcquireChats mocks base method.
|
||||
func (m *MockStore) AcquireChats(ctx context.Context, arg database.AcquireChatsParams) ([]database.Chat, error) {
|
||||
m.ctrl.T.Helper()
|
||||
ret := m.ctrl.Call(m, "AcquireChat", ctx, arg)
|
||||
ret0, _ := ret[0].(database.Chat)
|
||||
ret := m.ctrl.Call(m, "AcquireChats", ctx, arg)
|
||||
ret0, _ := ret[0].([]database.Chat)
|
||||
ret1, _ := ret[1].(error)
|
||||
return ret0, ret1
|
||||
}
|
||||
|
||||
// AcquireChat indicates an expected call of AcquireChat.
|
||||
func (mr *MockStoreMockRecorder) AcquireChat(ctx, arg any) *gomock.Call {
|
||||
// AcquireChats indicates an expected call of AcquireChats.
|
||||
func (mr *MockStoreMockRecorder) AcquireChats(ctx, arg any) *gomock.Call {
|
||||
mr.mock.ctrl.T.Helper()
|
||||
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AcquireChat", reflect.TypeOf((*MockStore)(nil).AcquireChat), ctx, arg)
|
||||
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AcquireChats", reflect.TypeOf((*MockStore)(nil).AcquireChats), ctx, arg)
|
||||
}
|
||||
|
||||
// AcquireLock mocks base method.
|
||||
|
||||
@@ -12,9 +12,9 @@ import (
|
||||
)
|
||||
|
||||
type sqlcQuerier interface {
|
||||
// Acquires a pending chat for processing. Uses SKIP LOCKED to prevent
|
||||
// multiple replicas from acquiring the same chat.
|
||||
AcquireChat(ctx context.Context, arg AcquireChatParams) (Chat, error)
|
||||
// Acquires up to @num_chats pending chats for processing. Uses SKIP LOCKED
|
||||
// to prevent multiple replicas from acquiring the same chat.
|
||||
AcquireChats(ctx context.Context, arg AcquireChatsParams) ([]Chat, error)
|
||||
// Blocks until the lock is acquired.
|
||||
//
|
||||
// This must be called from within a transaction. The lock will be automatically
|
||||
|
||||
@@ -2968,7 +2968,7 @@ func (q *sqlQuerier) UpdateChatProvider(ctx context.Context, arg UpdateChatProvi
|
||||
return i, err
|
||||
}
|
||||
|
||||
const acquireChat = `-- name: AcquireChat :one
|
||||
const acquireChats = `-- name: AcquireChats :many
|
||||
UPDATE
|
||||
chats
|
||||
SET
|
||||
@@ -2978,7 +2978,7 @@ SET
|
||||
updated_at = $1::timestamptz,
|
||||
worker_id = $2::uuid
|
||||
WHERE
|
||||
id = (
|
||||
id = ANY(
|
||||
SELECT
|
||||
id
|
||||
FROM
|
||||
@@ -2990,40 +2990,57 @@ WHERE
|
||||
FOR UPDATE
|
||||
SKIP LOCKED
|
||||
LIMIT
|
||||
1
|
||||
$3::int
|
||||
)
|
||||
RETURNING
|
||||
id, owner_id, workspace_id, title, status, worker_id, started_at, heartbeat_at, created_at, updated_at, parent_chat_id, root_chat_id, last_model_config_id, archived, last_error
|
||||
`
|
||||
|
||||
type AcquireChatParams struct {
|
||||
type AcquireChatsParams struct {
|
||||
StartedAt time.Time `db:"started_at" json:"started_at"`
|
||||
WorkerID uuid.UUID `db:"worker_id" json:"worker_id"`
|
||||
NumChats int32 `db:"num_chats" json:"num_chats"`
|
||||
}
|
||||
|
||||
// Acquires a pending chat for processing. Uses SKIP LOCKED to prevent
|
||||
// multiple replicas from acquiring the same chat.
|
||||
func (q *sqlQuerier) AcquireChat(ctx context.Context, arg AcquireChatParams) (Chat, error) {
|
||||
row := q.db.QueryRowContext(ctx, acquireChat, arg.StartedAt, arg.WorkerID)
|
||||
var i Chat
|
||||
err := row.Scan(
|
||||
&i.ID,
|
||||
&i.OwnerID,
|
||||
&i.WorkspaceID,
|
||||
&i.Title,
|
||||
&i.Status,
|
||||
&i.WorkerID,
|
||||
&i.StartedAt,
|
||||
&i.HeartbeatAt,
|
||||
&i.CreatedAt,
|
||||
&i.UpdatedAt,
|
||||
&i.ParentChatID,
|
||||
&i.RootChatID,
|
||||
&i.LastModelConfigID,
|
||||
&i.Archived,
|
||||
&i.LastError,
|
||||
)
|
||||
return i, err
|
||||
// Acquires up to @num_chats pending chats for processing. Uses SKIP LOCKED
|
||||
// to prevent multiple replicas from acquiring the same chat.
|
||||
func (q *sqlQuerier) AcquireChats(ctx context.Context, arg AcquireChatsParams) ([]Chat, error) {
|
||||
rows, err := q.db.QueryContext(ctx, acquireChats, arg.StartedAt, arg.WorkerID, arg.NumChats)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
defer rows.Close()
|
||||
var items []Chat
|
||||
for rows.Next() {
|
||||
var i Chat
|
||||
if err := rows.Scan(
|
||||
&i.ID,
|
||||
&i.OwnerID,
|
||||
&i.WorkspaceID,
|
||||
&i.Title,
|
||||
&i.Status,
|
||||
&i.WorkerID,
|
||||
&i.StartedAt,
|
||||
&i.HeartbeatAt,
|
||||
&i.CreatedAt,
|
||||
&i.UpdatedAt,
|
||||
&i.ParentChatID,
|
||||
&i.RootChatID,
|
||||
&i.LastModelConfigID,
|
||||
&i.Archived,
|
||||
&i.LastError,
|
||||
); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
items = append(items, i)
|
||||
}
|
||||
if err := rows.Close(); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
if err := rows.Err(); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
return items, nil
|
||||
}
|
||||
|
||||
const acquireStaleChatDiffStatuses = `-- name: AcquireStaleChatDiffStatuses :many
|
||||
|
||||
@@ -257,9 +257,9 @@ WHERE
|
||||
RETURNING
|
||||
*;
|
||||
|
||||
-- name: AcquireChat :one
|
||||
-- Acquires a pending chat for processing. Uses SKIP LOCKED to prevent
|
||||
-- multiple replicas from acquiring the same chat.
|
||||
-- name: AcquireChats :many
|
||||
-- Acquires up to @num_chats pending chats for processing. Uses SKIP LOCKED
|
||||
-- to prevent multiple replicas from acquiring the same chat.
|
||||
UPDATE
|
||||
chats
|
||||
SET
|
||||
@@ -269,7 +269,7 @@ SET
|
||||
updated_at = @started_at::timestamptz,
|
||||
worker_id = @worker_id::uuid
|
||||
WHERE
|
||||
id = (
|
||||
id = ANY(
|
||||
SELECT
|
||||
id
|
||||
FROM
|
||||
@@ -281,7 +281,7 @@ WHERE
|
||||
FOR UPDATE
|
||||
SKIP LOCKED
|
||||
LIMIT
|
||||
1
|
||||
@num_chats::int
|
||||
)
|
||||
RETURNING
|
||||
*;
|
||||
|
||||
Reference in New Issue
Block a user