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/21 20:11:03 UTC
[GitHub] [daffodil] stevedlawrence opened a new pull request #732: Add a global TDML CompileResult cache
stevedlawrence opened a new pull request #732:
URL: https://github.com/apache/daffodil/pull/732
We previosly only cache CompileResults on individual DFDLTestSuites.
This meant that we could compiled schemas within a single test suite,
but not between tests suites. This could lead to lots of duplicate
schema compilation if multiple .tdml files referenced the same schema
To avoid this, CompileResults are now stored in a global WeakHashMap
instead of the DFDLTestSuite. And DFDLTestSuites are changed to maintain
strong references to the keys of the CompileResult they use. This
ensures that as long as a DFDLTestSuite exists, so will all of its
CompileResults, so tests within a DFDLTestSuite can share them. And
after the DFDLTestSuite is garbage collected, as long as those weak
references aren't also garbage collected, other DFDLTestSuites will be
able to use them, and help to minimize compilation same schemas used in
different TDML files.
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 merged pull request #732: Add a global TDML CompileResult cache
Posted by GitBox <gi...@apache.org>.
stevedlawrence merged pull request #732:
URL: https://github.com/apache/daffodil/pull/732
--
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 #732: Add a global TDML CompileResult cache
Posted by GitBox <gi...@apache.org>.
tuxji commented on a change in pull request #732:
URL: https://github.com/apache/daffodil/pull/732#discussion_r790017012
##########
File path: daffodil-tdml-processor/src/main/scala/org/apache/daffodil/tdml/processor/Runtime2TDMLDFDLProcessor.scala
##########
@@ -113,6 +113,11 @@ final class Runtime2TDMLDFDLProcessorFactory private(
// Compile the generated code into an executable
val executable = generator.compileCode(codeDir)
+ // There is is no way to clean up TDMLDFDLProcessors, they are just garbage
+ // collected when no longer needed. So recursively mark all generated
+ // files as deleteOnExit so we at least clean them up when the JVM exits
+ os.walk.stream(tempDir).foreach { _.toIO.deleteOnExit() }
Review comment:
Have you tested that this line actually cleans up all temporary files used by runtime2 tests? If not, I'll still approve but I'll also check for left-over temporary files during my next work on runtime2 just in case.
In general, Daffodil tests aren't very good about removing temporary files. I always see a lot of Daffodil-created temporary files in /tmp after running "sbt test" and/or "sbt IntegrationTest/test".
--
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 pull request #732: Add a global TDML CompileResult cache
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on pull request #732:
URL: https://github.com/apache/daffodil/pull/732#issuecomment-1020129679
Sounds good. I'll merge, we can reopen and reinvestigate if we find there are still some issues.
--
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] jadams-tresys commented on pull request #732: Add a global TDML CompileResult cache
Posted by GitBox <gi...@apache.org>.
jadams-tresys commented on pull request #732:
URL: https://github.com/apache/daffodil/pull/732#issuecomment-1020126477
Yes, this fixes the performance issues I was seeing. Haven't run enough tests to make sure that it is back to the same levels before the changes to the TDML cache, but it seems pretty close.
--
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 pull request #732: Add a global TDML CompileResult cache
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on pull request #732:
URL: https://github.com/apache/daffodil/pull/732#issuecomment-1020033716
@jadams-tresys, can you test this to make sure this fixes the performance issues you are seeing?
--
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 #732: Add a global TDML CompileResult cache
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #732:
URL: https://github.com/apache/daffodil/pull/732#discussion_r790901572
##########
File path: daffodil-tdml-processor/src/main/scala/org/apache/daffodil/tdml/processor/Runtime2TDMLDFDLProcessor.scala
##########
@@ -113,6 +113,11 @@ final class Runtime2TDMLDFDLProcessorFactory private(
// Compile the generated code into an executable
val executable = generator.compileCode(codeDir)
+ // There is is no way to clean up TDMLDFDLProcessors, they are just garbage
+ // collected when no longer needed. So recursively mark all generated
+ // files as deleteOnExit so we at least clean them up when the JVM exits
+ os.walk.stream(tempDir).foreach { _.toIO.deleteOnExit() }
Review comment:
We need a JIRA ticket for this.
Honestly, the exact right idiom for creating and cleaning up temp files is not obvious to me. E.g., the fact that you can't just call deleteOnExit is news.
We should put a temp-files section in our coding-style-guide on wiki.
--
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 #732: Add a global TDML CompileResult cache
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #732:
URL: https://github.com/apache/daffodil/pull/732#discussion_r790042489
##########
File path: daffodil-tdml-processor/src/main/scala/org/apache/daffodil/tdml/processor/Runtime2TDMLDFDLProcessor.scala
##########
@@ -113,6 +113,11 @@ final class Runtime2TDMLDFDLProcessorFactory private(
// Compile the generated code into an executable
val executable = generator.compileCode(codeDir)
+ // There is is no way to clean up TDMLDFDLProcessors, they are just garbage
+ // collected when no longer needed. So recursively mark all generated
+ // files as deleteOnExit so we at least clean them up when the JVM exits
+ os.walk.stream(tempDir).foreach { _.toIO.deleteOnExit() }
Review comment:
> Have you tested that this line actually cleans up all temporary files used by runtime2 tests? If not, I'll still approve but I'll also check for left-over temporary files during my next work on runtime2 just in case.
Yep, I did actually test and verify that it works. Note that it's not enough to have delteOnExit() on just the root directory, it's needs to be on the directory and everything inside it.
> In general, Daffodil tests aren't very good about removing temporary files. I always see a lot of Daffodil-created temporary files in /tmp after running "sbt test" and/or "sbt IntegrationTest/test".
Yeah, we're really bad about this. I too have noticed how much we pollute /tmp. We really need to clean everything up, or at least put everything in /tmp/daffodil/ or something so everything is in one place.
--
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