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

Moe Sync #1316

Merged
merged 23 commits into from
Jul 29, 2019
Merged

Moe Sync #1316

merged 23 commits into from
Jul 29, 2019

Conversation

nick-someone
Copy link
Member

This code has been reviewed and submitted internally. Feel free to discuss on the PR and we can submit follow-up changes as necessary.

Commits:

Update javadoc to cover desugaring

45553cf


New checker to discourage extraneous uses of methodInvocation()

Many uses are leftovers from a time before MethodMatcher existed, and can simply
be removed.

b4b8896


Optimize InvalidPatternSyntax checker

Combining common cases and removing onDescendantOf doubles the speed of
this checker, and shaves 0.2% off of total javac time.

b554a1e


Fix InconsistentCapitalization errorprone warning

b30aba8


Add a hint to use VisitorState#getSourceForNode in TreeToString.

This is already used in a SuggestedFix if possible, but mentioning it in the description seems potentially useful.

Fixes external #1308

398d837


Make FieldCanBeLocal respect annotations.

That is: copy them, and don't propose making fields local if they have annotations that can't be used on local variables.

113a190


Add preliminary support for preferring java.time.Instant-based APIs.

553b8f4


Clean up various misguided method matchers

Mostly, unnecessary uses of onDescendantOf

8aefe67


Convert from stream to a for loop in hasAttribute

It's called more often than you might guess, and it's so short that
85% of its time was spent on stream overhead.

01515b0


Allocate fewer VisitorState objects

Most of these can be shared among matchers for the AST node they're
processing - the only thing that (rarely) differs is the
SuppressionState. Adding a check to decide whether we need to
allocate a new object reduces time spent allocating (including the new
time spent deciding) by 75%, from around 1.2% of javac time to 0.35% of
javac time.

3e5af8e


Fix WithSignatureDiscouraged errorprone warnings

df450d1


Remove extraneous uses of methodInvocation()

Many uses are leftovers from a time before MethodMatcher existed, and can simply
be removed.

265cab5


Add a warning if calling a org.joda.time.Instant-based API when a java.time.Instant-based overload exists.

Also fix a bug where Duration constructor invocations were not being properly re-written.

3d88760


Copy straight to set, instead of going through list

9f0ec32


Convert from String to Supplier only once per string

Iterables.transform is fairly expensive if you're going to iterate over the
result more than once, because it performs the transformation each time.

The speedup from this is pretty insignificant, but it's not nothing.

73c063a


Rewrite foo(javaDuration.getSeconds(), SECONDS) -> foo(javaDuration).

9aa023d


Add a test about being embedded inside another expression

f2cb45f


Add a cache around ASTHelpers.isInherited

This is called quite often, and with a very small number of distinct
arguments. I'd like to speed up the actual implementation, or avoid
calling it so often, but for starters introducing a cache speeds up
the method by around 80%, shaving 2% off of the entire javac action.

This adds a new dependency on Caffeine; I considered using a Guava
Cache since we already depend on Guava, but Caffeine's version is 3
times as fast for this use case.

e1acde9


Short circuit inside PreferDurationOverload when scanning for an acceptable overload.

e876562


Fix UnusedException warnings in SourceFile.java

899d205


MethodCanBeStatic: don't match synchronized methods, as that's behaviour-changing.

e3b24b9


Fix a bug in PreferDurationOverload that was triggering it for every invocation not-inside a method body.

bf3ec8f


Remove unused parameter

80eed45

clm and others added 23 commits July 24, 2019 13:00
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=258431740
Many uses are leftovers from a time before MethodMatcher existed, and can simply
be removed.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=258434141
Combining common cases and removing onDescendantOf doubles the speed of
this checker, and shaves 0.2% off of total javac time.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=258451279
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=258696537
This is already used in a SuggestedFix if possible, but mentioning it in the description seems potentially useful.

Fixes external #1308

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=258724013
That is: copy them, and don't propose making fields local if they have annotations that can't be used on local variables.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=258768997
Mostly, unnecessary uses of onDescendantOf

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=258821547
It's called more often than you might guess, and it's so short that
85% of its time was spent on stream overhead.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=258825400
Most of these can be shared among matchers for the AST node they're
processing - the only thing that (rarely) differs is the
SuppressionState.  Adding a check to decide whether we need to
allocate a new object reduces time spent allocating (including the new
time spent deciding) by 75%, from around 1.2% of javac time to 0.35% of
javac time.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=258845118
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=258870076
Many uses are leftovers from a time before MethodMatcher existed, and can simply
be removed.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=258924051
…a.time.Instant-based overload exists.

Also fix a bug where Duration constructor invocations were not being properly re-written.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=259003057
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=259007576
Iterables.transform is fairly expensive if you're going to iterate over the
result more than once, because it performs the transformation each time.

The speedup from this is pretty insignificant, but it's not nothing.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=259012455
This is called quite often, and with a very small number of distinct
arguments. I'd like to speed up the actual implementation, or avoid
calling it so often, but for starters introducing a cache speeds up
the method by around 80%, shaving 2% off of the entire javac action.

This adds a new dependency on Caffeine; I considered using a Guava
Cache since we already depend on Guava, but Caffeine's version is 3
times as fast for this use case.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=259064453
…ptable overload.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=259375033
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=259407224
…our-changing.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=259482803
…invocation not-inside a method body.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=259585368
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=259593984
Copy link
Contributor

@Stephan202 Stephan202 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice to see so much effort being put into optimizing Error Prone's performance!

@nick-someone nick-someone merged commit dfb09fa into master Jul 29, 2019
@nick-someone nick-someone deleted the sync-master-2019/07/24 branch July 29, 2019 13:15
nick-someone added a commit that referenced this pull request Jul 31, 2019
Per comment in #1316

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=260513433
nick-someone added a commit that referenced this pull request Jul 31, 2019
@nick-someone nick-someone mentioned this pull request Jul 31, 2019
@netdpb netdpb mentioned this pull request Aug 13, 2019
netdpb pushed a commit that referenced this pull request Aug 13, 2019
Per comment in #1316

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=260513433
netdpb pushed a commit that referenced this pull request Aug 13, 2019
eaftan pushed a commit that referenced this pull request Aug 13, 2019
Per comment in #1316

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=260513433
eaftan pushed a commit that referenced this pull request Aug 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants