Skip to content
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

Improve PRs/issues loading performance #1107

Merged
merged 5 commits into from
Sep 10, 2021

Conversation

Fs00
Copy link
Contributor

@Fs00 Fs00 commented Sep 9, 2021

While I was investigating why loading a PRs with lots of comments was extremely slow, I opened up the Android Studio network profiler and saw this:
pr_before

What was happening in thread RxNewThreadScheduler-23 was that these API calls used to load the PR conversation:

return Single.zip(
issueCommentItemSingle,
eventItemSingle,
reviewTimelineSingle,
commitCommentWithoutReviewSingle,
(comments, events, reviewItems, commentsWithoutReview) -> {

were all being executed sequentially!
To execute them in parallel, I only needed to explicitly subscribe each of those Singles on a background thread (63d4032).
After this change, this is what I saw in the profiler:
pr-after
Much better. The conversation of that specific PR (PowerShell/PowerShell#9900) now takes half the time to load (from 20 to 10 secs).

I noticed that also issues had the same performance problem (albeit much less serious), so I fixed it also for them (ff17c68).

There are some other misc small changes in this PR:

  • all Singles are now subscribed on Schedulers.io() rather than on newThread() (0aef6f6). This avoids spawning a thread for literally every asynchronous action in the app, which is unnecessarily costly.
    io() uses a shared thread pool that grows as needed, so we get the benefits of newThread but reusing previously allocated threads whenever possible.
  • I've removed preview headers for checks API, since it became an official part of the API last year.
  • I've renamed the PullRequestFragment to PullRequestConversationFragment, to improve clarity and symmetry with PullRequestFilesFragment.

@maniac103
Copy link
Collaborator

Great, thanks! Code looks good, I'll give it a run and merge before release tomorrow.

@smichel17
Copy link
Contributor

smichel17 commented Sep 9, 2021

It's nice to have another person besides @maniac103 helping with development, again :)

@maniac103
Copy link
Collaborator

It's nice to have another person besides @maniac103 helping with development, again :)

Indeed, exactly that was my thought as well! Unfortunately I lack time due to work and family time, so it's nice to see someone who can help maintaining the app to at least keep it working decently :-)

@Fs00
Copy link
Contributor Author

Fs00 commented Sep 9, 2021

It's nice to have another person besides @maniac103 helping with development, again :)

Indeed, exactly that was my thought as well!

Nice to hear that :)
I've been using this app since a long time and I've decided it was time to start improving it. The good thing is, it isn't neither dead like FastHub nor ugly like the official GH app 😆

Currently I have a list of things I want to address/improve. You can be sure that I won't make ground-breaking changes or add big new features 😅 (I don't even have time to).

@Fs00
Copy link
Contributor Author

Fs00 commented Sep 9, 2021

Unfortunately I lack time due to work and family time

Btw no problem for that, life happens :)
I might occasionally ask you for some help in case I run into difficulties I can't find a way through. Is there a way I can contact you without opening an issue here on the repo? @maniac103

@maniac103
Copy link
Collaborator

@Fs00 My email is in the commits ;-)

@Fs00
Copy link
Contributor Author

Fs00 commented Sep 9, 2021

Oh, right 😂

@maniac103 maniac103 merged commit 2baf3e9 into slapperwan:master Sep 10, 2021
@Fs00 Fs00 deleted the bodytext-optimization branch September 10, 2021 06:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants