Skip to content

Fail fast in the matcher, let the debug outputs use the cache#1251

Merged
tylerbenson merged 8 commits into
masterfrom
devinsba/fast-reference-matcher
Feb 28, 2020
Merged

Fail fast in the matcher, let the debug outputs use the cache#1251
tylerbenson merged 8 commits into
masterfrom
devinsba/fast-reference-matcher

Conversation

@devinsba
Copy link
Copy Markdown
Contributor

@devinsba devinsba commented Feb 24, 2020

Minimal CPU improvement unfortunately but does at least help some.

startup plot

Memory stats should be treated as grain of salt. The test app results in 5:1 cache hit:miss ratio and 2 total mismatches (scala concurrent classes). So doesn't really exercise this cache.

memory plot

Copy link
Copy Markdown
Contributor

@tylerbenson tylerbenson left a comment

Choose a reason for hiding this comment

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

Looks reasonable... what kind of improvement did you see with this?

@devinsba
Copy link
Copy Markdown
Contributor Author

I'm still in progress on the changes. I'll put results in the description after

@devinsba devinsba force-pushed the devinsba/fast-reference-matcher branch from 0d010f0 to af67dfa Compare February 24, 2020 19:39
@devinsba
Copy link
Copy Markdown
Contributor Author

devinsba commented Feb 24, 2020

So looks like this change didn't really give much improvement. The startup improvement appears to be modest

startup plot

@devinsba devinsba marked this pull request as ready for review February 25, 2020 17:07
@devinsba devinsba requested a review from a team as a code owner February 25, 2020 17:07
public class ReferenceMatcher implements WeakMap.ValueSupplier<ClassLoader, Boolean> {
private final WeakMap<ClassLoader, Boolean> mismatchCache = newWeakMap();
private final Reference[] references;
private final Set<String> helperClassNames;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

isn't this field always empty set ? where do we add elements to it ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Given that it is final, it must be assigned in the constructor.

Copy link
Copy Markdown
Contributor Author

@devinsba devinsba Feb 26, 2020

Choose a reason for hiding this comment

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

It is set to a list of the args in the constructor(s)

* @return A list of mismatched sources. A list of size 0 means the reference matches the class.
*/
public static List<Reference.Mismatch> checkMatch(Reference reference, ClassLoader loader) {
public static List<Reference.Mismatch> checkMatch(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

private ?

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.

Sure

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

still not private?

@devinsba
Copy link
Copy Markdown
Contributor Author

I just noticed a downside to this change is the muzzle gradle task takes considerably longer, are we fine with that?

@devinsba devinsba requested a review from tylerbenson February 26, 2020 14:43
@dougqh
Copy link
Copy Markdown
Contributor

dougqh commented Feb 26, 2020

I just noticed a downside to this change is the muzzle gradle task takes considerably longer, are we fine with that?

In general, I think trading build time for runtime is a good idea. But I guess our runtime gain was rather small. Although, we'd probably see bigger gains on something where there are multiple versions instrumentations at play at the same time.

@randomanderson
Copy link
Copy Markdown
Contributor

I just noticed a downside to this change is the muzzle gradle task takes considerably longer, are we fine with that?

I checked circle CI. The build was 11:40 instead of ~10ish. Not that big of a difference. The real fix is not checking all ~1000 versions for the amazon sdk

Copy link
Copy Markdown
Contributor

@randomanderson randomanderson left a comment

Choose a reason for hiding this comment

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

👍

@tylerbenson tylerbenson merged commit 5527614 into master Feb 28, 2020
@tylerbenson tylerbenson deleted the devinsba/fast-reference-matcher branch February 28, 2020 17:56
@tylerbenson tylerbenson added this to the 0.45.0 milestone Mar 2, 2020
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.

5 participants