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

review: perf: Avoid slow CCE. Check acceptable type when possible #1541

Merged
merged 4 commits into from
Sep 29, 2017

Conversation

pvojtechovsky
Copy link
Collaborator

@pvojtechovsky pvojtechovsky commented Sep 18, 2017

I wanted to debug MainTest#testMain and I have found that it is terribly slow (needs nearly 19 min) in debug mode. 18 min is spent in creation of ClassCastException stacktraces.

This PR detects acceptable type from message of first CCE and then checks whether class of input is acceptable - so it avoids throwing of CCE - performs faster in debug (52s)

The performance improvement is only little in non debug mode (28s origin, 25s after improvement).

Actually this PR fails on java 7/8 source compliance (will be fixed by #1539)

@tdurieux
Copy link
Collaborator

THANK YOU!

}
private static final Pattern cceMessagePattern = Pattern.compile("(\\S+) cannot be cast to (\\S+)");
private static boolean canExtractTypeFromCCE = true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it normal that canExtractTypeFromCCE cannot be set to true?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the canExtractTypeFromCCE is here as fallback for the case, when somebody is using different java implementation whose CCE error message looks different. Only in such case the value is set to false and the detection of acceptable type from message is OFF.

Is it normal that canExtractTypeFromCCE cannot be set to true?

So I see actually no case when such setter would be needed.

@pvojtechovsky pvojtechovsky changed the title perf: Avoid slow CCE. Check acceptable type when possible WiP: perf: Avoid slow CCE. Check acceptable type when possible Sep 19, 2017
@pvojtechovsky pvojtechovsky changed the title WiP: perf: Avoid slow CCE. Check acceptable type when possible review: perf: Avoid slow CCE. Check acceptable type when possible Sep 23, 2017
@pvojtechovsky
Copy link
Collaborator Author

The ignoring of CCE caused by CtQuery filters was buggy, because for example CtConsumer implemented
A) by lambda
B) by local class
has different length of stacktrace when CCE is thrown on input parameter.

This PR solves this problem by these changes:

  1. CtQuery tries to detect acceptable input type from each callback (CtFunction, CtConsumableFunction, CtConsumer) by java reflection - but it is not possible for Lambdas, which does not provide correct parameter type.
  2. so in case of Lambdas the accepted input type is not known and only in their case the accepted input type is detected from ClassCastException after it fails first time.
  3. when accepted input type is known then it is always checked before the callback is called, so CCE is not thrown and it performs much faster in debug mode in Eclipse.
  4. if CCE is thrown on any other place then it is correctly rethrown

This PR fixes two other bugs, which are now visible

  1. RtHelper#getMethod must ignore synthetic methods, otherwise incorrect acceptable type is detected. For example NamedElementFilter#matches has
    A) synthetic NamedElementFilter#matches(CtElement)
    B) public NamedElementFilter#matches(CtNamedElement)

  2. APITest#testSetterInNodes thrown unexpected ClassCastException, which was silently ignored by old code. @surli please review my fix in APITest - you are the author of this test.

This PR is finished and ready for merge from my point of view.

@surli
Copy link
Collaborator

surli commented Sep 26, 2017

It seems ok to me. @tdurieux ?

@tdurieux
Copy link
Collaborator

To be honest, I don't understand completely the impact of this change on CtQuery.
I will trust the tests.

@surli surli merged commit bbedd5c into INRIA:master Sep 29, 2017
@surli
Copy link
Collaborator

surli commented Sep 29, 2017

Thanks @pvojtechovsky

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.

3 participants