-
Notifications
You must be signed in to change notification settings - Fork 124
Optimised connection params building #852
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
Conversation
🦋 Changeset detectedLatest commit: cb29c3c The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughRefactors connection query parameter assembly in SignalClient: replaces list-based building with a StringBuilder and helper Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 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
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)livekit-android-sdk/src/main/java/io/livekit/android/room/SignalClient.kt (1)
🔇 Additional comments (2)
✏️ Tip: You can disable this entire section by setting Comment |
|
Hmm, it doesn't really make sense that the string concatenation here would account for such large slowdown; 150ms is a very long time for just adding 10 or so strings together. There's probably a separate underlying issue here. Nonetheless, it's still a good optimization to have here though. Thanks for the PR! |
|
I was benchmarking in the sample app in a debug environment so that could add to the reason of slow execution but as you said, nevertheless it's a good optimisation because for each query param we are concating twice so for about 10 parameters that's 20 concatenations plus variable assignments due to the nature of |
I noticed creation of
wsUrlStringtakes over 100ms, sometimes taking close to 150ms on my test device(Oneplus Nord CE 2 Lite). The function just builds a string of query parameters but does it inefficiently by using string concatenation. I replaced it withStringBuilderand prepared the string as we go. This brought down the execution time down to ~5ms or lesser.Summary by CodeRabbit
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.