Eliminate UI stuttering caused by main thread blocking network calls #579
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…alls Fixes GitHub issue CoplayDev#577: 28ms UI stutters every ~1 second when MCP plugin is running ## Root Cause The `MCPForUnityEditorWindow.OnEditorUpdate()` hook was being called every frame (60+ times/sec) to poll connection status. This triggered `UpdateConnectionStatus()` → `IsLocalHttpServerReachable()`, which makes network socket connection attempts with 50ms timeouts on the main thread. ## Architectural Changes - Remove `OnEditorUpdate` hook that polled every frame - Replace continuous polling with event-driven connection status updates: - User actions (manual refreshes via `RefreshAllData`) - Bridge state changes (via `RequestHealthVerification`) - Window focus events (via `OnFocus`) Connection status now only updates when actually needed. ## Performance Improvements 1. **Eliminated main thread blocking network I/O** - Network socket checks no longer execute 60+ times per second - Main thread stays responsive for gameplay 2. **TransportCommandDispatcher: Early exit optimization** - Early `Pending.Count` check prevents List allocations when idle - Before: ~90 allocations/sec → 28ms GC spike every ~1 second - After: 0 allocations/sec when idle → no GC stuttering 3. **EditorStateCache: Sequence-based caching** - Only increments sequence number when state actually changes - Prevents unnecessary `DeepClone()` calls on every `GetSnapshot()` 4. **StdioBridgeHost: Consistent early exit pattern** - Added same early `commandQueue.Count` check as TransportCommandDispatcher ## Testing - Verified MCP plugin runs without stuttering - Scene hierarchy queries work correctly - Editor menu items function properly - Smooth gameplay without frame drops Co-Authored-By: Claude Haiku 4.5 <[email protected]>
Contributor
Reviewer's GuideReworks editor networking and command dispatch to eliminate per-frame main-thread network checks and idle allocations, adds early-exit patterns for queues, and optimizes editor state caching to avoid unnecessary cloning, plus regression tests to guard against GC spikes and dispatcher regressions. Sequence diagram for event-driven main thread command dispatchsequenceDiagram
actor BackgroundTask
participant TransportCommandDispatcher
participant SynchronizationContext
participant UnityEditorLoop
BackgroundTask->>TransportCommandDispatcher: EnqueuePendingCommand
activate TransportCommandDispatcher
TransportCommandDispatcher->>TransportCommandDispatcher: RequestMainThreadPump
TransportCommandDispatcher->>SynchronizationContext: Post(ProcessQueue)
TransportCommandDispatcher->>UnityEditorLoop: QueuePlayerLoopUpdate
deactivate TransportCommandDispatcher
UnityEditorLoop->>TransportCommandDispatcher: EditorApplication.update -> ProcessQueue
activate TransportCommandDispatcher
TransportCommandDispatcher->>TransportCommandDispatcher: lock PendingLock
alt Pending.Count == 0
TransportCommandDispatcher->>TransportCommandDispatcher: UnhookUpdateIfIdle
TransportCommandDispatcher-->>UnityEditorLoop: return
else Pending.Count > 0
TransportCommandDispatcher->>TransportCommandDispatcher: ready = new List(Pending.Count)
TransportCommandDispatcher->>TransportCommandDispatcher: Move commands to ready
TransportCommandDispatcher-->>UnityEditorLoop: Process ready commands
TransportCommandDispatcher->>TransportCommandDispatcher: UnhookUpdateIfIdle (if now idle)
end
deactivate TransportCommandDispatcher
Sequence diagram for event-driven connection status updates in MCP editor windowsequenceDiagram
actor User
participant MCPForUnityEditorWindow
participant ConnectionSection
participant Bridge
User->>MCPForUnityEditorWindow: Open window
activate MCPForUnityEditorWindow
MCPForUnityEditorWindow->>MCPForUnityEditorWindow: OnEnable
MCPForUnityEditorWindow->>MCPForUnityEditorWindow: Do not hook OnEditorUpdate
User->>MCPForUnityEditorWindow: Focus window
MCPForUnityEditorWindow->>MCPForUnityEditorWindow: OnFocus
MCPForUnityEditorWindow->>MCPForUnityEditorWindow: RefreshAllData
MCPForUnityEditorWindow->>ConnectionSection: UpdateConnectionStatus
Bridge-->>MCPForUnityEditorWindow: RequestHealthVerification / state change
activate Bridge
MCPForUnityEditorWindow->>ConnectionSection: UpdateConnectionStatus
deactivate Bridge
User->>MCPForUnityEditorWindow: Manual refresh action
MCPForUnityEditorWindow->>MCPForUnityEditorWindow: RefreshAllData
MCPForUnityEditorWindow->>ConnectionSection: UpdateConnectionStatus
deactivate MCPForUnityEditorWindow
Updated class diagram for editor networking, dispatch, and state cacheclassDiagram
class EditorStateCache {
<<static>>
-double MinUpdateIntervalSeconds
-JObject _cached
-JObject _cachedClone
-long _cachedSequence
-string _lastTrackedScenePath
-bool _isUpdating
-bool? _lastTrackedIsFocused
-bool? _lastTrackedIsPlaying
-bool? _lastTrackedIsUpdating
-string _lastTrackedActivityPhase
-long _sequence
-long _observedUnixMs
+JObject GetSnapshot()
-JObject BuildSnapshot(string reason)
-void ForceUpdate(string reason)
}
class EditorStateSnapshot {
+string SchemaVersion
+long Sequence
+long ObservedUnixMs
+string ScenePath
+bool IsFocused
+bool IsPlaying
+bool IsUpdating
+string ActivityPhase
+string TestsMode
+string CurrentJobId
}
class TransportCommandDispatcher {
<<static>>
-Dictionary~string, PendingCommand~ Pending
-object PendingLock
-bool updateHooked
+void EnqueuePendingCommand(string id, PendingCommand command)
+void RequestMainThreadPump()
-void HookUpdate()
-void UnhookUpdateIfIdle()
-void ProcessQueue()
}
class PendingCommand {
+string Id
+object Payload
+void Execute()
}
class StdioBridgeHost {
<<static>>
-Dictionary~string, QueuedCommand~ commandQueue
-object lockObj
-void ProcessCommands()
}
class QueuedCommand {
+string Id
+object Payload
+void Execute()
}
class MCPForUnityEditorWindow {
-bool guiCreated
-bool toolsLoaded
-VisualElement rootVisualElement
-ConnectionSection connectionSection
+void OnEnable()
+void OnDisable()
+void OnFocus()
-void RefreshAllData()
-void EnsureToolsLoaded()
}
class ConnectionSection {
+void UpdateConnectionStatus()
}
EditorStateCache "1" *-- "1" EditorStateSnapshot
TransportCommandDispatcher "1" o-- "*" PendingCommand
StdioBridgeHost "1" o-- "*" QueuedCommand
MCPForUnityEditorWindow --> ConnectionSection
StdioBridgeHost --> TransportCommandDispatcher
MCPForUnityEditorWindow --> EditorStateCache
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
📝 WalkthroughWalkthroughThis PR optimizes MCPForUnity editor services by eliminating continuous polling and reducing garbage collection overhead. Changes include conditional editor update hook management, state caching with change tracking, early-return optimizations in queue processing, and comprehensive performance test coverage to validate reduced allocations during idle states. Changes
Sequence Diagram(s)sequenceDiagram
participant EA as EditorApplication
participant TCD as TransportCommandDispatcher
participant Q as Command Queue
rect rgba(100, 149, 237, 0.5)
Note over TCD,Q: Idle State - Hook Removed
EA->>TCD: Frame update (no pending commands)
TCD->>Q: ProcessQueue()
alt Queue is empty
TCD->>TCD: UnhookUpdateIfIdle()
TCD->>EA: Unsubscribe from update
end
end
rect rgba(144, 238, 144, 0.5)
Note over TCD,Q: Command Arrives - Hook Re-added
Q->>TCD: ExecuteCommandJsonAsync(cmd)
TCD->>Q: QueueCommand()
TCD->>TCD: RequestMainThreadPump()
TCD->>EA: Subscribe to update
end
rect rgba(255, 165, 0, 0.5)
Note over TCD,Q: Processing - Hook Active
EA->>TCD: Frame update (commands pending)
TCD->>Q: ProcessQueue()
TCD->>Q: Dequeue and execute
Q-->>TCD: Command result
alt All commands processed
TCD->>TCD: UnhookUpdateIfIdle()
TCD->>EA: Unsubscribe from update
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 5 issues, and left some high level feedback:
- The new
_cachedClonereuse inEditorStateCache.GetSnapshotassumes callers never mutate the returnedJObject; if any consumer currently modifies the snapshot, this change alters behavior (shared state, cross-call contamination) and may require either maintaining per-call cloning or enforcing immutability at the boundaries. - The new performance tests that assert on
GC.GetTotalMemorydeltas and memory growth thresholds are likely to be environment- and runtime-dependent; consider loosening these assertions or restructuring them to avoid hard numeric limits that can cause intermittent CI flakiness.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `_cachedClone` reuse in `EditorStateCache.GetSnapshot` assumes callers never mutate the returned `JObject`; if any consumer currently modifies the snapshot, this change alters behavior (shared state, cross-call contamination) and may require either maintaining per-call cloning or enforcing immutability at the boundaries.
- The new performance tests that assert on `GC.GetTotalMemory` deltas and memory growth thresholds are likely to be environment- and runtime-dependent; consider loosening these assertions or restructuring them to avoid hard numeric limits that can cause intermittent CI flakiness.
## Individual Comments
### Comment 1
<location> `MCPForUnity/Editor/Services/EditorStateCache.cs:346-355` </location>
<code_context>
+ // Only increment sequence if something actually changed.
+ // This prevents unnecessary DeepClone() calls in GetSnapshot() when state is idle.
+ bool hasChanges = _lastTrackedScenePath != scenePath
+ || _lastTrackedIsFocused != isFocused
+ || _lastTrackedIsPlaying != isPlaying
+ || _lastTrackedIsUpdating != isUpdating
+ || _lastTrackedActivityPhase != activityPhase;
+
+ if (hasChanges)
+ {
+ _sequence++;
+ }
+
+ // Update tracked state
+ _lastTrackedScenePath = scenePath;
+ _lastTrackedIsFocused = isFocused;
+ _lastTrackedIsPlaying = isPlaying;
+ _lastTrackedIsUpdating = isUpdating;
+ _lastTrackedActivityPhase = activityPhase;
+
var snapshot = new EditorStateSnapshot
</code_context>
<issue_to_address>
**issue (bug_risk):** Sequence increment is now decoupled from actual snapshot changes, which can cause stale snapshots to be served.
`_sequence` now only changes when the tracked fields change, but `BuildSnapshot` includes additional fields (e.g., compile state, test status, job IDs, timestamps). Those can change without updating `_sequence`, so `_cachedClone` may not be regenerated and callers can get a stale snapshot that doesn’t match `_cached` or the editor state.
If `_sequence` is meant to represent “snapshot content changed”, it should either be incremented on every `BuildSnapshot` call, or `hasChanges` should be expanded to include all snapshot fields that affect consumers. Otherwise `_cached` and `_cachedClone` can diverge in ways that are likely to break callers expecting current state.
</issue_to_address>
### Comment 2
<location> `MCPForUnity/Editor/Services/EditorStateCache.cs:467` </location>
<code_context>
+ _cachedSequence = _sequence;
+ }
+
+ return _cachedClone;
}
}
</code_context>
<issue_to_address>
**issue (bug_risk):** Returning the shared cached clone allows external mutation of the cache state.
Previously `GetSnapshot()` always returned a fresh `DeepClone()`, so callers effectively saw an immutable snapshot. Reusing a single `_cachedClone` means any mutation of the returned `JObject` (intentional or accidental) will corrupt the shared cache for all callers and may desynchronize it from `_cached` and the editor state.
Unless we can guarantee all consumers treat the snapshot as read-only, this optimization is risky. A safer approach is to keep `_cached` as the authoritative state and still `DeepClone()` on each `GetSnapshot()`, only avoiding rebuilding `_cached` when `_sequence` hasn’t changed. If you keep the shared clone, please explicitly document/enforce an immutable-view contract for `GetSnapshot()` and audit current usages.
</issue_to_address>
### Comment 3
<location> `MCPForUnity/Editor/Services/EditorStateCache.cs:289-292` </location>
<code_context>
private static JObject BuildSnapshot(string reason)
{
- _sequence++;
_observedUnixMs = DateTimeOffset.UtcNow.ToUnixTimeMilliseconds();
</code_context>
<issue_to_address>
**question (bug_risk):** Timestamps and other ephemeral fields will now remain constant while state is idle, which may break time-based consumers.
Because `_sequence` no longer increments on every snapshot, timestamps like `_observedUnixMs` (and any derived values) will now stay frozen while the logical state is idle. Any consumer relying on these timestamps for heartbeats, timeouts/latency, or "last seen" diagnostics will observe a static time instead of a continuously advancing one. If those use cases matter, consider either a separate monotonic heartbeat field or forcing `_sequence` to advance (and the snapshot to update) after some time threshold, even when the tracked state is unchanged.
</issue_to_address>
### Comment 4
<location> `MCPForUnity/Editor/Services/Transport/TransportCommandDispatcher.cs:210-219` </location>
<code_context>
private static void UnhookUpdateIfIdle()
{
- // Intentionally no-op: keep update hook installed so background commands always process.
- // This avoids "must focus Unity to re-establish contact" edge cases.
- return;
+ // Only unhook if there are no pending commands. RequestMainThreadPump() guarantees
+ // re-hooking via SynchronizationContext.Post + QueuePlayerLoopUpdate when commands arrive.
+ if (Pending.Count == 0 && updateHooked)
+ {
+ updateHooked = false;
</code_context>
<issue_to_address>
**suggestion (bug_risk):** UnhookUpdateIfIdle reads Pending.Count without locking, which may race if called from multiple code paths.
Within `ProcessQueue`, `UnhookUpdateIfIdle()` is called while holding `PendingLock`, so `Pending.Count` is safe there. The risk is future or alternate call sites that invoke `UnhookUpdateIfIdle()` without `PendingLock`, which would make the unsynchronized `Pending.Count` read race with writers and potentially throw `InvalidOperationException`.
If this method must always be called under `PendingLock`, consider either documenting that explicitly (e.g., name/comment) or taking `PendingLock` inside `UnhookUpdateIfIdle()` before accessing `Pending` to prevent incorrect future usage.
```suggestion
private static void UnhookUpdateIfIdle()
{
// Only unhook if there are no pending commands. RequestMainThreadPump() guarantees
// re-hooking via SynchronizationContext.Post + QueuePlayerLoopUpdate when commands arrive.
// Must hold PendingLock while inspecting Pending to avoid races with writers.
lock (PendingLock)
{
if (Pending.Count == 0 && updateHooked)
{
updateHooked = false;
EditorApplication.update -= ProcessQueue;
}
}
}
```
</issue_to_address>
### Comment 5
<location> `TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/TransportCommandDispatcherPerformanceTests.cs:169-193` </location>
<code_context>
+
+ // Helper: Simulates EditorApplication.update callback
+ // Uses reflection to invoke ProcessQueue on the TransportCommandDispatcher
+ private void SimulateEditorUpdate()
+ {
+ try
+ {
+ var method = typeof(TransportCommandDispatcher)
+ .GetMethod("ProcessQueue",
+ System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Static);
+
+ if (method != null)
+ {
+ method.Invoke(null, null);
+ }
+ }
+ catch (System.Reflection.TargetInvocationException ex)
+ {
+ // Unwrap the inner exception from reflection
+ if (ex.InnerException != null)
+ {
+ throw ex.InnerException;
+ }
+ throw;
+ }
+ }
</code_context>
<issue_to_address>
**suggestion (testing):** Reflection-based invocation of ProcessQueue is brittle and could be made safer/clearer
This reflection on a private `ProcessQueue` makes the tests fragile: any rename, signature, or visibility change will cause runtime failures rather than clear compile-time errors. If you must use this pattern, consider centralizing the reflection lookup in a cached static field with an explicit assertion when the method isn’t found, or exposing an internal test hook (e.g., via `InternalsVisibleTo`) so tests can call into the dispatcher without private reflection.
```suggestion
// Helper: Simulates EditorApplication.update callback.
// Uses a cached reflection lookup to invoke ProcessQueue on the TransportCommandDispatcher.
// This centralizes the private reflection and fails fast with a clear assertion if the
// target method signature or visibility changes.
private static readonly System.Reflection.MethodInfo s_processQueueMethod =
typeof(TransportCommandDispatcher).GetMethod(
"ProcessQueue",
System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Static);
private void SimulateEditorUpdate()
{
// Fail fast with a clear message if the method can no longer be located.
NUnit.Framework.Assert.That(
s_processQueueMethod,
NUnit.Framework.Is.Not.Null,
"TransportCommandDispatcher.ProcessQueue signature or visibility has changed. " +
"Update the test's reflection hook accordingly.");
try
{
s_processQueueMethod!.Invoke(null, null);
}
catch (System.Reflection.TargetInvocationException ex)
{
// Unwrap the inner exception from reflection for clearer test failures.
if (ex.InnerException != null)
{
throw ex.InnerException;
}
throw;
}
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+346
to
+355
| bool hasChanges = _lastTrackedScenePath != scenePath | ||
| || _lastTrackedIsFocused != isFocused | ||
| || _lastTrackedIsPlaying != isPlaying | ||
| || _lastTrackedIsUpdating != isUpdating | ||
| || _lastTrackedActivityPhase != activityPhase; | ||
|
|
||
| if (hasChanges) | ||
| { | ||
| _sequence++; | ||
| } |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Sequence increment is now decoupled from actual snapshot changes, which can cause stale snapshots to be served.
_sequence now only changes when the tracked fields change, but BuildSnapshot includes additional fields (e.g., compile state, test status, job IDs, timestamps). Those can change without updating _sequence, so _cachedClone may not be regenerated and callers can get a stale snapshot that doesn’t match _cached or the editor state.
If _sequence is meant to represent “snapshot content changed”, it should either be incremented on every BuildSnapshot call, or hasChanges should be expanded to include all snapshot fields that affect consumers. Otherwise _cached and _cachedClone can diverge in ways that are likely to break callers expecting current state.
| _cachedSequence = _sequence; | ||
| } | ||
|
|
||
| return _cachedClone; |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Returning the shared cached clone allows external mutation of the cache state.
Previously GetSnapshot() always returned a fresh DeepClone(), so callers effectively saw an immutable snapshot. Reusing a single _cachedClone means any mutation of the returned JObject (intentional or accidental) will corrupt the shared cache for all callers and may desynchronize it from _cached and the editor state.
Unless we can guarantee all consumers treat the snapshot as read-only, this optimization is risky. A safer approach is to keep _cached as the authoritative state and still DeepClone() on each GetSnapshot(), only avoiding rebuilding _cached when _sequence hasn’t changed. If you keep the shared clone, please explicitly document/enforce an immutable-view contract for GetSnapshot() and audit current usages.
Comment on lines
-289
to
292
| _sequence++; | ||
| _observedUnixMs = DateTimeOffset.UtcNow.ToUnixTimeMilliseconds(); | ||
|
|
||
| bool isCompiling = EditorApplication.isCompiling; |
Contributor
There was a problem hiding this comment.
question (bug_risk): Timestamps and other ephemeral fields will now remain constant while state is idle, which may break time-based consumers.
Because _sequence no longer increments on every snapshot, timestamps like _observedUnixMs (and any derived values) will now stay frozen while the logical state is idle. Any consumer relying on these timestamps for heartbeats, timeouts/latency, or "last seen" diagnostics will observe a static time instead of a continuously advancing one. If those use cases matter, consider either a separate monotonic heartbeat field or forcing _sequence to advance (and the snapshot to update) after some time threshold, even when the tracked state is unchanged.
Comment on lines
210
to
219
| private static void UnhookUpdateIfIdle() | ||
| { | ||
| // Intentionally no-op: keep update hook installed so background commands always process. | ||
| // This avoids "must focus Unity to re-establish contact" edge cases. | ||
| return; | ||
| // Only unhook if there are no pending commands. RequestMainThreadPump() guarantees | ||
| // re-hooking via SynchronizationContext.Post + QueuePlayerLoopUpdate when commands arrive. | ||
| if (Pending.Count == 0 && updateHooked) | ||
| { | ||
| updateHooked = false; | ||
| EditorApplication.update -= ProcessQueue; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): UnhookUpdateIfIdle reads Pending.Count without locking, which may race if called from multiple code paths.
Within ProcessQueue, UnhookUpdateIfIdle() is called while holding PendingLock, so Pending.Count is safe there. The risk is future or alternate call sites that invoke UnhookUpdateIfIdle() without PendingLock, which would make the unsynchronized Pending.Count read race with writers and potentially throw InvalidOperationException.
If this method must always be called under PendingLock, consider either documenting that explicitly (e.g., name/comment) or taking PendingLock inside UnhookUpdateIfIdle() before accessing Pending to prevent incorrect future usage.
Suggested change
| private static void UnhookUpdateIfIdle() | |
| { | |
| // Intentionally no-op: keep update hook installed so background commands always process. | |
| // This avoids "must focus Unity to re-establish contact" edge cases. | |
| return; | |
| // Only unhook if there are no pending commands. RequestMainThreadPump() guarantees | |
| // re-hooking via SynchronizationContext.Post + QueuePlayerLoopUpdate when commands arrive. | |
| if (Pending.Count == 0 && updateHooked) | |
| { | |
| updateHooked = false; | |
| EditorApplication.update -= ProcessQueue; | |
| } | |
| } | |
| private static void UnhookUpdateIfIdle() | |
| { | |
| // Only unhook if there are no pending commands. RequestMainThreadPump() guarantees | |
| // re-hooking via SynchronizationContext.Post + QueuePlayerLoopUpdate when commands arrive. | |
| // Must hold PendingLock while inspecting Pending to avoid races with writers. | |
| lock (PendingLock) | |
| { | |
| if (Pending.Count == 0 && updateHooked) | |
| { | |
| updateHooked = false; | |
| EditorApplication.update -= ProcessQueue; | |
| } | |
| } | |
| } |
Comment on lines
+169
to
+193
| // Helper: Simulates EditorApplication.update callback | ||
| // Uses reflection to invoke ProcessQueue on the TransportCommandDispatcher | ||
| private void SimulateEditorUpdate() | ||
| { | ||
| try | ||
| { | ||
| var method = typeof(TransportCommandDispatcher) | ||
| .GetMethod("ProcessQueue", | ||
| System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Static); | ||
|
|
||
| if (method != null) | ||
| { | ||
| method.Invoke(null, null); | ||
| } | ||
| } | ||
| catch (System.Reflection.TargetInvocationException ex) | ||
| { | ||
| // Unwrap the inner exception from reflection | ||
| if (ex.InnerException != null) | ||
| { | ||
| throw ex.InnerException; | ||
| } | ||
| throw; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
suggestion (testing): Reflection-based invocation of ProcessQueue is brittle and could be made safer/clearer
This reflection on a private ProcessQueue makes the tests fragile: any rename, signature, or visibility change will cause runtime failures rather than clear compile-time errors. If you must use this pattern, consider centralizing the reflection lookup in a cached static field with an explicit assertion when the method isn’t found, or exposing an internal test hook (e.g., via InternalsVisibleTo) so tests can call into the dispatcher without private reflection.
Suggested change
| // Helper: Simulates EditorApplication.update callback | |
| // Uses reflection to invoke ProcessQueue on the TransportCommandDispatcher | |
| private void SimulateEditorUpdate() | |
| { | |
| try | |
| { | |
| var method = typeof(TransportCommandDispatcher) | |
| .GetMethod("ProcessQueue", | |
| System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Static); | |
| if (method != null) | |
| { | |
| method.Invoke(null, null); | |
| } | |
| } | |
| catch (System.Reflection.TargetInvocationException ex) | |
| { | |
| // Unwrap the inner exception from reflection | |
| if (ex.InnerException != null) | |
| { | |
| throw ex.InnerException; | |
| } | |
| throw; | |
| } | |
| } | |
| // Helper: Simulates EditorApplication.update callback. | |
| // Uses a cached reflection lookup to invoke ProcessQueue on the TransportCommandDispatcher. | |
| // This centralizes the private reflection and fails fast with a clear assertion if the | |
| // target method signature or visibility changes. | |
| private static readonly System.Reflection.MethodInfo s_processQueueMethod = | |
| typeof(TransportCommandDispatcher).GetMethod( | |
| "ProcessQueue", | |
| System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Static); | |
| private void SimulateEditorUpdate() | |
| { | |
| // Fail fast with a clear message if the method can no longer be located. | |
| NUnit.Framework.Assert.That( | |
| s_processQueueMethod, | |
| NUnit.Framework.Is.Not.Null, | |
| "TransportCommandDispatcher.ProcessQueue signature or visibility has changed. " + | |
| "Update the test's reflection hook accordingly."); | |
| try | |
| { | |
| s_processQueueMethod!.Invoke(null, null); | |
| } | |
| catch (System.Reflection.TargetInvocationException ex) | |
| { | |
| // Unwrap the inner exception from reflection for clearer test failures. | |
| if (ex.InnerException != null) | |
| { | |
| throw ex.InnerException; | |
| } | |
| throw; | |
| } | |
| } |
Collaborator
|
Thanks much. I'm reviewing this, hopefully can test tomorrow and merge. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Potentially Fixes GitHub issue #577: 28ms UI stutters every ~1 second when MCP plugin is running
Root Cause
The
MCPForUnityEditorWindow.OnEditorUpdate()hook was being called every frame (60+ times/sec) to poll connection status. This triggeredUpdateConnectionStatus()→IsLocalHttpServerReachable(), which makes network socket connection attempts with 50ms timeouts on the main thread.Architectural Changes
OnEditorUpdatehook that polled every frameRefreshAllData)RequestHealthVerification)OnFocus)Connection status now only updates when actually needed.
Performance Improvements
Eliminated main thread blocking network I/O
TransportCommandDispatcher: Early exit optimization
Pending.Countcheck prevents List allocations when idleEditorStateCache: Sequence-based caching
DeepClone()calls on everyGetSnapshot()StdioBridgeHost: Consistent early exit pattern
commandQueue.Countcheck as TransportCommandDispatcherTesting
Summary by Sourcery
Reduce editor stuttering and GC pressure by avoiding unnecessary per-frame work and allocations in MCP editor integration.
Bug Fixes:
Enhancements:
Tests:
Summary by CodeRabbit
Performance
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.