From 7d4109486b2266f8491d3473f43555dec6e996ee Mon Sep 17 00:00:00 2001 From: François Garillot Date: Mon, 18 Nov 2013 15:29:08 +0100 Subject: SI-7982 Changed contract of askLoadedType to unload units by default The rationale for not keeping units loaded by default is that the more units are loaded, the slower is background compilation. For instance, in the Scala IDE for Eclipse (which uses the presentation compiler), typechecking occurs every time the reconciler kicks-in (~500millis after you stop typing), hence it is important that units are not kept loaded unless strictly necessary (for some extra information about this, see https://www.assembla.com/spaces/scala-ide/tickets/1001388) While I agree that using a boolean argument (`keepLoaded`) for deciding if a unit should be loaded isn't a great design, other methods in `CompilerControl` also have a keepLoaded parameter, so at least we have some consistency. For the future, I'm thinking we should be able to remove the `keepLoaded` flag altogether, and change the implementation of `askLoadedType` to preserve the same units loaded in the presentation compiler before and after its execution. Basically, if you want a unit to be kept loaded, you should call `askReload` first, and then `askLoadedType`. However, to reduce impact, I think the changes carried by this commit will help us estimate if the solution I just outlined is viable (because `askLoadeType` won't be keeping units loaded by default, which wasn't the case with the former implementation). (While the patch was mostly contributed by @huitseeker, @dotta has edited the commit message to preserve the comments in the PR https://github.com/scala/scala/pull/3209) --- .../tools/nsc/interactive/CompilerControl.scala | 23 +++++++++++------- .../scala/tools/nsc/interactive/Global.scala | 7 ++++-- .../scala/tools/nsc/interactive/Picklers.scala | 2 +- .../scala/tools/nsc/interactive/REPL.scala | 2 +- .../nsc/interactive/tests/core/AskCommand.scala | 4 ++-- test/files/presentation/ide-t1001388.check | 1 + test/files/presentation/ide-t1001388/Test.scala | 28 ++++++++++++++++++++++ test/files/presentation/ide-t1001388/src/a/A.scala | 6 +++++ 8 files changed, 58 insertions(+), 15 deletions(-) create mode 100644 test/files/presentation/ide-t1001388.check create mode 100644 test/files/presentation/ide-t1001388/Test.scala create mode 100644 test/files/presentation/ide-t1001388/src/a/A.scala diff --git a/src/compiler/scala/tools/nsc/interactive/CompilerControl.scala b/src/compiler/scala/tools/nsc/interactive/CompilerControl.scala index af82957a2e..8d12581c9c 100644 --- a/src/compiler/scala/tools/nsc/interactive/CompilerControl.scala +++ b/src/compiler/scala/tools/nsc/interactive/CompilerControl.scala @@ -202,15 +202,20 @@ trait CompilerControl { self: Global => postWorkItem(new AskToDoFirstItem(source)) /** If source is not yet loaded, loads it, and starts a new run, otherwise - * continues with current pass. - * Waits until source is fully type checked and returns body in response. - * @param source The source file that needs to be fully typed. - * @param response The response, which is set to the fully attributed tree of `source`. + * continues with current pass. + * Waits until source is fully type checked and returns body in response. + * @param source The source file that needs to be fully typed. + * @param keepLoaded Whether to keep that file in the PC if it was not loaded before. If + the file is already loaded, this flag is ignored. + * @param response The response, which is set to the fully attributed tree of `source`. * If the unit corresponding to `source` has been removed in the meantime * the a NoSuchUnitError is raised in the response. */ - def askLoadedTyped(source: SourceFile, response: Response[Tree]) = - postWorkItem(new AskLoadedTypedItem(source, response)) + def askLoadedTyped(source:SourceFile, keepLoaded: Boolean, response: Response[Tree]): Unit = + postWorkItem(new AskLoadedTypedItem(source, keepLoaded, response)) + + final def askLoadedTyped(source: SourceFile, response: Response[Tree]): Unit = + askLoadedTyped(source, false, response) /** If source if not yet loaded, get an outline view with askParseEntered. * If source is loaded, wait for it to be typechecked. @@ -219,7 +224,7 @@ trait CompilerControl { self: Global => */ def askStructure(keepSrcLoaded: Boolean)(source: SourceFile, response: Response[Tree]) = { getUnit(source) match { - case Some(_) => askLoadedTyped(source, response) + case Some(_) => askLoadedTyped(source, keepSrcLoaded, response) case None => askParsedEntered(source, keepSrcLoaded, response) } } @@ -403,8 +408,8 @@ trait CompilerControl { self: Global => response raise new MissingResponse } - case class AskLoadedTypedItem(val source: SourceFile, response: Response[Tree]) extends WorkItem { - def apply() = self.waitLoadedTyped(source, response, this.onCompilerThread) + case class AskLoadedTypedItem(val source: SourceFile, keepLoaded: Boolean, response: Response[Tree]) extends WorkItem { + def apply() = self.waitLoadedTyped(source, response, keepLoaded, this.onCompilerThread) override def toString = "wait loaded & typed "+source def raiseMissing() = diff --git a/src/compiler/scala/tools/nsc/interactive/Global.scala b/src/compiler/scala/tools/nsc/interactive/Global.scala index 49f6cb2373..686335dc6c 100644 --- a/src/compiler/scala/tools/nsc/interactive/Global.scala +++ b/src/compiler/scala/tools/nsc/interactive/Global.scala @@ -1065,7 +1065,7 @@ class Global(settings: Settings, _reporter: Reporter, projectName: String = "") } /** Implements CompilerControl.askLoadedTyped */ - private[interactive] def waitLoadedTyped(source: SourceFile, response: Response[Tree], onSameThread: Boolean = true) { + private[interactive] def waitLoadedTyped(source: SourceFile, response: Response[Tree], keepLoaded: Boolean = false, onSameThread: Boolean = true) { getUnit(source) match { case Some(unit) => if (unit.isUpToDate) { @@ -1083,7 +1083,10 @@ class Global(settings: Settings, _reporter: Reporter, projectName: String = "") case None => debugLog("load unit and type") try reloadSources(List(source)) - finally waitLoadedTyped(source, response, onSameThread) + finally { + waitLoadedTyped(source, response, onSameThread) + if (!keepLoaded) removeUnitOf(source) + } } } diff --git a/src/compiler/scala/tools/nsc/interactive/Picklers.scala b/src/compiler/scala/tools/nsc/interactive/Picklers.scala index 2b389158c3..64e050e799 100644 --- a/src/compiler/scala/tools/nsc/interactive/Picklers.scala +++ b/src/compiler/scala/tools/nsc/interactive/Picklers.scala @@ -172,7 +172,7 @@ trait Picklers { self: Global => implicit def askLoadedTypedItem: CondPickler[AskLoadedTypedItem] = pkl[SourceFile] - .wrapped { source => new AskLoadedTypedItem(source, new Response) } { _.source } + .wrapped { source => new AskLoadedTypedItem(source, false, new Response) } { _.source } .asClass (classOf[AskLoadedTypedItem]) implicit def askParsedEnteredItem: CondPickler[AskParsedEnteredItem] = diff --git a/src/compiler/scala/tools/nsc/interactive/REPL.scala b/src/compiler/scala/tools/nsc/interactive/REPL.scala index 7b89d5b0aa..4b64313e1b 100644 --- a/src/compiler/scala/tools/nsc/interactive/REPL.scala +++ b/src/compiler/scala/tools/nsc/interactive/REPL.scala @@ -170,7 +170,7 @@ object REPL { comp.askReload(List(toSourceFile(file)), reloadResult) Thread.sleep(millis.toInt) println("ask type now") - comp.askLoadedTyped(toSourceFile(file), typedResult) + comp.askLoadedTyped(toSourceFile(file), keepLoaded = true, typedResult) typedResult.get case List("typeat", file, off1, off2) => doTypeAt(makePos(file, off1, off2)) diff --git a/src/compiler/scala/tools/nsc/interactive/tests/core/AskCommand.scala b/src/compiler/scala/tools/nsc/interactive/tests/core/AskCommand.scala index 4f9df6808f..d5da52bc13 100644 --- a/src/compiler/scala/tools/nsc/interactive/tests/core/AskCommand.scala +++ b/src/compiler/scala/tools/nsc/interactive/tests/core/AskCommand.scala @@ -113,9 +113,9 @@ trait AskTypeAt extends AskCommand { trait AskLoadedTyped extends AskCommand { import compiler.Tree - protected def askLoadedTyped(source: SourceFile)(implicit reporter: Reporter): Response[Tree] = { + protected def askLoadedTyped(source: SourceFile, keepLoaded: Boolean = false)(implicit reporter: Reporter): Response[Tree] = { ask { - compiler.askLoadedTyped(source, _) + compiler.askLoadedTyped(source, keepLoaded, _) } } diff --git a/test/files/presentation/ide-t1001388.check b/test/files/presentation/ide-t1001388.check new file mode 100644 index 0000000000..d58f86d6c6 --- /dev/null +++ b/test/files/presentation/ide-t1001388.check @@ -0,0 +1 @@ +Test OK \ No newline at end of file diff --git a/test/files/presentation/ide-t1001388/Test.scala b/test/files/presentation/ide-t1001388/Test.scala new file mode 100644 index 0000000000..f6079cf0b2 --- /dev/null +++ b/test/files/presentation/ide-t1001388/Test.scala @@ -0,0 +1,28 @@ +import scala.tools.nsc.interactive.tests.InteractiveTest +import scala.reflect.internal.util.SourceFile +import scala.tools.nsc.interactive.Response + +object Test extends InteractiveTest { + override def execute(): Unit = { + val sourceA = loadSourceAndWaitUntilTypechecked("A.scala") + checkPresent(sourceA) + } + + private def loadSourceAndWaitUntilTypechecked(sourceName: String): SourceFile = { + val sourceFile = sourceFiles.find(_.file.name == sourceName).head + askLoadedTyped(sourceFile).get + /* The response to `askLoadedType` may return before `interactive.Global.waitLoadedType` + * fully executes. Because this test expects `waitLoadedType` is fully executed before + * calling `checkPresent`, with the below no-op presentation compiler request we make + * sure this requirement is fulfilled. + */ + compiler.askForResponse(() => ()).get + sourceFile + } + + private def checkPresent(source: SourceFile): Unit = compiler.getUnitOf(source) match { + case Some(unit) => reporter.println("Compilation Unit for " + source.file.name + " still loaded after askLoadedTyped") + + case None => reporter.println("Test OK") + } +} diff --git a/test/files/presentation/ide-t1001388/src/a/A.scala b/test/files/presentation/ide-t1001388/src/a/A.scala new file mode 100644 index 0000000000..be09097598 --- /dev/null +++ b/test/files/presentation/ide-t1001388/src/a/A.scala @@ -0,0 +1,6 @@ +package a + +object A { + val tagString = "foo" + Seq.empty[Byte].toArray.toSeq +} -- cgit v1.2.3