Closed qiemem closed 5 years ago
This isn't ready yet. The problem is that, when running with multiple BehaviorSpace thread, or with child models that use LevelSpace running in parallel, one model might complete loading and turn the generator back on before another model has checked if it should use the generator or not.
I see two possible solutions that don't involve modifying NetLogo:
Version
is a good candidate, since that's the object that is actually used to determine whether or not to use the classloader. This looks like: Version.synchronized {
try {
// See https://github.com/NetLogo/LevelSpace/issues/123
System.setProperty("org.nlogo.noGenerator", "true")
workspace.open(path)
} catch {
case e: IllegalStateException =>
throw new ExtensionException(
s"$path is from an incompatible version of NetLogo. Try opening it in NetLogo to convert it.", e
)
} finally {
System.setProperty("org.nlogo.noGenerator", "false")
}
}
This disadvantage of this approach is that only one model can load at a time, which is definitely a detriment for BehaviorSpace in particular.
try {
// See https://github.com/NetLogo/LevelSpace/issues/123
Version.synchronized {
System.setProperty("org.nlogo.ls.modelsLoading", (Integer.getInteger("org.nlogo.ls.modelsLoading") + 1).toString)
System.setProperty("org.nlogo.noGenerator", "true")
}
workspace.open(path)
} catch {
case e: IllegalStateException =>
throw new ExtensionException(
s"$path is from an incompatible version of NetLogo. Try opening it in NetLogo to convert it.", e
)
} finally {
Version.synchronized {
val modelsLeft = Integer.getInteger("org.nlogo.ls.modelsLoading") - 1
if (modelsLeft == 0) {
// Need to make sure to turn the generator back on
System.setProperty("org.nlogo.noGenerator", "false")
}
System.setProperty("org.nlogo.ls.modelsLoading", modelsLeft.toString)
}
}
Note that although all system property methods are synchronized, this requires an atomic read-and-update operation, which I don't believe exists for system properties. Thus, we still synchronize on Version
. Now, this is gross, but setting system properties takes significantly less time than loading models, and so there is substantially less contention.
This is clearly visible in the cpu usage of each method. This is a short BehaviorSpace run of the Sheep with Brains model using the synchronize method:
Most model loading takes place in the beginning, and the low cpu usage indicates clear contention.
This is the same BehaviorSpace experiment with the semaphore method:
The cpu usage looks much healthier (though I'm not sure why it drops later; maybe IO contention), and the runtime is about 45 seconds less. I'm not sure the improved runtime is worth the grosser code however.
I thought of another solution. We could re-enable the generator on extension unload
. The problem with this is a child model could re-enable the generator prematurely. To prevent that, we could have a child detect if it's a child by checking its parent classloader for the LevelSpace class.
It's not related, but I "fixed" it anyway. It looks like workspace.getModelPath
now returns an absolute path whereas it used to return the original path. I changed the error message to return the original path.
Fixes #123