Skip to content

Improve JetBrains port processing performance #20907

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.

Sign up for GitHub

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
roboquat merged 2 commits into from
Jun 17, 2025
Merged

Improve JetBrains port processing performance #20907

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9039888
Improve JetBrains port processing performance
mustard-mh Jun 17, 2025
1b318f3
revert sdkman path
mustard-mh Jun 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 21 additions & 1 deletion ...-plugin/src/main/kotlin/io/gitpod/jetbrains/remote/AbstractGitpodPortForwardingService.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import com.intellij.openapi.components.service
import com.intellij.openapi.diagnostic.thisLogger
import com.intellij.remoteDev.util.onTerminationOrNow
import com.intellij.ui.RowIcon
import com.intellij.util.Alarm
import com.intellij.util.application
import com.jetbrains.rd.platform.codeWithMe.portForwarding.*
import com.jetbrains.rd.util.URI
Expand All @@ -35,6 +36,11 @@ abstract class AbstractGitpodPortForwardingService : GitpodPortForwardingService
private val ignoredPortsForNotificationService = service<GitpodIgnoredPortsForNotificationService>()
private val lifetime = Lifetime.Eternal.createNested()

// Throttling mechanism to prevent rapid successive port updates using IntelliJ Alarm
private var pendingUpdate: Status.PortsStatusResponse? = null
private val updateAlarm = Alarm(Alarm.ThreadToUse.SWING_THREAD, this)
Copy link

Copilot AI Jun 17, 2025

Choose a reason for hiding this comment

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

Using this as the parent Disposable may leak if AbstractGitpodPortForwardingService isn't disposable. Consider supplying a proper Disposable (e.g., the plugin’s lifecycle parent) or disposing the Alarm when the service shuts down.

Suggested change
private val updateAlarm = Alarm(Alarm.ThreadToUse.SWING_THREAD, this)
private val updateAlarm = Alarm(Alarm.ThreadToUse.SWING_THREAD, application)

Copilot uses AI. Check for mistakes.
private val updateThrottleMs = 100 // 100ms throttle
Copy link

Copilot AI Jun 17, 2025

Choose a reason for hiding this comment

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

[nitpick] The throttle interval is a magic number here. Consider extracting it into a named constant or configuration entry so it’s easier to adjust and document.

Suggested change
private val updateThrottleMs = 100 // 100ms throttle
private val updateThrottleMs = DEFAULT_UPDATE_THROTTLE_MS // Default throttle interval

Copilot uses AI. Check for mistakes.

init { start() }

private fun start() {
Expand Down Expand Up @@ -86,7 +92,7 @@ abstract class AbstractGitpodPortForwardingService : GitpodPortForwardingService
}

override fun onNext(response: Status.PortsStatusResponse) {
application.invokeLater { syncPortsListWithClient(response) }
application.invokeLater { throttledSyncPortsListWithClient(response) }
}

override fun onCompleted() {
Expand All @@ -103,6 +109,20 @@ abstract class AbstractGitpodPortForwardingService : GitpodPortForwardingService
return completableFuture
}

private fun throttledSyncPortsListWithClient(response: Status.PortsStatusResponse) {
// Store the latest update (overwrites any pending update)
pendingUpdate = response

// Cancel any existing scheduled update and schedule a new one
if (!updateAlarm.isEmpty) {
updateAlarm.cancelAllRequests()
}
Comment on lines +117 to +119
Copy link

Copilot AI Jun 17, 2025

Choose a reason for hiding this comment

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

[nitpick] Checking isEmpty before cancelAllRequests() may be unnecessary, since cancelAllRequests() is idempotent. You could simplify by always calling cancelAllRequests() or guard by isDisposed if needed.

Suggested change
if (!updateAlarm.isEmpty) {
updateAlarm.cancelAllRequests()
}
updateAlarm.cancelAllRequests()

Copilot uses AI. Check for mistakes.
updateAlarm.addRequest({
pendingUpdate?.let { syncPortsListWithClient(it) }
pendingUpdate = null
}, updateThrottleMs)
}

private fun isLocalPortForwardingDisabled(): Boolean {
return System.getenv("GITPOD_DISABLE_JETBRAINS_LOCAL_PORT_FORWARDING")?.toBoolean() ?: false
}
Expand Down
Loading