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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid stalls caused by large diffs fetched from GitHub #2195

Merged
merged 20 commits into from Jul 16, 2019

Conversation

@smashwilson
Copy link
Member

commented Jul 4, 2019

Please be sure to read the contributor's guide to the GitHub package before submitting any pull requests.

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • Suggestion: You can use checklists to keep track of progress for the sections on metrics, tests, documentation, and user research.

Description of the Change

Our PullRequestPatchContainer component fetches and parses diffs from GitHub's REST API, then makes this available to child components as a MultiFilePatch. Unfortunately, it seems that pull requests with many or large files -- such as atom/flight-manual.atom.io#541 -- can cause stalls of a full minute (馃槺) while these are being parsed.

Interestingly, the stalls seem to be in what-the-diff's PEG parser rather than our own buildMultiFilePatch() function; in the past, buildMultiFilePatch() was the bottleneck. As a consequence our "large diff detection" logic isn't helping.

To mitigate this:

  • I'm using conditional requests to avoid fetching and re-parsing the same large diff from dotcom more than once.
  • We don't need the patch at all in CommentDecorationsContainer if the current PR has no review comments. Let's find review comments first and bail if there are none (which is the common case anyway).
  • Finally, I'll investigate setting an upper size bound on which diffs we attempt to parse from dotcom.
    • It would be good to offer some notification of degraded functionality when we do have to drop comments.
    • When the patch is large enough to trigger the MultiFilePatch "large diff" gate, but not the raw diff filter, comments are unavailable, but only until the diff is expanded. Let's show a notification with a control that allows users to opt in to parsing the diff when it does have comments.
    • We'll need to make sure that the PullRequestDetailView can gracefully handle a missing diff. Or at least not crash horribly.
    • Settling on an appropriate size limit will take some experimentation.

Screenshot/Gif

Here's what I'm doing if you open an editor on a file that has review comments, but whose patch has been omitted for size:

too-large

Alternate Designs

N/A

Benefits

Using Atom to edit files on branches corresponding to pull requests with "large" diffs will no longer cause periodic stalls.

Possible Drawbacks

The review comment and patch fetches can happen in parallel presently because they're independent operations. By making the patch fetch depend on the result of the review comment aggregation, it'll take longer to load all of the data and display the first review comment decoration or populate the reviews tab.

Applicable Issues

N/A

Metrics

N/A

Tests

  • Normal case: small patch, review comments.
    • Ensure comment decorations are rendered appropriately.
    • Ensure that the ETag and 304 response are used to prevent re-parsing the patch.
  • On a PR with no review comments, ensure the patch is not fetched at all.
  • On a PR with a large file patch and a small patch that contains review comments:
    • Ensure the large file patch is filtered out and does not cause a performance stall.
    • Ensure the review comments on the file with the small patch are rendered appropriately.
  • On a PR with a large file patch that contains review comments:
    • Ensure the large file patch is filtered out and does not cause a performance stall.
    • Ensure a block decoration is rendered at the front of the editor of the file with the large patch.
    • Ensure that opening the ReviewsItem on the PR does not crash as a result of the missing patch.

Documentation

N/A

Release Notes

  • Improved the performance of the GitHub package when working on checked-out pull request with a large diff.

User Experience Research (Optional)

N/A

@codecov

This comment has been minimized.

Copy link

commented Jul 4, 2019

Codecov Report

Merging #2195 into master will decrease coverage by 0.01%.
The diff coverage is 91.26%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2195      +/-   ##
=========================================
- Coverage   92.72%   92.7%   -0.02%     
=========================================
  Files         215     216       +1     
  Lines       12269   12350      +81     
  Branches     1796    1814      +18     
=========================================
+ Hits        11376   11449      +73     
- Misses        893     901       +8
Impacted Files Coverage 螖
lib/containers/reviews-container.js 100% <酶> (酶) 猬嗭笍
lib/models/patch/filter.js 100% <100%> (酶)
lib/containers/comment-decorations-container.js 100% <100%> (酶) 猬嗭笍
...ntrollers/editor-comment-decorations-controller.js 91.66% <100%> (+1.92%) 猬嗭笍
lib/views/multi-file-patch-view.js 99.78% <100%> (酶) 猬嗭笍
lib/models/patch/patch.js 100% <100%> (酶) 猬嗭笍
lib/models/patch/builder.js 100% <100%> (酶) 猬嗭笍
lib/containers/comment-positioning-container.js 78.82% <30%> (-6.51%) 猬囷笍
lib/containers/pr-patch-container.js 92.98% <92.59%> (-1.89%) 猬囷笍
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
螖 = absolute <relative> (impact), 酶 = not affected, ? = missing data
Powered by Codecov. Last update 7e7c414...4712435. Read the comment docs.

@smashwilson

This comment has been minimized.

Copy link
Member Author

commented Jul 4, 2019

cc @jasonrudolph and @nathansobo who brought this to our attention 馃槃

smashwilson added some commits Jul 5, 2019

@smashwilson

This comment has been minimized.

Copy link
Member Author

commented Jul 5, 2019

Lol okay then:

smashwilson @ hubtop ~/src/atom/github (aw/large-dotcom-diffs %=) 
$ curl https://api.github.com/repos/atom/flight-manual.atom.io/pulls/541 -H 'Accept: application/vnd.github.v3.diff' -o big-patch.diff
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  124M  100  124M    0     0  3021k      0  0:00:42  0:00:42 --:--:-- 3890k
smashwilson @ hubtop ~/src/atom/github (aw/large-dotcom-diffs %=) 
$ du -h big-patch.diff 
128M    big-patch.diff

smashwilson added some commits Jul 5, 2019

@smashwilson smashwilson referenced this pull request Jul 16, 2019
1 of 1 task complete

smashwilson added some commits Jul 16, 2019

@smashwilson smashwilson marked this pull request as ready for review Jul 16, 2019

@smashwilson

This comment has been minimized.

Copy link
Member Author

commented Jul 16, 2019

It looks like GitHub itself marks review comments as "outdated" on files with patches that grow beyond 1MB, so I'll match that threshold here.

Should be good to go 馃憣

@smashwilson smashwilson merged commit 4035bde into master Jul 16, 2019

14 of 17 checks passed

Consider pull request for release board Consider pull request for release board
Details
Consider pull request for release board Consider pull request for release board
Details
Consider pull request for release board Consider pull request for release board
Details
atom.github Build #20190716.16 succeeded
Details
atom.github (Lint) Lint succeeded
Details
atom.github (Linux beta) Linux beta succeeded
Details
atom.github (Linux dev) Linux dev succeeded
Details
atom.github (Linux stable) Linux stable succeeded
Details
atom.github (MacOS beta) MacOS beta succeeded
Details
atom.github (MacOS dev) MacOS dev succeeded
Details
atom.github (MacOS stable) MacOS stable succeeded
Details
atom.github (Snapshot) Snapshot succeeded
Details
atom.github (Windows beta) Windows beta succeeded
Details
atom.github (Windows dev) Windows dev succeeded
Details
atom.github (Windows stable) Windows stable succeeded
Details
codecov/patch 91.26% of diff hit (within 2% threshold of 92.72%)
Details
codecov/project 92.7% (-0.02%) compared to 7e7c414
Details

Release : 9 May 2019 - 5 June 2019 : v0.30.0 automation moved this from In progress to Merged Jul 16, 2019

@smashwilson smashwilson deleted the aw/large-dotcom-diffs branch Jul 16, 2019

@jasonrudolph

This comment has been minimized.

Copy link
Member

commented Jul 16, 2019

Thanks for digging into this, @smashwilson. 鈿★笍馃檱

@smashwilson smashwilson referenced this pull request Jul 18, 2019
4 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
2 participants
You can鈥檛 perform that action at this time.