You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@daffodil.apache.org by GitBox <gi...@apache.org> on 2022/01/31 14:49:47 UTC
[GitHub] [daffodil] stevedlawrence opened a new pull request #739: Replace WeakHashMap TDML cache with a custom cache we have more control over
stevedlawrence opened a new pull request #739:
URL: https://github.com/apache/daffodil/pull/739
Commit 4cc0863e71 modified the TDML compile result cache to use a
WeakHashMap. While this worked in some cases, it relied on the garbage
collector not being too aggressive. If the garbage collector
aggressively cleans up our WeakHashMap entries, or something like an IDE
frequently causes the GC to be run in between tests, it becomes
difficult to share compiled results between different DFDL Test Suites.
This leads to excessive schema compilation.
To fix this, this modifies to global TDML cache to be a normal HashMap,
with expiration times stored with each compile result entry. When a DFDL
Test suite finishes, it marks the compile results it uses with an
expiration time. Entries are removed form this cache when the expire
time is reached.
To minimize the size of this global cache, we also only store
non-embedded schemas in it. Embedded schemas cannot be shared between
different TDML files/DFDL Test suites, so putting them in the global
cache is unnecessary. Instead, compile results for embedded schemas are
stored in a cache local to the DFDL Test Suite. They will be garbage
collected when the DFDL Test Suite is garbage collected.
DAFFODIL-2627
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #739: Replace WeakHashMap TDML cache with a custom cache we have more control over
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #739:
URL: https://github.com/apache/daffodil/pull/739#discussion_r796826670
##########
File path: daffodil-tdml-lib/src/main/scala/org/apache/daffodil/tdml/TDMLRunner.scala
##########
@@ -461,7 +461,20 @@ class DFDLTestSuite private[tdml] (
}
/**
- * Get the CompileResult from from the TDMLCompileResultCache.
+ * Get the CompileResult from a TDMLCompileResultCache.
+ *
+ * This uses one of two different caches to look for compile results:
+ *
+ * Embedded schemas cannot be shared among different DFDL Test Suites, so
+ * they are stored in a cache local to this test suite. This avoids having to
+ * run the expiration logic for such compile results, and the compile results
+ * will be garbage collected when this DFDL Test Suite is garbage collected.
Review comment:
That's just a bug, we really need to be cleaning up tmp files created in tests. I'll open a ticket.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] mbeckerle commented on a change in pull request #739: Replace WeakHashMap TDML cache with a custom cache we have more control over
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #739:
URL: https://github.com/apache/daffodil/pull/739#discussion_r795807359
##########
File path: daffodil-tdml-lib/src/main/scala/org/apache/daffodil/tdml/TDMLRunner.scala
##########
@@ -2585,64 +2613,85 @@ object UTF8Encoder {
}
-object TDMLCompileResultCache {
+case class TDMLCompileResultCacheKey(
+ impl: String,
+ suppliedSchema: DaffodilSchemaSource,
+ useSerializedProcessor: Boolean,
+ optRootName: Option[String],
+ optRootNamespace: Option[String],
+ tunables: Map[String, String],
+)
- case class Key (
- impl: String,
- suppliedSchema: DaffodilSchemaSource,
- useSerializedProcessor: Boolean,
- optRootName: Option[String],
- optRootNamespace: Option[String],
- tunables: Map[String, String],
- )
+/**
+ * Stores the compile result as well as the time when this compile result
+ * should be removed from the cache. If optExpireTime is set to None, it means
+ * a DFDLTestSuite exists that is using this compile result, and so it should
+ * not expire. If optExpireTime is set to Some, it means no DFDLTestSuite
+ * currently uses the compile result, and so it can be expired once we reach
+ * the current time reaches the Some value time
Review comment:
This is a good algorithm. I think just add this detail about the re-extending of the timeout to comments.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence merged pull request #739: Replace WeakHashMap TDML cache with a custom cache we have more control over
Posted by GitBox <gi...@apache.org>.
stevedlawrence merged pull request #739:
URL: https://github.com/apache/daffodil/pull/739
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #739: Replace WeakHashMap TDML cache with a custom cache we have more control over
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #739:
URL: https://github.com/apache/daffodil/pull/739#discussion_r795803630
##########
File path: daffodil-tdml-lib/src/main/scala/org/apache/daffodil/tdml/TDMLRunner.scala
##########
@@ -2585,64 +2613,85 @@ object UTF8Encoder {
}
-object TDMLCompileResultCache {
+case class TDMLCompileResultCacheKey(
+ impl: String,
+ suppliedSchema: DaffodilSchemaSource,
+ useSerializedProcessor: Boolean,
+ optRootName: Option[String],
+ optRootNamespace: Option[String],
+ tunables: Map[String, String],
+)
- case class Key (
- impl: String,
- suppliedSchema: DaffodilSchemaSource,
- useSerializedProcessor: Boolean,
- optRootName: Option[String],
- optRootNamespace: Option[String],
- tunables: Map[String, String],
- )
+/**
+ * Stores the compile result as well as the time when this compile result
+ * should be removed from the cache. If optExpireTime is set to None, it means
+ * a DFDLTestSuite exists that is using this compile result, and so it should
+ * not expire. If optExpireTime is set to Some, it means no DFDLTestSuite
+ * currently uses the compile result, and so it can be expired once we reach
+ * the current time reaches the Some value time
Review comment:
> I didn't see logic for this
`removeExpiredEntries` has the logic for for removing entries that have `Some` expire time and have reached that expire time. It keeps entries that do not have an expire time via the `getOrElse(true)`
`setEntriesFinished` has the logic for setting the expire time for keys that are finished (which is `now` + the expiration duration).
> I think also a successful cache hit should re-extend the time limit
We sort of do that. At line 2676, when we have a cache hit we set `v.optExpireTime = None`. So when we have a cache hit we disable expire time. Once the DFDLTestSuite finishes that had that cache hit, it will call `setEntriesFinished` and add a new expire time. There's a comment there, but maybe it isn't good enough?
So the duration isn't actually from the last time there's a cache hit. It's from the latest time a DFDLTestSuite that used that compile result "finished". This way compile results will exist X seconds after the DFDLTestSuite ended, which should hopefully be enough for another test suite to start and start using it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #739: Replace WeakHashMap TDML cache with a custom cache we have more control over
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #739:
URL: https://github.com/apache/daffodil/pull/739#discussion_r796830221
##########
File path: daffodil-tdml-lib/src/main/scala/org/apache/daffodil/tdml/TDMLRunner.scala
##########
@@ -2585,64 +2613,107 @@ object UTF8Encoder {
}
-object TDMLCompileResultCache {
+case class TDMLCompileResultCacheKey(
+ impl: String,
+ suppliedSchema: DaffodilSchemaSource,
+ useSerializedProcessor: Boolean,
+ optRootName: Option[String],
+ optRootNamespace: Option[String],
+ tunables: Map[String, String],
+)
- case class Key (
- impl: String,
- suppliedSchema: DaffodilSchemaSource,
- useSerializedProcessor: Boolean,
- optRootName: Option[String],
- optRootNamespace: Option[String],
- tunables: Map[String, String],
- )
+/**
+ * Stores the compile result as well as the time when this compile result
+ * should be removed from the cache.
+ *
+ * If optExpireTimeMillis is set to None, it means a DFDLTestSuite exists that
+ * is using this compile result, and so it will not expire.
+ *
+ * If optExpireTimeMillis is set to Some, it means no DFDLTestSuite currently
+ * uses the compile result, and so it can be expired once we reach the current
+ * time reaches the Some value time
Review comment:
Agreed, will fix
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] mbeckerle commented on a change in pull request #739: Replace WeakHashMap TDML cache with a custom cache we have more control over
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #739:
URL: https://github.com/apache/daffodil/pull/739#discussion_r795775944
##########
File path: daffodil-tdml-lib/src/main/scala/org/apache/daffodil/tdml/TDMLRunner.scala
##########
@@ -2585,64 +2613,85 @@ object UTF8Encoder {
}
-object TDMLCompileResultCache {
+case class TDMLCompileResultCacheKey(
+ impl: String,
+ suppliedSchema: DaffodilSchemaSource,
+ useSerializedProcessor: Boolean,
+ optRootName: Option[String],
+ optRootNamespace: Option[String],
+ tunables: Map[String, String],
+)
- case class Key (
- impl: String,
- suppliedSchema: DaffodilSchemaSource,
- useSerializedProcessor: Boolean,
- optRootName: Option[String],
- optRootNamespace: Option[String],
- tunables: Map[String, String],
- )
+/**
+ * Stores the compile result as well as the time when this compile result
+ * should be removed from the cache. If optExpireTime is set to None, it means
+ * a DFDLTestSuite exists that is using this compile result, and so it should
+ * not expire. If optExpireTime is set to Some, it means no DFDLTestSuite
+ * currently uses the compile result, and so it can be expired once we reach
+ * the current time reaches the Some value time
+ */
+case class TDMLCompileResultCacheValue(
+ compileResult: TDML.CompileResult,
+ var optExpireTime: Option[Long] = None,
+)
- private val cache = new mutable.WeakHashMap[Key, TDML.CompileResult]()
+case class TDMLCompileResultCache(entryExpireDuration: Option[Long]) {
+
+ private val cache = new mutable.HashMap[TDMLCompileResultCacheKey, TDMLCompileResultCacheValue]()
+
+ private def removeExpiredEntries(): Unit = {
+ val now = System.currentTimeMillis()
+ cache.retain { case (_, v) =>
+ val retainEntry = v.optExpireTime.map { now < _ }.getOrElse(true)
+ retainEntry
+ }
+ }
+
+ def setEntriesFinished(keys: mutable.Set[TDMLCompileResultCacheKey]): Unit = this.synchronized {
+ val now = System.currentTimeMillis()
+ val expireTime = Some(now + (entryExpireDuration.get * 1000))
+ keys.foreach { key =>
+ val optVal = cache.get(key)
+ if (optVal.isDefined) {
+ optVal.get.optExpireTime = expireTime
+ }
+ }
+ }
- /**
- * Get a compile result. This might come from a cache of compiled results, or
- * it might compile a new processor if it hasn't been cached or has been
- * removed from the cache.
- *
- * The CompileResult is guaranteed to not be evicted from cache as long as
- * there exists a strong reference to the returned key. For this reason, it
- * is important that callers must maintain a strong reference to the key as
- * long as they want the CompileResult to stay in the cache.
- */
def getCompileResult(
impl: AbstractTDMLDFDLProcessorFactory,
- suppliedSchema: DaffodilSchemaSource,
- useSerializedProcessor: Boolean,
- optRootName: Option[String],
- optRootNamespace: Option[String],
- tunables: Map[String, String]): (Key, TDML.CompileResult) = this.synchronized {
+ key: TDMLCompileResultCacheKey): TDML.CompileResult = this.synchronized {
- val tmpKey = Key(
- impl.implementationName,
- suppliedSchema,
- useSerializedProcessor,
- optRootName,
- optRootNamespace,
- tunables)
+ if (entryExpireDuration.isDefined) {
+ removeExpiredEntries()
+ }
- val maybeKeyVal = cache.find { case (k, v) => k == tmpKey }
- if (maybeKeyVal.isDefined) {
- // the key is already in the cache, throw away our temp key and return
- // the actual key and value that is in the WeakHashMap
- maybeKeyVal.get
+ val optVal = cache.get(key)
+ if (optVal.isDefined) {
+ val v = optVal.get
+ // if some other DFDL Test Suite set an expire time for this compile
+ // result, but another test suite is now reusing it before reaching the
+ // expire time, we reset the expire time so that the entry isn't removed
+ // until the new test suite using it is finished.
+ v.optExpireTime = None
+ v.compileResult
} else {
- // the key either was never added to the cache, or it was garbage
- // collected from the WeakHashMap. Get the processor, add it to the
- // cache, and return the key/value
- val processor = impl.getProcessor(
- suppliedSchema,
- useSerializedProcessor,
- optRootName,
- optRootNamespace,
- tunables)
- val kv = (tmpKey, processor)
- cache += kv
- kv
+ val compileResult = impl.getProcessor(
+ key.suppliedSchema,
+ key.useSerializedProcessor,
+ key.optRootName,
+ key.optRootNamespace,
+ key.tunables)
+ cache += (key -> TDMLCompileResultCacheValue(compileResult, None))
+ compileResult
}
}
}
+
+object GlobalTDMLCompileResultCache {
+ val expireTime = 30
Review comment:
Add units of measure to name. expireTimeSeconds
##########
File path: daffodil-tdml-lib/src/main/scala/org/apache/daffodil/tdml/TDMLRunner.scala
##########
@@ -2585,64 +2613,85 @@ object UTF8Encoder {
}
-object TDMLCompileResultCache {
+case class TDMLCompileResultCacheKey(
+ impl: String,
+ suppliedSchema: DaffodilSchemaSource,
+ useSerializedProcessor: Boolean,
+ optRootName: Option[String],
+ optRootNamespace: Option[String],
+ tunables: Map[String, String],
+)
- case class Key (
- impl: String,
- suppliedSchema: DaffodilSchemaSource,
- useSerializedProcessor: Boolean,
- optRootName: Option[String],
- optRootNamespace: Option[String],
- tunables: Map[String, String],
- )
+/**
+ * Stores the compile result as well as the time when this compile result
+ * should be removed from the cache. If optExpireTime is set to None, it means
+ * a DFDLTestSuite exists that is using this compile result, and so it should
+ * not expire. If optExpireTime is set to Some, it means no DFDLTestSuite
+ * currently uses the compile result, and so it can be expired once we reach
+ * the current time reaches the Some value time
Review comment:
I didn't see logic for this. I think also a successful cache hit should re-extend the time limit so instead of 30 seconds after the first compilation the schema needs to be recompiled it would instead be 30 seconds after the last usage of the compiled schema.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] tuxji commented on a change in pull request #739: Replace WeakHashMap TDML cache with a custom cache we have more control over
Posted by GitBox <gi...@apache.org>.
tuxji commented on a change in pull request #739:
URL: https://github.com/apache/daffodil/pull/739#discussion_r796814936
##########
File path: daffodil-tdml-lib/src/main/scala/org/apache/daffodil/tdml/TDMLRunner.scala
##########
@@ -2585,64 +2613,107 @@ object UTF8Encoder {
}
-object TDMLCompileResultCache {
+case class TDMLCompileResultCacheKey(
+ impl: String,
+ suppliedSchema: DaffodilSchemaSource,
+ useSerializedProcessor: Boolean,
+ optRootName: Option[String],
+ optRootNamespace: Option[String],
+ tunables: Map[String, String],
+)
- case class Key (
- impl: String,
- suppliedSchema: DaffodilSchemaSource,
- useSerializedProcessor: Boolean,
- optRootName: Option[String],
- optRootNamespace: Option[String],
- tunables: Map[String, String],
- )
+/**
+ * Stores the compile result as well as the time when this compile result
+ * should be removed from the cache.
+ *
+ * If optExpireTimeMillis is set to None, it means a DFDLTestSuite exists that
+ * is using this compile result, and so it will not expire.
+ *
+ * If optExpireTimeMillis is set to Some, it means no DFDLTestSuite currently
+ * uses the compile result, and so it can be expired once we reach the current
+ * time reaches the Some value time
Review comment:
Comment has some unnecessary words, would read better as:
```
* If optExpireTimeMillis is set to Some, it means no DFDLTestSuite currently
* uses the compile result, and it can be expired once the current time reaches
* the Some value time
```
##########
File path: daffodil-tdml-lib/src/main/scala/org/apache/daffodil/tdml/TDMLRunner.scala
##########
@@ -461,7 +461,20 @@ class DFDLTestSuite private[tdml] (
}
/**
- * Get the CompileResult from from the TDMLCompileResultCache.
+ * Get the CompileResult from a TDMLCompileResultCache.
+ *
+ * This uses one of two different caches to look for compile results:
+ *
+ * Embedded schemas cannot be shared among different DFDL Test Suites, so
+ * they are stored in a cache local to this test suite. This avoids having to
+ * run the expiration logic for such compile results, and the compile results
+ * will be garbage collected when this DFDL Test Suite is garbage collected.
Review comment:
Note that embedded schemas also are written to files in /tmp which should be deleted after the tests finish, but currently are not cleaned up.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #739: Replace WeakHashMap TDML cache with a custom cache we have more control over
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #739:
URL: https://github.com/apache/daffodil/pull/739#discussion_r795804526
##########
File path: daffodil-tdml-lib/src/main/scala/org/apache/daffodil/tdml/TDMLRunner.scala
##########
@@ -2585,64 +2613,85 @@ object UTF8Encoder {
}
-object TDMLCompileResultCache {
+case class TDMLCompileResultCacheKey(
+ impl: String,
+ suppliedSchema: DaffodilSchemaSource,
+ useSerializedProcessor: Boolean,
+ optRootName: Option[String],
+ optRootNamespace: Option[String],
+ tunables: Map[String, String],
+)
- case class Key (
- impl: String,
- suppliedSchema: DaffodilSchemaSource,
- useSerializedProcessor: Boolean,
- optRootName: Option[String],
- optRootNamespace: Option[String],
- tunables: Map[String, String],
- )
+/**
+ * Stores the compile result as well as the time when this compile result
+ * should be removed from the cache. If optExpireTime is set to None, it means
+ * a DFDLTestSuite exists that is using this compile result, and so it should
+ * not expire. If optExpireTime is set to Some, it means no DFDLTestSuite
+ * currently uses the compile result, and so it can be expired once we reach
+ * the current time reaches the Some value time
+ */
+case class TDMLCompileResultCacheValue(
+ compileResult: TDML.CompileResult,
+ var optExpireTime: Option[Long] = None,
+)
- private val cache = new mutable.WeakHashMap[Key, TDML.CompileResult]()
+case class TDMLCompileResultCache(entryExpireDuration: Option[Long]) {
+
+ private val cache = new mutable.HashMap[TDMLCompileResultCacheKey, TDMLCompileResultCacheValue]()
+
+ private def removeExpiredEntries(): Unit = {
+ val now = System.currentTimeMillis()
+ cache.retain { case (_, v) =>
+ val retainEntry = v.optExpireTime.map { now < _ }.getOrElse(true)
+ retainEntry
+ }
+ }
+
+ def setEntriesFinished(keys: mutable.Set[TDMLCompileResultCacheKey]): Unit = this.synchronized {
+ val now = System.currentTimeMillis()
+ val expireTime = Some(now + (entryExpireDuration.get * 1000))
+ keys.foreach { key =>
+ val optVal = cache.get(key)
+ if (optVal.isDefined) {
+ optVal.get.optExpireTime = expireTime
+ }
+ }
+ }
- /**
- * Get a compile result. This might come from a cache of compiled results, or
- * it might compile a new processor if it hasn't been cached or has been
- * removed from the cache.
- *
- * The CompileResult is guaranteed to not be evicted from cache as long as
- * there exists a strong reference to the returned key. For this reason, it
- * is important that callers must maintain a strong reference to the key as
- * long as they want the CompileResult to stay in the cache.
- */
def getCompileResult(
impl: AbstractTDMLDFDLProcessorFactory,
- suppliedSchema: DaffodilSchemaSource,
- useSerializedProcessor: Boolean,
- optRootName: Option[String],
- optRootNamespace: Option[String],
- tunables: Map[String, String]): (Key, TDML.CompileResult) = this.synchronized {
+ key: TDMLCompileResultCacheKey): TDML.CompileResult = this.synchronized {
- val tmpKey = Key(
- impl.implementationName,
- suppliedSchema,
- useSerializedProcessor,
- optRootName,
- optRootNamespace,
- tunables)
+ if (entryExpireDuration.isDefined) {
+ removeExpiredEntries()
+ }
- val maybeKeyVal = cache.find { case (k, v) => k == tmpKey }
- if (maybeKeyVal.isDefined) {
- // the key is already in the cache, throw away our temp key and return
- // the actual key and value that is in the WeakHashMap
- maybeKeyVal.get
+ val optVal = cache.get(key)
+ if (optVal.isDefined) {
+ val v = optVal.get
+ // if some other DFDL Test Suite set an expire time for this compile
+ // result, but another test suite is now reusing it before reaching the
+ // expire time, we reset the expire time so that the entry isn't removed
+ // until the new test suite using it is finished.
+ v.optExpireTime = None
+ v.compileResult
} else {
- // the key either was never added to the cache, or it was garbage
- // collected from the WeakHashMap. Get the processor, add it to the
- // cache, and return the key/value
- val processor = impl.getProcessor(
- suppliedSchema,
- useSerializedProcessor,
- optRootName,
- optRootNamespace,
- tunables)
- val kv = (tmpKey, processor)
- cache += kv
- kv
+ val compileResult = impl.getProcessor(
+ key.suppliedSchema,
+ key.useSerializedProcessor,
+ key.optRootName,
+ key.optRootNamespace,
+ key.tunables)
+ cache += (key -> TDMLCompileResultCacheValue(compileResult, None))
+ compileResult
}
}
}
+
+object GlobalTDMLCompileResultCache {
+ val expireTime = 30
Review comment:
Fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org