Skip to content

Commit

Permalink
Eliminate isError inside synchronized block
Browse files Browse the repository at this point in the history
Avoids slowdown of Daffodil test suite.

DAFFODIL-2965
  • Loading branch information
mbeckerle committed Dec 23, 2024
1 parent 38ad1bd commit 69e3876
Show file tree
Hide file tree
Showing 3 changed files with 29 additions and 11 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ final class ProcessorFactory private (
codeGenerator
}

override def isError: Boolean = sset.isError
override lazy val isError: Boolean = sset.isError

def withDistinguishedRootNode(name: String, namespace: String): ProcessorFactory = {
Assert.usage(name ne null)
Expand Down Expand Up @@ -378,8 +378,13 @@ class Compiler private (
tunables
)
}
pf.isError // call here to drive compilation, so that compilation at least at the level of the PF
// is over when we return the pf.
// It is tempting to call pf.isError here to drive compilation to completion before we
// return the pf to the caller.
// However, this slows down TDML-based testing in Daffodil substantially by moving
// the entire isError checking pass inside the synchronized block of the Daffodil
// schema compiler. This results in reduced concurrency which substantially slows
// the daffodil test suite.
// pf.isError // don't call this here. Call it outside the synchronized block.
pf
}

Expand Down Expand Up @@ -410,9 +415,16 @@ object Compiler {
optRootName: Option[String],
optRootNamespace: Option[String]
): ProcessorFactory = {
synchronized {
val pf = synchronized {
c.compileSourceInternal(schemaSource, optRootName, optRootNamespace)
}
// Force all compilation to complete. Called here outside of synchronized block on purpose
// to avoid over-serializing things (which would slow down large test suites like Daffodil's test suite.)
// Note that this requires that the shared data structures which require Daffodil schema compilation to
// be serialized do *not* include the data structures being modified during isError processing (which is
// lots of OOLAG evaluations).
pf.isError
pf
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -683,7 +683,7 @@ final class SchemaSet private (
* and finally the AST objects are checked for errors, which recursively
* demands that all other aspects of compilation occur.
*/
override def isError: Boolean = {
override lazy val isError: Boolean = {
if (!isValid) true
else if (
// use keepGoing so we can capture errors and
Expand All @@ -694,6 +694,15 @@ final class SchemaSet private (
}
) true
else {
// we must check for errors here via the super method, as that iterates over
// all the objects evaluating them for any errors in their required evaluations.
// This has to be after everything else that could report an error here (on some
// other object) has been done.
//
// That is to say, if we called super.isError at the top of this method that would
// be incorrect since isValid and areComponentsConstructed above might cause errors
// to be recorded or objects created that this call to super.isError would then not
// take into account.
val hasErrors = super.isError
if (!hasErrors) {
// must be called after compilation is done
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -478,16 +478,13 @@ object OOLAG {
}

/**
* Currently we depend on being able to evaluate these
* repeatedly, and get different answers.
*
* because it forces evaluation of all the requiredEvaluationsAlways(...)
* Forces evaluation of all the requiredEvaluationsAlways(...)
* or requiredEvaluationsIfActivated(...)
* on all objects first, but that is only for the objects
* that have been created and activated at the time this is called.
*/

def isError: Boolean = {
def isError: Boolean = isErrorOnce
private lazy val isErrorOnce: Boolean = {
oolagRoot.checkErrors
val errorCount = oolagRoot.errors.size
errorCount > 0
Expand Down

0 comments on commit 69e3876

Please sign in to comment.