scala / bug

Scala 2 bug reports only. Please, no questions — proper bug reports only.
https://scala-lang.org
232 stars 21 forks source link

Use scala.util.DynamicVariable instead of ad hoc implementations #7918

Closed scabug closed 10 years ago

scabug commented 11 years ago

There are a few instances of ad hoc implementations of dynamic variables in the Scala codebase. I suggest these are replaced with use of scala.util.DynamicVariable. We should eat our own dog food drink our own champagne.

scabug commented 11 years ago

Imported From: https://issues.scala-lang.org/browse/SI-7918?orig=1 Reporter: @qerub Assignee: @qerub

scabug commented 11 years ago

@retronym said: Are you planning on submitting a patch? If not, could you please list the places.

scabug commented 11 years ago

@qerub said: Yes, pull request is available here: https://github.com/scala/scala/pull/3058

(Sorry, should have been more clear.)

scabug commented 10 years ago

@paulp said: Reproducing the comment I left there, here:

These examples in the repl don't make any difference, but there are dozens more of these in the compiler and I am sure you don't want to continue this substitution. That ThreadLocal is not a desirable change, and this will be way slower, which you won't have any trouble seeing it if you go after any remotely hot spot where those 3-4 lines of boilerplate exist.

When marked @inline, these calls are compiled ideally - no thunks, no thread locals, state managed on the stack. This is a local optimum, all directions away lead down. You might be able to reduce the boilerplate with a macro. That is the only change I can see making sense.

Here's a partial list of additional sites, which I offer to show how far this PR comes from eliminating these, and not as encouragement to change others. These are only the ones which matched regexp finally.*\= saved (which means I was most likely the author, or at least the tweaker) but it's not by any means complete.

src/compiler/scala/tools/nsc/ast/parser/Parsers.scala:229:      finally smartParsing = saved
src/compiler/scala/tools/nsc/ast/parser/Parsers.scala:311:      finally classContextBounds = saved
src/compiler/scala/tools/nsc/ast/parser/Parsers.scala:497:      finally inFunReturnType = saved
src/compiler/scala/tools/nsc/backend/icode/GenICode.scala:1775:      finally ctx.cleanups = saved
src/compiler/scala/tools/nsc/reporters/Reporter.scala:42:    finally _truncationOK = saved
src/compiler/scala/tools/nsc/reporters/Reporter.scala:51:    finally incompleteHandler = saved
src/compiler/scala/tools/nsc/symtab/classfile/Pickler.scala:388:        try body finally refs = saved
src/compiler/scala/tools/nsc/transform/ExplicitOuter.scala:293:      finally outerParam = savedOuterParam
src/compiler/scala/tools/nsc/transform/Mixin.scala:1249:      finally localTyper = saved
src/compiler/scala/tools/nsc/transform/TailCalls.scala:193:      finally this.ctx = saved
src/compiler/scala/tools/nsc/transform/TailCalls.scala:396:      finally maybeTail = saved
src/compiler/scala/tools/nsc/transform/UnCurry.scala:393:        finally needTryLift = saved
src/compiler/scala/tools/nsc/transform/UnCurry.scala:412:        finally this.inConstructorFlag = saved
src/compiler/scala/tools/nsc/typechecker/Contexts.scala:189:      try a finally enclClass = saved
src/compiler/scala/tools/nsc/typechecker/Contexts.scala:314:        finally undetparams = saved
src/compiler/scala/tools/nsc/typechecker/Contexts.scala:373:      finally contextMode = saved
src/compiler/scala/tools/nsc/typechecker/RefChecks.scala:118:      try body finally inPattern = saved
src/compiler/scala/tools/nsc/typechecker/SuperAccessors.scala:363:      finally validCurrentOwner = saved
src/compiler/scala/tools/nsc/typechecker/TypeDiagnostics.scala:63:    finally isTyperInPattern = saved
src/reflect/scala/reflect/internal/ExistentialsAndSkolems.scala:40:    finally skolemizationLevel = saved
src/reflect/scala/reflect/internal/Variances.scala:30:      try body finally inRefinement = saved
src/reflect/scala/reflect/internal/Variances.scala:131:        finally this.base = saved
src/reflect/scala/reflect/internal/pickling/UnPickler.scala:79:      try body finally readIndex = saved
src/reflect/scala/reflect/internal/tpe/TypeMaps.scala:198:      try body finally variance = saved
src/reflect/scala/reflect/internal/tpe/TypeMaps.scala:634:        finally if (wroteAnnotation) giveup() else wroteAnnotation = saved
src/repl/scala/tools/nsc/interpreter/ILoop.scala:92:    finally replayCommandStack = saved
src/repl/scala/tools/nsc/interpreter/ILoop.scala:97:    finally in = saved
src/repl/scala/tools/nsc/interpreter/IMain.scala:202:    finally printResults = saved
src/repl/scala/tools/nsc/interpreter/IMain.scala:208:    finally totalSilence = saved
src/repl/scala/tools/nsc/interpreter/IMain.scala:1137:    finally isettings.unwrapStrings = saved
src/repl/scala/tools/nsc/interpreter/session/FileBackedHistory.scala:28:    finally isPersistent = saved
scabug commented 10 years ago

@retronym said: Closing following the PR discussion.