Performance improvements in completions #206

Merged
merged 2 commits into from Oct 15, 2012

Conversation

Projects
None yet
4 participants
@dragos
Member

dragos commented Oct 8, 2012

A couple of performance tweaks:

  • don't wait for the JDT indexer to finish when adding completions from the classpath
  • checking for duplicates is no longer quadratic (when the number of types on the classpath is large this can be felt)
  • don't show classes with $ in their names.

dragos added some commits Oct 6, 2012

Filter out types containing `$` from type completions.
Don't wait for the JDT indexer when searching for types on the class path. 
This should make completions a bit more responsive. Refs #1001020.

Fixed #1001264.
Removed quadratic search for duplicates in the types added from class…
… path.

Switched from ListBuffer to MultiMap, it is consistently faster in my tests. For large class paths this can make a big difference.
@scala-jenkins

This comment has been minimized.

Show comment
Hide comment
@scala-jenkins

This comment has been minimized.

Show comment
Hide comment
@dragos

This comment has been minimized.

Show comment
Hide comment
@dragos

dragos Oct 8, 2012

Member

PLS REBUILD pr-validator-master-trunk

Member

dragos commented Oct 8, 2012

PLS REBUILD pr-validator-master-trunk

@dotta

This comment has been minimized.

Show comment
Hide comment
@dotta

dotta Oct 10, 2012

Member

Is the failure expected?

Member

dotta commented Oct 10, 2012

Is the failure expected?

@dragos

This comment has been minimized.

Show comment
Hide comment
@dragos

dragos Oct 10, 2012

Member

No, but given when it ran, I'm not surprised it failed :) Let's wait for another build kitten

Member

dragos commented Oct 10, 2012

No, but given when it ran, I'm not surprised it failed :) Let's wait for another build kitten

@dragos

This comment has been minimized.

Show comment
Hide comment
@dragos

dragos Oct 10, 2012

Member

Not much love from the build bot these days...

Member

dragos commented Oct 10, 2012

Not much love from the build bot these days...

@scala-jenkins

This comment has been minimized.

Show comment
Hide comment
@scala-jenkins

This comment has been minimized.

Show comment
Hide comment
@@ -134,11 +135,12 @@ class ScalaCompletions extends HasLogger {
IJavaSearchConstants.TYPE,
SearchEngine.createJavaSearchScope(Array[IJavaElement](scu.scalaProject.javaProject), true),
requestor,
- IJavaSearchConstants.WAIT_UNTIL_READY_TO_SEARCH, // wait until all types are indexed by the JDT
+ IJavaSearchConstants.FORCE_IMMEDIATE_SEARCH, // don't wait until all types are indexed by the JDT

This comment has been minimized.

@dotta

dotta Oct 11, 2012

Member

I'm wondering if the performance gain this is worth the loss of precision. Did you test this in some big file, e.g., Typer.scala, and noticed better responsiveness?

@dotta

dotta Oct 11, 2012

Member

I'm wondering if the performance gain this is worth the loss of precision. Did you test this in some big file, e.g., Typer.scala, and noticed better responsiveness?

This comment has been minimized.

@dragos

dragos Oct 11, 2012

Member

I tried it on large files, but the biggest gains came from removing quadratic behavior. It's hard to test this, since I don't know when is the indexer up to date, and when not.

However, it seems to me that I would never want to wait for the indexer when asking for completions. Note that in this case we'd be waiting for types that are not imported, but happen to be on the classpath.

@dragos

dragos Oct 11, 2012

Member

I tried it on large files, but the biggest gains came from removing quadratic behavior. It's hard to test this, since I don't know when is the indexer up to date, and when not.

However, it seems to me that I would never want to wait for the indexer when asking for completions. Note that in this case we'd be waiting for types that are not imported, but happen to be on the classpath.

@dotta

This comment has been minimized.

Show comment
Hide comment
@dotta

dotta Oct 11, 2012

Member

don't show classes with $ in their names.

Really good idea!

Member

dotta commented Oct 11, 2012

don't show classes with $ in their names.

Really good idea!

@dragos

This comment has been minimized.

Show comment
Hide comment
@dragos

dragos Oct 12, 2012

Member

PLS REBUILD ALL

Member

dragos commented Oct 12, 2012

PLS REBUILD ALL

@scala-jenkins

This comment has been minimized.

Show comment
Hide comment
@scala-jenkins

This comment has been minimized.

Show comment
Hide comment
@scala-jenkins

This comment has been minimized.

Show comment
Hide comment
@scala-jenkins

This comment has been minimized.

Show comment
Hide comment
@dragos

This comment has been minimized.

Show comment
Hide comment
@dragos

dragos Oct 15, 2012

Member

Ping! :)

Member

dragos commented Oct 15, 2012

Ping! :)

@dotta

This comment has been minimized.

Show comment
Hide comment
@dotta

dotta Oct 15, 2012

Member

LOL, It's all good from my side! :)

Member

dotta commented Oct 15, 2012

LOL, It's all good from my side! :)

dragos added a commit that referenced this pull request Oct 15, 2012

Merge pull request #206 from dragos/issue/no-dollars-in-scope-complet…
…ions-1001264

Performance improvements in completions

@dragos dragos merged commit dbcf6f8 into scala-ide:master Oct 15, 2012

@hubertp

This comment has been minimized.

Show comment
Hide comment
@hubertp

hubertp Oct 15, 2012

Member

Not sure if there is some performance penalty but I thought I would chime in and do some code review :)
How about use packageNameArray.length and make packageName a def?

Not sure if there is some performance penalty but I thought I would chime in and do some code review :)
How about use packageNameArray.length and make packageName a def?

This comment has been minimized.

Show comment
Hide comment
@dragos

dragos Oct 15, 2012

Member

Could help, you're right. But I don't understand why making packageName a def is faster?

Member

dragos replied Oct 15, 2012

Could help, you're right. But I don't understand why making packageName a def is faster?

This comment has been minimized.

Show comment
Hide comment
@hubertp

hubertp Oct 15, 2012

Member

Now I see that packageName is used later in the diff as well - I initially saw it only used in the if condition. You can ignore my comment then.

Member

hubertp replied Oct 15, 2012

Now I see that packageName is used later in the diff as well - I initially saw it only used in the if condition. You can ignore my comment then.

@hubertp

This comment has been minimized.

Show comment
Hide comment
@hubertp

hubertp Oct 15, 2012

Member

I would imagine that there is a lot of dollar signs used in the codebase and everytime we hard-code it we increase the chance of code-duplication. Shouldn't tests like these be refactored to some util object or sth?

This is just a friendly code review, you are free to ignore it ;) Thanks for fixing this annoying bug.

I would imagine that there is a lot of dollar signs used in the codebase and everytime we hard-code it we increase the chance of code-duplication. Shouldn't tests like these be refactored to some util object or sth?

This is just a friendly code review, you are free to ignore it ;) Thanks for fixing this annoying bug.

This comment has been minimized.

Show comment
Hide comment
@dragos

dragos Oct 15, 2012

Member

Sorry Hubert, I don't know why, but your comments didn't appear in the pull request discussion. I already merged the pull request, but I can make some changes later. It's a good observation.

Member

dragos replied Oct 15, 2012

Sorry Hubert, I don't know why, but your comments didn't appear in the pull request discussion. I already merged the pull request, but I can make some changes later. It's a good observation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment