-
Notifications
You must be signed in to change notification settings - Fork 491
perf: incremental memory optimization across Maka #5153
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
Changes from all commits
9be32ca
e9289a6
c864564
d0f8b3e
78f521d
e5885fc
46c8211
34f4ea8
c66cffe
8908edd
6eea5ba
f3fbce2
0fa4626
d02caf9
31a5a44
b634df8
e0dd3c8
0e7aa4f
e055012
cd36563
fcd7e0a
c809a7e
cb1ae62
ae5d246
4888ea8
719b646
f4c1a3a
5af4f26
fdc1c82
878520a
98e1944
1789afe
9cbd018
40866de
2c282d2
3616b27
c45d118
3f6613e
1271fe4
6c0eb68
0fa074d
be86089
dc8b79a
00bbf5d
0dfa876
848eaa8
7d9bdaf
3f5b3eb
09bad37
bdd4fc6
f949ea7
0daa01c
970fb26
f7b3b2b
0ccd888
233e212
f2585c7
27565ac
81ca6b2
67b04c4
e44d8a0
403ec16
d6f2dcf
9686f2b
0d72fe7
2b497ec
5bccbba
063d915
24c77a0
5cb9da5
77aafae
c9635cd
44bc559
b64ec34
8f70f8f
8429829
ab5266c
4aa9799
e272198
4bbe8fd
b2c6333
2bb0095
13f27da
48c5189
6c62438
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -136,11 +136,12 @@ const bySession = new Map<string, Connection>(); | |
| // so two concurrent first calls for one conversation must share one attempt | ||
| // instead of racing into a second connection (which the bridge would reject). | ||
| const pendingAcquires = new Map<string, Promise<Connection>>(); | ||
| // Release epoch per conversation. A delete/archive cannot reliably see an | ||
| // Release epoch per in-flight acquire. A delete/archive cannot reliably see an | ||
| // in-flight acquire, so instead of the release waiting on the acquire, the | ||
| // acquire notices the bump after connecting and unwinds itself — otherwise its | ||
| // resolveEndpoint would resurrect the just-disposed view and the connection | ||
| // would outlive the conversation with nothing left to ever clean it up. | ||
| // would outlive the conversation with nothing left to ever clean it up. The | ||
| // entry only lives until the acquire settles, not for every released session. | ||
| const releaseEpochs = new Map<string, number>(); | ||
| // In-flight actions per conversation, so the visible lease can REVOKE — not just | ||
| // preflight. canDrive gates the START on screen; this severs an action that was | ||
|
|
@@ -244,8 +245,11 @@ async function acquire(sessionId: string): Promise<Connection> { | |
| // call retries fresh; concurrent callers share the same outcome either way. | ||
| const inflight = pendingAcquires.get(sessionId); | ||
| if (inflight) return inflight; | ||
| const epoch = 0; | ||
| // Register before resolveEndpoint, which may synchronously release the session | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3 — "Register before resolveEndpoint, which may synchronously release the session before this attempt can be registered in pendingAcquires" — the old code handled that too: The change itself is fine — there is no await between 中文"Register before resolveEndpoint, which may synchronously release the session before this attempt can be registered in pendingAcquires"——旧代码同样处理得了: 改动本身没问题——
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. EnglishKeeping this comment. Agreed that the old implementation also handled synchronous release, and that the optimization removes historical map entries. The comment is not claiming an old correctness bug: it records an ordering requirement of the new pending-acquire-only representation. Because release only advances a registered in-flight epoch now, registration must happen before a reentrant resolveEndpoint call, not after it. Describing that required order remains useful even though both implementations are correct. No functional change requested here is needed. Automated follow-up by OpenAI Codex. 中文保留这条注释。同意旧实现也能处理同步 release,也同意优化目的是移除历史 map 项。但注释没有声称旧实现存在正确性缺陷,而是在记录新“仅待决 acquire 持有状态”表示的顺序要求。 现在 release 只推进已注册的在飞 epoch,因此必须在可能重入的 resolveEndpoint 调用之前注册,不能放到之后。两版都正确,不影响该顺序说明仍有价值;这里无需功能修改。 OpenAI Codex 自动跟进回复。 |
||
| // before this attempt can be registered in pendingAcquires. | ||
| releaseEpochs.set(sessionId, epoch); | ||
| const promise = (async () => { | ||
| const epoch = releaseEpochs.get(sessionId); | ||
| const endpoint = await browserViewHost().resolveEndpoint(sessionId); | ||
| let conn: Connection; | ||
| try { | ||
|
|
@@ -272,7 +276,10 @@ async function acquire(sessionId: string): Promise<Connection> { | |
| } | ||
| bySession.set(sessionId, conn); | ||
| return conn; | ||
| })().finally(() => pendingAcquires.delete(sessionId)); | ||
| })().finally(() => { | ||
| pendingAcquires.delete(sessionId); | ||
| releaseEpochs.delete(sessionId); | ||
| }); | ||
| pendingAcquires.set(sessionId, promise); | ||
| return promise; | ||
| } | ||
|
|
@@ -412,7 +419,8 @@ export async function releaseBrowserSession(sessionId: string): Promise<void> { | |
| // when it sees the new epoch (see acquire) — it cannot be awaited here because | ||
| // it may not have registered in pendingAcquires yet, and a hung endpoint | ||
| // resolution must not block the session's deletion. | ||
| releaseEpochs.set(sessionId, (releaseEpochs.get(sessionId) ?? 0) + 1); | ||
| const epoch = releaseEpochs.get(sessionId); | ||
| if (epoch !== undefined) releaseEpochs.set(sessionId, epoch + 1); | ||
| const conn = bySession.get(sessionId); | ||
| if (conn) { | ||
| bySession.delete(sessionId); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P3 —
apps/desktop/src/main/__tests__/workhub-presentation.test.ts:555The rename to
creates on first shortcuthas nothing asserted betweenrefreshSettings()and the firsth.shortcut(). Item 01's only behavioural claim — enabling alone must not create the renderer — has no regression cover; re-adding prewarm leaves this test green.assert.equal(h.views.length, 0, 'enabling alone must not create the renderer'). Same shape as the assertions at :492 and :508.中文
改名成
creates on first shortcut,但refreshSettings()和第一次h.shortcut()之间什么都没断言。第 01 项唯一的行为主张——启用本身不创建渲染器——没有回归保护,把 prewarm 加回来这个测试照样绿。加
assert.equal(h.views.length, 0, 'enabling alone must not create the renderer'),和 :492、:508 的断言同形状。There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
English
Accepted in e2721989f. The existing test now asserts views.length === 0 immediately after refreshSettings and before the first shortcut. It also checks that cold loading does not show/focus the window until ready, while subsequent warm toggles remain synchronous. All 24 presentation tests pass.
Automated follow-up by OpenAI Codex.
中文
接受,已在 e2721989f 的现有测试中,在 refreshSettings 后、第一次快捷键前断言 views.length === 0。还检查冷加载期间不显示/聚焦窗口、ready 后展示,以及后续热启动切换仍同步完成。24 项展示控制测试通过。
OpenAI Codex 自动跟进回复。