Improve runtime performance of the Go runtime test suite

[jimidle]

feat: Speed up the go runtime tests

Prior to this change, after writing the go.mod file for each test, we had to run a go mod tidy command. This is an extra process sparked for each test, which can be quite the overhead.

This change runs a go mod tidy only if we have not yet cached the results (go.mod and go.sum) for any prior test and therefore saves sparking a process for each test.

@parrt When you get a chance, could you change the Github test runtime environment to use go 1.20.1 ? I would like to keep our development and testing versions of the go compiler up to date, even if I decide not to require go 1.20 at this time. Cheers.

[jimidle]

Ok. I’ll take care of your requests tomorrow... After reviewing your requests, I cannot make the changes to do caching. The initialization sequence is all wrong and I do not wish to start changing the synchronization of this and risk introducing strange bugs.

…

On Wed, Mar 15, 2023 at 18:16 Ivan Kochurkin ***@***.***> wrote: ***@***.**** requested changes on this pull request. ------------------------------ In runtime-testsuite/test/org/antlr/v4/test/runtime/go/GoRunner.java <#4181 (comment)>: > +// List<GeneratedFile> generatedFiles = generatedState.generatedFiles; +// String tempDirPath = getTempDirPath(); +// File generatedParserDir = new File(tempDirPath, "parser"); +// if (!generatedParserDir.mkdir()) { +// return new CompiledState(generatedState, new Exception("can't make dir " + generatedParserDir)); +// } +// +// // The generated files seem to need to be in the parser subdirectory. +// // We have no need to change the import of the runtime because of go mod replace so, we could just generate them +// // directly in to the parser subdir. But in case down the line, there is some reason to want to replace things in +// // the generated code, then I will leave this here, and we can use replaceInFile() +// // +// for (GeneratedFile generatedFile : generatedFiles) { +// try { +// Path originalFile = Paths.get(tempDirPath, generatedFile.name); +// Files.move(originalFile, Paths.get(tempDirPath, "parser", generatedFile.name)); +// } catch (IOException e) { +// return new CompiledState(generatedState, e); +// } +// } + + // We have already created a suitable go.mod file, though it may need to have go mod tidy run on it one time + // Please remove commented code. Git keeps history very well. ------------------------------ In runtime-testsuite/test/org/antlr/v4/test/runtime/RuntimeRunner.java <#4181 (comment)>: > @@ -154,6 +154,13 @@ public static String getRuntimePath(String language) { return runtimePath.toString() + FileSeparator + language; } + // Allows any target to add additional options for the antlr tool such as the location of the output files + // which is useful for the Go target for instance to avoid having to move them before running the test + // + protected List<String> getTargetToolOptions() { Intead of adding quite abstract options I suggest adding concrete generatorOutputDirectory option. ------------------------------ In runtime-testsuite/test/org/antlr/v4/test/runtime/RuntimeRunner.java <#4181 (comment)>: > @@ -162,6 +169,14 @@ public State run(RunOptions runOptions) { if (runOptions.superClass != null && runOptions.superClass.length() > 0) { options.add("-DsuperClass=" + runOptions.superClass); } + + // See if the target wants to add tool options + // + List<String>targetOpts = getTargetToolOptions(); Whitespace is missing: ⬇️ Suggested change - List<String>targetOpts = getTargetToolOptions(); + List<String> targetOpts = getTargetToolOptions(); ------------------------------ In runtime-testsuite/test/org/antlr/v4/test/runtime/go/GoRunner.java <#4181 (comment)>: > + ArrayList<String> options = new ArrayList<String>(); + options.add("-o"); + options.add(tempTestDir.resolve("parser").toString()); + return options; It can be calculated once and used for every test since tempTestDir is calculated once. ------------------------------ In runtime-testsuite/resources/junit-platform.properties <#4181 (comment)>: > \ No newline at end of file +junit.jupiter.execution.parallel.mode.classes.default = concurrent +junit.jupiter.execution.parallel.config.fixed.parallelism=4 Setting up a manual value may decrease runtime tests performance. Also, I don't think 4 is suitable value, modern processors have many thread. I suggest using 8 or at least 6. — Reply to this email directly, view it on GitHub <#4181 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJ7TMBIIG7RKFPL643X3X3W4GJHTANCNFSM6AAAAAAV3NDQO4> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[jimidle]

Yes. It can. But I happen to agree with it. If only we knew people that worked at Jetbrains! ;) Are we good with the PR now?

…

On Fri, Mar 17, 2023 at 20:24 Ivan Kochurkin ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In runtime-testsuite/test/org/antlr/v4/test/runtime/RuntimeRunner.java <#4181 (comment)>: > + protected String getExtension() { + return getLanguage().toLowerCase(); + } I suspect it can be disabled in the options. — Reply to this email directly, view it on GitHub <#4181 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJ7TMAP35Q4Z6ETSZKKJPLW4RJXTANCNFSM6AAAAAAV3NDQO4> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[jimidle]

I’ll cycle back to this. I need to be able to change it just for go and allow other target authors to decide what works for them.

…

On Fri, Mar 17, 2023 at 00:27 Ivan Kochurkin ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In runtime-testsuite/resources/junit-platform.properties <#4181 (comment)>: > \ No newline at end of file +junit.jupiter.execution.parallel.mode.classes.default = concurrent +junit.jupiter.execution.parallel.config.fixed.parallelism=4 Probably you also have to add junit.jupiter.execution.parallel.config.strategy=fixed to the config file, see https://junit.org/junit5/docs/snapshot/user-guide/#writing-tests-parallel-execution-config — Reply to this email directly, view it on GitHub <#4181 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJ7TMAKWUHWN7F5BPBZP7LW4M5PPANCNFSM6AAAAAAV3NDQO4> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[parrt]

I'll wait for your signal Jim.

[jimidle]

This is good to go Ter. I took a day to improve the Go performance, and I’ve spent 4 days bikeshedding this PR ;). Let’s press the merge button oh dictator supreme. ;)

…

On Sat, Mar 18, 2023 at 09:02 Terence Parr ***@***.***> wrote: I'll wait for your signal Jim. — Reply to this email directly, view it on GitHub <#4181 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJ7TMFS6E66UDU2ULWWSXTW4UCRJANCNFSM6AAAAAAV3NDQO4> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[parrt]

I hear and obey!

[jimidle]

Fair enough ;). I’ll work it out. I think Ivan was on the right track.

…

On Sun, Mar 19, 2023 at 01:59 Terence Parr ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In runtime-testsuite/resources/junit-platform.properties <#4181 (comment)>: > \ No newline at end of file +junit.jupiter.execution.parallel.mode.classes.default = concurrent +junit.jupiter.execution.parallel.config.fixed.parallelism=4 @jimidle <https://github.com/jimidle> I can't remember what I had for lunch so.... haha — Reply to this email directly, view it on GitHub <#4181 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJ7TMFTUMYKQMDUZXZ7VELW4XZYVANCNFSM6AAAAAAV3NDQO4> . You are receiving this because you were mentioned.Message ID: ***@***.***>