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

Performance improvements for cli json report and minor refactoring #724

Merged
merged 1 commit into from Jan 1, 2018

Conversation

Projects
None yet
2 participants
@wisechengyi
Collaborator

wisechengyi commented Dec 28, 2017

Major change:

  • Identified hotspot in JsonElem.hashCode because it hashes children recursively. Hence simplified hashing scheme, reducing CPU time on JsonReport.apply() from 64.7 sec to 2.5 sec

Before:
screen shot 2017-12-28 at 1 26 20 pm

After:
screen shot 2017-12-28 at 1 26 27 pm

Other minor changes:

  • Make JsonRequirement use immutable classes
  • Simplify JsonRequirement composition

@wisechengyi wisechengyi changed the title from Make JsonPrintRequirement take immutable fileByArtifact to Minor refactoring for cli json report Dec 28, 2017

@wisechengyi wisechengyi changed the title from Minor refactoring for cli json report to Refactoring and Performance improvements for cli json report Dec 28, 2017

@wisechengyi wisechengyi changed the title from Refactoring and Performance improvements for cli json report to Performance improvements for cli json report and minor refactoring Dec 28, 2017

@wisechengyi wisechengyi requested a review from alexarchambault Dec 28, 2017

@alexarchambault

alexarchambault approved these changes Dec 29, 2017 edited

👍

Maybe squash a bit before merging.

(github says you need to rebase too. Ok, I updated the branch settings to not need it - but also clicked update branch before that, that added a useless merge commit)

@wisechengyi

This comment has been minimized.

Show comment
Hide comment
@wisechengyi

wisechengyi Dec 29, 2017

Collaborator

That should be fine. GitHub has the feature to merge the PR squashed, so we don't have to do it manually

Collaborator

wisechengyi commented Dec 29, 2017

That should be fine. GitHub has the feature to merge the PR squashed, so we don't have to do it manually

@wisechengyi

This comment has been minimized.

Show comment
Hide comment
@wisechengyi

wisechengyi Dec 29, 2017

Collaborator

For example #692 was squash merged, and it showed up as a single commit on master

Collaborator

wisechengyi commented Dec 29, 2017

For example #692 was squash merged, and it showed up as a single commit on master

@alexarchambault

This comment has been minimized.

Show comment
Hide comment
@alexarchambault

alexarchambault Dec 29, 2017

Member

Yep, I know that feature, I meant you could have squashed it in two commits, then merge as is.

Member

alexarchambault commented Dec 29, 2017

Yep, I know that feature, I meant you could have squashed it in two commits, then merge as is.

@wisechengyi

This comment has been minimized.

Show comment
Hide comment
@wisechengyi

wisechengyi Dec 29, 2017

Collaborator

Copying from gitter:

Hi @alexarchambault, by squashing into 2 commits, are you mostly concerned about the commit messages showing up in master?

Typically each commit message does not really master, and with Github's sqaush and merge, the description of the PR can go into the final commit message.

Maybe we can be more clear about how the PR should go. For example, https://github.com/pantsbuild/pants/blob/a439c87a1cef96f31bda6eb557716d5df3fd2a09/src/docs/howto_contribute.md#committing-a-change

Additional benefits if we keep the commits in the PR:

  1. Github reviews are associated with the commit sha, so after force push after rebase/squash, they may be gone.
  2. It allows for inspecting a range of shas on github to see what changes are made after the review for example.
  3. Book keeping.
  4. Personally I am bad at squashing commits manually :D
Collaborator

wisechengyi commented Dec 29, 2017

Copying from gitter:

Hi @alexarchambault, by squashing into 2 commits, are you mostly concerned about the commit messages showing up in master?

Typically each commit message does not really master, and with Github's sqaush and merge, the description of the PR can go into the final commit message.

Maybe we can be more clear about how the PR should go. For example, https://github.com/pantsbuild/pants/blob/a439c87a1cef96f31bda6eb557716d5df3fd2a09/src/docs/howto_contribute.md#committing-a-change

Additional benefits if we keep the commits in the PR:

  1. Github reviews are associated with the commit sha, so after force push after rebase/squash, they may be gone.
  2. It allows for inspecting a range of shas on github to see what changes are made after the review for example.
  3. Book keeping.
  4. Personally I am bad at squashing commits manually :D
@alexarchambault

This comment has been minimized.

Show comment
Hide comment
@alexarchambault

alexarchambault Dec 30, 2017

Member

The way to merge PRs could be clarified, yes… I'm more for loose guidelines rather than hard ones.

That could be either squash-and-merge if the PR basically adds or changes one thing, or merge with merge commit if the succesive commits of the PR somehow make sense without the next ones (that is, in a PR with commits a -> b -> c, just adding a to master would have been fine, as would have adding a -> b).

I often tend to add a bunch of small things in one PR, so I usually follow the latter case.

Member

alexarchambault commented Dec 30, 2017

The way to merge PRs could be clarified, yes… I'm more for loose guidelines rather than hard ones.

That could be either squash-and-merge if the PR basically adds or changes one thing, or merge with merge commit if the succesive commits of the PR somehow make sense without the next ones (that is, in a PR with commits a -> b -> c, just adding a to master would have been fine, as would have adding a -> b).

I often tend to add a bunch of small things in one PR, so I usually follow the latter case.

@wisechengyi

This comment has been minimized.

Show comment
Hide comment
@wisechengyi

wisechengyi Dec 30, 2017

Collaborator
Collaborator

wisechengyi commented Dec 30, 2017

@wisechengyi

This comment has been minimized.

Show comment
Hide comment
@wisechengyi

wisechengyi Dec 30, 2017

Collaborator

squashed. thanks

Collaborator

wisechengyi commented Dec 30, 2017

squashed. thanks

@alexarchambault

Ah, I meant squash-and-merge via github would have been ok, if you wanted to squash into a single commit. Merge as soon as you want!

@wisechengyi

This comment has been minimized.

Show comment
Hide comment
@wisechengyi

wisechengyi Jan 1, 2018

Collaborator
Collaborator

wisechengyi commented Jan 1, 2018

@alexarchambault

This comment has been minimized.

Show comment
Hide comment
@alexarchambault

alexarchambault Jan 1, 2018

Member

Oh, possible github misconfig on my side. Is it better now?

Member

alexarchambault commented Jan 1, 2018

Oh, possible github misconfig on my side. Is it better now?

@wisechengyi wisechengyi merged commit 5da2210 into coursier:master Jan 1, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@wisechengyi

This comment has been minimized.

Show comment
Hide comment
@wisechengyi

wisechengyi Jan 1, 2018

Collaborator

thank you!

Collaborator

wisechengyi commented Jan 1, 2018

thank you!

@wisechengyi wisechengyi deleted the wisechengyi:immutable_map branch Jan 1, 2018

@alexarchambault

This comment has been minimized.

Show comment
Hide comment
@alexarchambault

alexarchambault Jan 1, 2018

Member

@wisechengyi Will you need a release that would include your recent PRs?

Member

alexarchambault commented Jan 1, 2018

@wisechengyi Will you need a release that would include your recent PRs?

@wisechengyi

This comment has been minimized.

Show comment
Hide comment
@wisechengyi

wisechengyi Jan 1, 2018

Collaborator

This change does not require a release. We are currently consuming directly off master. Thanks for asking :)

Collaborator

wisechengyi commented Jan 1, 2018

This change does not require a release. We are currently consuming directly off master. Thanks for asking :)

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