SI-6240 synchronization for runtime reflection #2083

Merged
merged 14 commits into from Feb 27, 2013

Projects

None yet

6 participants

@xeno-by
The Scala Programming Language member

Unlike its predecessors, this pull request is for realz!

I have several stress tests for reflection, and I also have successfully run parallel tests in a couple of projects, whose authors reported crashes when using runtime reflection.

review @paulp @adriaanm @retronym

@xeno-by
The Scala Programming Language member

The most controversial part of this PR is e2efc0c. Take a good look at it.

@xeno-by
The Scala Programming Language member

Reminds me of ScalaDays 2012 and desperate attempts to get reflection working by building a blob of boilerplate. @paulp is probably going to hate me again, for the very same reason.

@xeno-by
The Scala Programming Language member

PLS REBUILD ALL?

@xeno-by
The Scala Programming Language member

Jenkins shows no active jobs. Kitty, where are you?

@adriaanm
The Scala Programming Language member

I'll reboot it asap.

@paulp

"Reminds me of ScalaDays 2012 and desperate attempts to get reflection working by building a blob of boilerplate. @paulp is probably going to hate me again, for the very same reason."

In that case, I don't think I have to even look. DNLGTM.

@xeno-by
The Scala Programming Language member

@paulp Maybe you could still take a look and give an improvement advice?

@paulp

@xeno-by soon as I can. I wasn't really not going to look.

@xeno-by
The Scala Programming Language member

@paulp My sarcasm detector has broken. Sorry :)

@paulp

@xeno-by it's my fault because I like for people not to be entirely sure whether or not I'm kidding.

@xeno-by
The Scala Programming Language member

BUILDLOG?

@xeno-by
The Scala Programming Language member

Oh, look - all green! Now it's only a matter of an lgtm or two to get this into 2.10.x :)

@scala-jenkins

(kitty-note-to-self: ignore 13286500)
pr-rangepos-per-commit:

pr-checkin-per-commit:

@adriaanm
The Scala Programming Language member
@scala-jenkins

(kitty-note-to-self: ignore 13240015)
🐱 Roger! Rebuilding pr-rangepos-per-commit, pr-checkin-per-commit for 39bbb95, 11e6200, 6adaaf5, aeb5a43, e4d57ca, e13c2f1, e2efc0c, da98009, 3a1475e, 365321b, a57495c, b527bdd, d1d5ad7, 8f72a6d. 🚨

@scala-jenkins

(kitty-note-to-self: ignore 13302268)
🐱 Roger! Rebuilding pr-rangepos-per-commit for d1d5ad7. 🚨

@scala-jenkins scala-jenkins referenced this pull request Feb 8, 2013
@xeno-by xeno-by initializes lazy vals and inner objects in advance
As discussed at http://groups.google.com/group/scala-internals/browse_thread/thread/97840ba4fd37b52e,
`synchronized(this)` employed by lazy val and inner object initialization
is an excellent way to deadlock yourself in the foot.

Imagine a thread, which grabs a reflection GIL and then calls one of those
lazy vals / objects that reflection exposes (e.g. a companion module of
an innocently looking SingleType case class). Then imagine another thread,
which calls something else in SymbolTable, grabbing symbol table's monitor,
and then tries to get a reflection GIL to do something non-trivial. Hello,
we've just arrived at a deadlock.

Since, as discussed in the aforementioned thread, there's no easy way to
change lazy vals / inner objects in reflection to use GIL instead of
synchronizing on this, I bit the bullet and manually initialized all
things with deferred initialization defined in reflect.runtime.SymbolTable.

The list of all things `$lzycompute` has been mined by a simple Python script,
then I copy/pasted that list into `JavaUniverse.scala` and went ahead forcing
objects and lazy vals mentioned there. Notably, I've been able to force all
lazy vals in Definitions.scala.

There are some todos left, but I suggest we move forward without securing them,
because the 2.10.1-RC1 release date is very close, so we'd better have a 95%
solution instead of keeping reflection thread-unsafe. Though here's the list of
todo lazy vals for the reference:

  * BaseTypeSeq.maxDepth
  * WeakTypeTag.tpe
  * AnnotationInfo.forcedInfo

