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

stop calling `repository.didUpdate` when commit message changes. #1464

Merged
merged 4 commits into from May 16, 2018

Conversation

Projects
None yet
3 participants
@annthurium
Contributor

annthurium commented May 15, 2018

We are performing a bunch of unnecessary operations when the commit message changes. It's causing some slowness.

See #1463 for details.

As far as I can tell, updating the repository when the commit message changes may have been necessary at some point in the past (like for the old way amending used to work, perhaps?) But it doesn't seem to be useful now. Nevertheless, it's important to test carefully here to make sure removing this won't cause any surprise regressions.

Test Plan

manually tested the following flows:

  • make a commit from Git pane, with and without co authors
  • undo a commit, with and without co authors
  • make a commit with expanded editor
  • toggle back and forth between expanded editor and commit editor in Git pane
  • make a commit from the command line. Verify that commit shows up in recent commit history.

unit tests

  • added a test that spies on the function we don't want to be called. Negated assertions to prove that it's actually testing the thing we care about.

annthurium added some commits May 15, 2018

add unit test
negated assertions ftw

@annthurium annthurium requested review from smashwilson and maxbrunsfeld May 15, 2018

@maxbrunsfeld

Thanks for fixing this @annthurium!

@@ -87,6 +87,16 @@ describe('CommitController', function() {
assert.strictEqual(wrapper.find('CommitView').prop('message'), 'some message');
});
it('repository.didUpdate is not called when commit message changes', function() {

This comment has been minimized.

@maxbrunsfeld

maxbrunsfeld May 15, 2018

Contributor

This is very minor, but it looks like all of the surrounding tests' descriptions are phrased so that they sound right after the word "it". I'd maybe tweak the description to say something like

it("does not cause the repository to update").

annthurium added some commits May 16, 2018

this.commitMessage = message;
if (oldMessage !== message) {
this.didUpdate();
}

This comment has been minimized.

@smashwilson

smashwilson May 16, 2018

Member

Oh, interesting. Changing the message triggered an update, and therefore at least a partial re-render of the React tree, so that UI elements that change state based on the message's length and presence have a chance to be updated. Specifically I'm thinking of:

  • The enablement of the "Commit" button; it should be disabled if the message is empty; and
  • The "remaining characters" counter that changes as you type the subject line;

But, the way the commit message is handled has been shuffled a lot with the introduction of the AtomTextEditor wrapper component and various refactorings, so I'm not 100% sure that these things won't be updated by something else.

Can you confirm that those didn't break? If they still work, then we definitely should take this out because it's not needed. If they don't, an alternate approach would be to fuss with either the UserStore or the GitTabController to fine-tune when loadUsersFromLocalRepo is invoked. We should only really need to re-call it when something has changed that would change the available co-authors, like a fetch or a commit.

This comment has been minimized.

@smashwilson

smashwilson May 16, 2018

Member

Looking a little more closely at @maxbrunsfeld's flame graph, one thing that stands out to me is that we're calling setState() and running an entire render cycle every time loadUsersFromLocalRepo finishes, even though the result is the same. I'm guessing that's the setState call in here:

this.userStore = new UserStore({
repository: this.props.repository,
onDidUpdate: users => {
this.setState({mentionableUsers: users});
},
});

Maybe we should also prevent UserStore.onDidUpdate() from firing when the user list hasn't actually changed... ?

This comment has been minimized.

@annthurium

annthurium May 16, 2018

Contributor

@smashwilson yes, my original solution was for the UserStore to not invoke loadUsersFromLocalRepo when not needed.

However, I noticed we were also performing a bunch of other unnecessary operations on the repository when the commit message was updated (, and figured it would be a better performance improvement to fix the problem here instead of in the user store.

I did some additional testing and confirmed that the commit button is still disabled if the message is empty, and the remaining characters counter changes as you type in the commit box. Which makes sense, because the CommitView component has its own logic for dealing with commit message updates, that forces a re render when the message has changed.

This comment has been minimized.

@smashwilson

smashwilson May 16, 2018

Member

Which makes sense, because the CommitView component has its own logic for dealing with commit message updates, that forces a re render when the message has changed.

This was the piece that I wasn't sure was in place or not. Excellent 🍰

This comment has been minimized.

@annthurium

annthurium May 16, 2018

Contributor

great, thanks @smashwilson !

@annthurium annthurium merged commit ff5fb6c into master May 16, 2018

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@annthurium annthurium deleted the tt-18-may-load-users-bug branch May 16, 2018

@kuychaco kuychaco restored the tt-18-may-load-users-bug branch Oct 24, 2018

kuychaco added a commit that referenced this pull request Oct 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment