Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 321383

Summary: [performance] ProblembindingChecker.isInClassContext is very slow
Product: [Tools] CDT Reporter: Marc-André Laperle <malaperle>
Component: cdt-codanAssignee: Elena Laskavaia <elaskavaia.cdt>
Status: RESOLVED FIXED QA Contact: Elena Laskavaia <elaskavaia.cdt>
Severity: normal    
Priority: P3 CC: cdtdoug
Version: 8.0   
Target Milestone: 8.0   
Hardware: All   
OS: All   
Whiteboard:
Attachments:
Description Flags
proposed fix elaskavaia.cdt: iplog+

Description Marc-André Laperle CLA 2010-07-30 13:04:49 EDT 
CDT 7.1.0.201007271806 (nightly build)

Steps to reproduce:

1. Open a non trivial file (1K lines+) with the ProblemBinding checker activated
2. Modify the code, save.
3. Notice the indexing job taking a long time. The call stack reveals that the checker is spending most of its time in isInClassContext.

The ckecker now takes minutes to complete. I also experienced bug 220585 many times because of this.

I don't think there is a need to get the actual ast node of the class in this case. We only need to need if it's a class (or struct?), so we could check if the binding is an instance of ICompositeType and return a bool.
Comment 1 Elena Laskavaia CLA 2010-07-30 14:18:05 EDT 
Not sure how you propose to fix it. Is patch coming?
Comment 2 Elena Laskavaia CLA 2010-07-30 20:03:58 EDT 
I checked the performance. It is unacceptable for run as type checkers.
On my machine all checkers runs 2-3 milliseconds each. This one runs
3 min. If I remove most of isInClassContext method it gets down to 152 ms
which is still too much... If we cannot improve it we cannot leave it on.
Comment 3 Tomasz Wesolowski CLA 2010-07-31 07:32:38 EDT 
Created attachment 175646 [details]
proposed fix

I agree, this can be done on index simpler. I was just starting to explore the wonders of PDOM back then.
This patch should do the trick, but please have a look there.

BTW- there's a discouraged access warning there; thats a simple function, you can copy/paste the contents to clear the warning, but I really think that Codan should have access to org.eclipse.internal.core.dom. Eclipse infractructure allows that, doesn't it?
Comment 4 Marc-André Laperle CLA 2010-07-31 16:02:50 EDT 
(In reply to comment #3)
> This patch should do the trick, but please have a look there.

Looks good to me.

> BTW- there's a discouraged access warning there; thats a simple function, you
> can copy/paste the contents to clear the warning, but I really think that Codan
> should have access to org.eclipse.internal.core.dom. Eclipse infractructure
> allows that, doesn't it?

I think you'd add cdt.codan.checkers as a friend of cdt.core. I'm not sure it's worth it for a method this small.

(In reply to comment #2)
> I checked the performance. It is unacceptable for run as type checkers.
> On my machine all checkers runs 2-3 milliseconds each. This one runs
> 3 min. If I remove most of isInClassContext method it gets down to 152 ms
> which is still too much... If we cannot improve it we cannot leave it on.

I agree that performance could be improved and I will work on that. If we can't bring it down further then it could be off by default but still included. I used this checker for weeks on a very big project and the ~150 ms version was acceptable to me. I opened bug 321436 for this issue.
Comment 5 Elena Laskavaia CLA 2010-08-03 20:56:15 EDT 
fixed on head (8.0), thanks for the patch
Comment 6 CDT Genie CLA 2010-08-03 21:23:03 EDT 
*** cdt cvs genie on behalf of elaskavaia ***
Bug 321383 - fixed performance issue for ProblemBindingChecker

[*] ProblemBindingChecker.java 1.2 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/ProblemBindingChecker.java?root=Tools_Project&r1=1.1&r2=1.2
Comment 7 Marc-André Laperle CLA 2010-08-04 23:13:30 EDT 
(In reply to comment #5)
> fixed on head (8.0), thanks for the patch

I don't think the patch was correctly applied. Can you double-check? Thanks!
Comment 8 Elena Laskavaia CLA 2010-08-05 07:44:02 EDT 
yeah, apparently I did not apply the patch - I just committed the code I was playing with, thanks for noticing.