krzema12 / kotlin-python

Python target for the Kotlin Programming Language. See https://github.com/krzema12/kotlin-python/tree/python-backend/python
https://discuss.kotlinlang.org/t/idea-python-backend/19852
48 stars 1 forks source link

OOM when generating box plot after syncing with Kotlin 1.6.0 #50

Closed krzema12 closed 2 years ago

krzema12 commented 2 years ago

https://kotlinlang.slack.com/archives/C0289CS37AS/p1637332034038000?thread_ts=1637332034.038000&cid=C0289CS37AS

https://github.com/krzema12/kotlin-python/commit/42ac4763cea56584784c4140abdc6c44e1658edf#diff-e745cd83095cccd974c16374e666a5617b2f4588fc0ea93807a0db575eac6263

Ideas:

krzema12 commented 2 years ago

@SerVB since it doesn't reproduce on my machine, I need two hints from your side:

I have a plan to convert it to sequences which will be an improvement regardless if it fixes the issue, but first I want to gather some baseline data and compare it with my environment.

krzema12 commented 2 years ago

@SerVB Please try with such changes:

git diff python/experiments/generate-box-tests-reports.main.kts
diff --git a/python/experiments/generate-box-tests-reports.main.kts b/python/experiments/generate-box-tests-reports.main.kts
index f2d563b7fae..e31ececd2c7 100755
--- a/python/experiments/generate-box-tests-reports.main.kts
+++ b/python/experiments/generate-box-tests-reports.main.kts
@@ -43,7 +43,7 @@ val testResults = getTestResults(Paths.get("python/box.tests/build/test-results/
 testResults.writeFailedTestsSummary(targetFailedTestsReportPath)
 testResults.writeSummaryTsvToFile(targetBoxTestsReportPath)
 testResults.writeFailureCount(failureCountReportPath)
-//generateGitHistoryPlot(gitHistoryPlotPath)  // todo: resolve OOM
+generateGitHistoryPlot(gitHistoryPlotPath)  // todo: resolve OOM

 fun List<TestResult>.writeFailedTestsSummary(targetFile: Path) = targetFile.toFile().printWriter().use { out ->
     this
@@ -241,6 +241,7 @@ fun generateGitHistoryPlot(gitHistoryPlotPath: Path) {
     val git = Git.open(File("."))

     val data = git.log().call()
+        .asSequence()
         .filter { it.authorIdent.emailAddress in kotlinPythonAuthors }
         .filter { it.name !in commitsContainingIncorrectData }
         .sortedBy { it.commitTime }
@@ -269,6 +270,7 @@ fun generateGitHistoryPlot(gitHistoryPlotPath: Path) {
                 null
             }
         }
+        .toList()

     val dataForPlot = mapOf(
         "date" to data.map { it.date } + data.map { it.date },

On my machine they don't change much - both memory usage plot and running time are almost identical: image

SerVB commented 2 years ago

Thanks, will do on Friday!

SerVB commented 2 years ago

a screenshot of memory usage graph from your OS, including time before and after running the script

Not much to screenshot, there are no visible spikes... Before running the script, I have 10.1/31.3 GB used, after running and crashing the maximum is 10.5/31.3...

After applying your patch with sequences, not much is changed for me: same crash :(

if you have any JVM heap size overrides, and if not or you're unaware, would be cool to check heap settings from within the script

I've just added the following lines to the script:

println("""
    Current heap size: ${"%.1f".format(Runtime.getRuntime().totalMemory().toDouble() / 1024 / 1024)} MB
    Max heap size: ${"%.1f".format(Runtime.getRuntime().maxMemory().toDouble() / 1024 / 1024)} MB
    Free heap size: ${"%.1f".format(Runtime.getRuntime().freeMemory().toDouble() / 1024 / 1024)} MB
""".trimIndent())

And turns out I have quite small heap:

Current heap size: 169.5 MB
Max heap size: 228.0 MB
Free heap size: 83.1 MB

Searching how I can increase it.

krzema12 commented 2 years ago

To make stuff funnier, I have similar values and it works fine:

Current heap size: 74.0 MB
Max heap size: 256.0 MB
Free heap size: 34.6 MB

It's probably not the root cause :thinking:

SerVB commented 2 years ago

Kotlin Team suggested me that simple JAVA_OPTS="-Xmx1G" python/experiments/generate-box-tests-reports.main.kts should work to set up the heap size. This solves the problem for me, now there is no crash!

krzema12 commented 2 years ago

Cool! I'd just add it to our README and we're done, right? :)

SerVB commented 2 years ago

Yeah, let's add the suggested script in the readme and uncomment that line, then this issue can be closed