Skip to content

Optimizations to relocation checks. Up to 3x faster#535

Merged
johnrengelman merged 1 commit intoGradleUp:masterfrom
Armaxis:armaxis/relocator-optimization
Jan 20, 2020
Merged

Optimizations to relocation checks. Up to 3x faster#535
johnrengelman merged 1 commit intoGradleUp:masterfrom
Armaxis:armaxis/relocator-optimization

Conversation

@Armaxis
Copy link
Copy Markdown
Contributor

@Armaxis Armaxis commented Jan 6, 2020

Performance boost on large project (5:48 -> 2:02 min total duration).

  • Changed signature of methods to accept String as path instead of RelocatePathContext/RelocateClassContext to prevent unnecessary allocations for builder objects
  • Added extra string length checks to avoid string operations where possible
  • Reordered checks in canRelocateClass so that expensive string replacement is the last operation
  • (Main Optimization) Avoided string concatenation (which led to creating StringBuilder instances and expensive array copy operations) by adding check for first character being /: if so - perform a startsWith check with an offset.

interface Relocator {
String ROLE = Relocator.class.getName()

boolean canRelocatePath(RelocatePathContext context)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this is a breaking change. Is it really necessary? These context objects were introduced a while ago in order to abstract the details away as I was finding that we needed to add additional data for certain features.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Oh, I see, you're just removing it from some of the methods.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, I removed context only from the methods that aren't using it. It's done to decrease the number of allocations we make and to save a bit of time on creating the builder

@johnrengelman johnrengelman added this to the 6.0 milestone Jan 14, 2020
@johnrengelman
Copy link
Copy Markdown
Collaborator

@Armaxis there are some conflicts now on this branch. Would you mind resolving them and then I’ll get this in as part of the 6.0 release.

@Armaxis
Copy link
Copy Markdown
Contributor Author

Armaxis commented Jan 19, 2020

Sure, will do later today

1 similar comment
@Armaxis
Copy link
Copy Markdown
Contributor Author

Armaxis commented Jan 19, 2020

Sure, will do later today

@Armaxis Armaxis force-pushed the armaxis/relocator-optimization branch from c8b4631 to dd19e9f Compare January 20, 2020 01:23
…mance boost on large project (5:48 -> 2:02 min total duration).

- Changed signature of methods to accept String as path instead of RelocatePathContext/RelocateClassContext to prevent unnecessary allocations for builder objects
- Added extra string length checks to avoid string operations where possible
- Reordered checks in canRelocateClass so that expensive string replacement is the last operation
- (Main Optimization) Avoided string concatenation (which led to creating StringBuilder instances and expensive array copy operations) by adding check for first character being '/': if so - perform a startsWith check with an offset.

- Removed an incorrect length check
- Moved context creation outside the loop
@Armaxis Armaxis force-pushed the armaxis/relocator-optimization branch from dd19e9f to bce1218 Compare January 20, 2020 01:34
@johnrengelman johnrengelman merged commit a7f0908 into GradleUp:master Jan 20, 2020
@Armaxis Armaxis deleted the armaxis/relocator-optimization branch May 29, 2020 16:15
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.

2 participants