For each of those lazy vals we need to make sure that their initializers never
call back into themselves. Otherwise, there's a danger of a deadlock.
e2efc0c
@scala-jenkins scala-jenkins referenced this pull request Feb 8, 2013
@xeno-by xeno-by synchronizes names
Previously we didn't have all possible name creation facilities covered
with locks, so some of them silently misbehaved and caused much grief:
http://groups.google.com/group/scala-internals/browse_thread/thread/ec1d3e2c4bcb000a.

This patch gets all the name factories under control. Unfortunately it
comes at a performance cost, which has to be evaluated.
a57495c
@scala-jenkins scala-jenkins referenced this pull request Feb 8, 2013
@xeno-by xeno-by synchronizes pendingVolatiles
Called from isVolatile, which is called from isStable, which is a part
of the public reflection API.
365321b
@scala-jenkins scala-jenkins referenced this pull request Feb 8, 2013
@xeno-by xeno-by optimizes Scala reflection GIL
First of all, GIL should only apply to runtime reflection, because noone
is going to run toolboxes in multiple threads: a) that's impossible, b/c
the compiler isn't thread safe, b) ToolBox api prevents that.

Secondly, the only things in symbols which require synchronization are:
1) info/validTo (completers aren't thread-safe),
2) rawInfo and its dependencies (it shares a mutable field with info)
3) non-trivial caches like in typeAsMemberOfLock

If you think about it, other things like sourceModule or associatedFile
don't need synchronization, because they are either set up when a symbol
is created or cloned or when it's completed. The former is obviously safe,
while the latter is safe as well, because before acquiring init-dependent
state of symbols, the compiler calls `initialize`, which is synchronized.

We can say that symbols can be in four possible states: 1) being created,
2) created, but not yet initialized, 3) initializing, 4) initialized.
in runtime reflection can undergo is init. #3 is dangerous and needs protection
3a1475e
@scala-jenkins scala-jenkins referenced this pull request Feb 8, 2013
@xeno-by xeno-by removes the assertion in missingHook
In the current synchronization scheme multiple threads can enter the
missingHook trying to materialize a package, which hasn't been created.

That's fine, because makeScalaPackage, which creates and enters package
symbols is synchronized and checks whether the creation is necessary
before commencing. Therefore even if makeScalaPackage is called multiple
times in rapid succession, the calls will be serialized and all calls
except the first one won't do anything.
8f72a6d
@scala-jenkins scala-jenkins referenced this pull request Feb 8, 2013
@xeno-by xeno-by SI-7045 reflection now auto-initializes selfType
selfType joins the happy family of flags, annotations and privateWithin,
which automatically trigger initialization, when used within runtime
reflection.
b527bdd
@scala-jenkins scala-jenkins referenced this pull request Feb 8, 2013
@xeno-by xeno-by synchronizes toolboxes
We don't guarantee thread-safety of the front end, but everything else
looks good now.
da98009
@xeno-by
The Scala Programming Language member

@adriaanm Why was that a rebuild all?

@JamesIry

Moving to 2.10.2-RC1 unless somebody can convince that it's both needed and safe for 2.10.1

@xeno-by
The Scala Programming Language member

There weren't too much excited reviews, so I guess we should indeed omit this PR from 2.10.1's feature set.

@retronym
The Scala Programming Language member

The patch looks okay to me. It is a pity not to book some progress on this, as the real world usage of the result would help to flush out residual problems. But I agree the timing of the release makes this all difficult.

@JamesIry

LGTM from @retronym, but some per commit tests are failing. We've branched so when this PR is ready it can be merged to 2.10.x which now targets 2.10.2-RC1.

@xeno-by
The Scala Programming Language member

@adriaanm There's a bunch of spurious failures. How do I best deal with them?

@adriaanm
The Scala Programming Language member

I'm not sure. Are you sure they are spurious? The "results" link takes you straight to the jenkins output and for the ones I clicked on, it looks like it doesn't compile because pendingSuperCall was removed.

@xeno-by
The Scala Programming Language member

Oh. I'm very sorry. Indeed, the failures are genuine.

xeno-by added some commits Feb 7, 2013
@xeno-by xeno-by moves Symbol#SymbolKind to Symbols
Too bad I didn't notice that before. That will free up quite a bit of
memory, removing an extraneous field in every single Symbol, namely the:

private volatile Symbols.Symbol.SymbolKind$ SymbolKind$module
21d5d38
@xeno-by xeno-by cleans up initialization of runtime reflection
At first I just tried to remove syntheticCoreClasses from missingHook
and put them into the initializer of freshly created mirrors in order to
reduce the non-determinism in mutations of the global symbol table.

And then it didn't work, crashing on me claiming that AnyRef is missing.
Apparently we still need AnyRefClass in missingHook, just because it's
impossible to initialize (i.e. unpickle) ScalaPackageClass without it.

And then it still didn't work, whining about multiple overloaded defs
of some synthetic symbols. That was really tricky, but I figured it out
as well by initializing ScalaPackageClass first before forcing any
synthetic symbols (see the details in comments).
981da8e
@xeno-by xeno-by introduces GIL to Scala reflection
On a serious note, I feel really uncomfortable about having to juggle
this slew of locks. Despite that I can't immediately find a deadlock,
I'm 100% sure there is one hiding in the shadows. Hence, I'm abandoning
all runtime reflection locks in favor of a single per-universe one.
5b37cfb
@xeno-by xeno-by reflection no longer uses atPhase and friends
Mentioned methods mutate the global `atPhaseStack` variable, which can
easily lead to imbalances and, ultimately, to the empty stack error.

Luckily for us, there's only one dummy phase, SomePhase, which is used
by runtime reflection, so there is absolutely zero need to invoke atPhase
in non-compiler reflexive universes.

The cleanest solution would be to override `atPhase` for runtime reflection,
but it's @inline final, so I didn't want to pay performance penalties for
something that's used three times in runtime reflection (during unpickling, in
reflection-specific completers and in `Symbol.typeParams/unsafeTypeParams`).

Therefore I added overrideable analogues of `atPhase` and `atPhaseNotLaterThan`
which are called from the aforementioned code shared between the compiler and
runtime reflection. I also had to duplicate the code of `Symbol.XXXtypeParams`,
again due to them being very performance-sensitive.
b2c2493
@xeno-by xeno-by synchronizes symbols
Synchronization via decoration would be neat if it actually worked.
Unfortunately, root symbols never got decorated, therefore their children
also never got decorated and all the way down to the very turtles.

This commit fixes this sad issue by turning root symbols from objects
to lazy vals. Yes, this is going to induce a performance penalty, which
will hopefully not be high enough to invalidate this cornerstone of our
synchronization strategy.

Now when root symbols are lazy vals, they can be overridden in the runtime
reflexive universe and decorated with SynchronizedSymbol, which makes their
children sync and sound.
a9dca51
@xeno-by xeno-by removes the crazy extraneous log 0262941
@xeno-by xeno-by optimizes Scala reflection GIL
First of all, GIL should only apply to runtime reflection, because noone
is going to run toolboxes in multiple threads: a) that's impossible, b/c
the compiler isn't thread safe, b) ToolBox api prevents that.

Secondly, the only things in symbols which require synchronization are:
1) info/validTo (completers aren't thread-safe),
2) rawInfo and its dependencies (it shares a mutable field with info)
3) non-trivial caches like in typeAsMemberOfLock

If you think about it, other things like sourceModule or associatedFile
don't need synchronization, because they are either set up when a symbol
is created or cloned or when it's completed. The former is obviously safe,
while the latter is safe as well, because before acquiring init-dependent
state of symbols, the compiler calls `initialize`, which is synchronized.

We can say that symbols can be in four possible states: 1) being created,
2) created, but not yet initialized, 3) initializing, 4) initialized.
in runtime reflection can undergo is init. #3 is dangerous and needs protection
bebd62d
@xeno-by xeno-by initializes lazy vals and inner objects in advance
As discussed at http://groups.google.com/group/scala-internals/browse_thread/thread/97840ba4fd37b52e,
`synchronized(this)` employed by lazy val and inner object initialization
is an excellent way to deadlock yourself in the foot.

Imagine a thread, which grabs a reflection GIL and then calls one of those
lazy vals / objects that reflection exposes (e.g. a companion module of
an innocently looking SingleType case class). Then imagine another thread,
which calls something else in SymbolTable, grabbing symbol table's monitor,
and then tries to get a reflection GIL to do something non-trivial. Hello,
we've just arrived at a deadlock.

Since, as discussed in the aforementioned thread, there's no easy way to
change lazy vals / inner objects in reflection to use GIL instead of
synchronizing on this, I bit the bullet and manually initialized all
things with deferred initialization defined in reflect.runtime.SymbolTable.

The list of all things `$lzycompute` has been mined by a simple Python script,
then I copy/pasted that list into `JavaUniverse.scala` and went ahead forcing
objects and lazy vals mentioned there. Notably, I've been able to force all
lazy vals in Definitions.scala.

There are some todos left, but I suggest we move forward without securing them,
because the 2.10.1-RC1 release date is very close, so we'd better have a 95%
solution instead of keeping reflection thread-unsafe. Though here's the list of
todo lazy vals for the reference:

  * BaseTypeSeq.maxDepth
  * WeakTypeTag.tpe
  * AnnotationInfo.forcedInfo

For each of those lazy vals we need to make sure that their initializers never
call back into themselves. Otherwise, there's a danger of a deadlock.
735634f
@xeno-by xeno-by synchronizes toolboxes
We don't guarantee thread-safety of the front end, but everything else
looks good now.
4cbb935
@xeno-by xeno-by runtime reflection: death from thousand threads
not anymore
387b259
@xeno-by xeno-by removes the assertion in missingHook
In the current synchronization scheme multiple threads can enter the
missingHook trying to materialize a package, which hasn't been created.

That's fine, because makeScalaPackage, which creates and enters package
symbols is synchronized and checks whether the creation is necessary
before commencing. Therefore even if makeScalaPackage is called multiple
times in rapid succession, the calls will be serialized and all calls
except the first one won't do anything.
73d079f
@xeno-by xeno-by synchronizes pendingVolatiles
Called from isVolatile, which is called from isStable, which is a part
of the public reflection API.
dd148de
@xeno-by xeno-by synchronizes names
Previously we didn't have all possible name creation facilities covered
with locks, so some of them silently misbehaved and caused much grief:
http://groups.google.com/group/scala-internals/browse_thread/thread/ec1d3e2c4bcb000a.

This patch gets all the name factories under control. Unfortunately it
comes at a performance cost, which has to be evaluated.
f4dd56c
@xeno-by xeno-by SI-7045 reflection now auto-initializes selfType
selfType joins the happy family of flags, annotations and privateWithin,
which automatically trigger initialization, when used within runtime
reflection.
07bcb61
@xeno-by
The Scala Programming Language member

Rebased against the new 2.10.x. PLS REBUILD ALL

@scala-jenkins

(kitty-note-to-self: ignore 13394690)
🐱 Roger! Rebuilding pr-rangepos-per-commit, pr-checkin-per-commit for 21d5d38, 981da8e, 5b37cfb, b2c2493, a9dca51, 0262941, bebd62d, 735634f, 4cbb935, 387b259, 73d079f, dd148de, f4dd56c, 07bcb61. 🚨

@adriaanm
The Scala Programming Language member

sorry i was just changing the jenkins scripts as described on scala-internals
I'll make sure it gets built.

@adriaanm
The Scala Programming Language member

PLS REBUILD pr-rangepos-per-commit

@scala-jenkins

(kitty-note-to-self: ignore 13395906)
🐱 Roger! Rebuilding pr-rangepos-per-commit for 21d5d38, 981da8e, 5b37cfb, b2c2493, a9dca51, 0262941, bebd62d, 735634f, 4cbb935, 387b259, 73d079f, dd148de, f4dd56c, 07bcb61. 🚨

@scala-jenkins

Job pr-rangepos-per-commit failed for 0262941 (results):


Took 21 s.
sad kitty
to rebuild, comment "PLS REBUILD/pr-rangepos-per-commit@0262941"on PR #2083

@scala-jenkins

Job pr-rangepos-per-commit failed for a9dca51 (results):


Took 49 s.
sad kitty
to rebuild, comment "PLS REBUILD/pr-rangepos-per-commit@a9dca51"on PR #2083

@scala-jenkins

Job pr-rangepos-per-commit failed for dd148de (results):


Took 12 s.
sad kitty
to rebuild, comment "PLS REBUILD/pr-rangepos-per-commit@dd148de"on PR #2083

@scala-jenkins

Job pr-rangepos-per-commit failed for 4cbb935 (results):


Took 39 s.
sad kitty
to rebuild, comment "PLS REBUILD/pr-rangepos-per-commit@4cbb935"on PR #2083

@scala-jenkins

Job pr-rangepos-per-commit failed for 07bcb61 (results):


Took 10 s.
sad kitty
to rebuild, comment "PLS REBUILD/pr-rangepos-per-commit@07bcb61"on PR #2083

@scala-jenkins

Job pr-rangepos-per-commit failed for b2c2493 (results):


Took 23 s.
sad kitty
to rebuild, comment "PLS REBUILD/pr-rangepos-per-commit@b2c2493"on PR #2083

@scala-jenkins

Job pr-rangepos-per-commit failed for 5b37cfb (results):


Took 11 s.
sad kitty
to rebuild, comment "PLS REBUILD/pr-rangepos-per-commit@5b37cfb"on PR #2083

@scala-jenkins

Job pr-rangepos-per-commit failed for f4dd56c (results):


Took 9 s.
sad kitty
to rebuild, comment "PLS REBUILD/pr-rangepos-per-commit@f4dd56c"on PR #2083

@scala-jenkins

Job pr-rangepos-per-commit failed for 981da8e (results):


Took 13 s.
sad kitty
to rebuild, comment "PLS REBUILD/pr-rangepos-per-commit@981da8e"on PR #2083

@scala-jenkins

Job pr-rangepos-per-commit failed for 73d079f (results):


Took 29 s.
sad kitty
to rebuild, comment "PLS REBUILD/pr-rangepos-per-commit@73d079f"on PR #2083

@scala-jenkins

Job pr-rangepos-per-commit failed for 735634f (results):


Took 15 s.
sad kitty
to rebuild, comment "PLS REBUILD/pr-rangepos-per-commit@735634f"on PR #2083

@scala-jenkins

Job pr-rangepos-per-commit failed for 387b259 (results):


Took 27 s.
sad kitty
to rebuild, comment "PLS REBUILD/pr-rangepos-per-commit@387b259"on PR #2083

@scala-jenkins

Job pr-rangepos-per-commit failed for bebd62d (results):


Took 21 s.
sad kitty
to rebuild, comment "PLS REBUILD/pr-rangepos-per-commit@bebd62d"on PR #2083

@scala-jenkins

Job pr-rangepos-per-commit failed for 21d5d38 (results):


Took 15 s.
sad kitty
to rebuild, comment "PLS REBUILD/pr-rangepos-per-commit@21d5d38"on PR #2083

@JamesIry

PLS REBUILD ALL

@retronym retronym was assigned Feb 21, 2013
@scala-jenkins

(kitty-note-to-self: ignore 13907017)
🐱 Roger! Rebuilding pr-rangepos-per-commit, pr-checkin-per-commit for 21d5d38, 981da8e, 5b37cfb, b2c2493, a9dca51, 0262941, bebd62d, 735634f, 4cbb935, 387b259, 73d079f, dd148de, f4dd56c, 07bcb61. 🚨

@scala-jenkins

(kitty-note-to-self: ignore 13911379)
🐱 Roger! Rebuilding pr-rangepos-per-commit for 387b259. 🚨

@JamesIry

PLS SYNCH

@scala-jenkins

(kitty-note-to-self: ignore 13918751)
🐱 Synchronaising! 🙏

@adriaanm adriaanm was assigned Feb 23, 2013
@JamesIry

ping @adriaanm I think we should merge this puppy and see what it breaks.

@adriaanm adriaanm merged commit b98cc58 into scala:2.10.x Feb 27, 2013

1 check passed

Details default pr-rangepos-per-commit Took 69 min.
@adriaanm adriaanm added a commit to adriaanm/scala that referenced this pull request Mar 1, 2013
@adriaanm adriaanm Revert SI-6240 synchronization for runtime reflection
This commit reverts #2083:
  - 387b259 runtime reflection: death from thousand threads
  - 73d079f removes the assertion in missingHook
  - f4dd56c synchronizes names
  - dd148de synchronizes pendingVolatiles
  - 4cbb935 synchronizes toolboxes
  - 07bcb61 SI-7045 reflection now auto-initializes selfType
  - bebd62d optimizes Scala reflection GIL
  - 735634f initializes lazy vals and inner objects in advance
  - 5b37cfb introduces GIL to Scala reflection
  - 981da8e cleans up initialization of runtime reflection
  - b2c2493 reflection no longer uses atPhase and friends
  - a9dca51 synchronizes symbols
  - 0262941 removes the crazy extraneous log
  - 21d5d38 moves Symbol#SymbolKind to Symbols
0420b2d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment