[GitHub] [daffodil] stevedlawrence commented on a change in pull request #739: Replace WeakHashMap TDML cache with a custom cache we have more control over

[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