Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

run multiple scripts in the same GroovyScriptEngine #782

Closed
wants to merge 3 commits into from

Conversation

sheehan
Copy link
Contributor

@sheehan sheehan commented Mar 12, 2016

This updates DslScriptLoader to run the batch of scripts using the same GroovyScriptEngine instance, rather than creating a new GroovyScriptEngine instance for each script. We have 20+ scripts and this change cuts the execution time in half.

I also cleaned up the file a bit and made it instance-based, rather than static.

@jenkinsadmin
Copy link
Member

Thank you for this pull request! Please check this document for how the Jenkins project handles pull requests.

@daspilker
Copy link
Member

Script execution time is becoming an issue. I started to work an a similar change a few weeks ago, but stopped because changing the classpath for the script engine may introduce problems for users. Currently each script engine gets a classpath consisting of the script`s directory and the additional classpath. When sharing a script engine, there can only be one classpath. One way of solving this is to merge all classpaths as you did. But user's may rely on having separate classpaths, e.g. when using the following workspace layout:

projectA/script.groovy
projectA/Utils.groovy
projectB/script.groovy
projectB/Utils.groovy

When using a combined classpath, the helper class in projectA/Utils.groovy will hide the helper class in projectB/Utils.groovy. It pushed a test case for this to master: 0f6e0ca

This is a subtle change and probably hard to debug for users. Can we find a compatible solution? E.g. only sharing a script engine for scripts with an identical classpath?

@daspilker
Copy link
Member

Sharing a binding between script will probably cause similar compatibility issues. Is there a reason why you share the binding?

@sheehan
Copy link
Contributor Author

sheehan commented Mar 14, 2016

Good points. I've updated to share only for scripts with an identical classpath. Also unshared the binding.

@@ -118,18 +162,6 @@ class DslScriptLoader {
idx > -1 ? fileName[0..idx - 1] : fileName
}

/**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's good that DslScriptLoader no longer relies on static methods, but do not move any methods. Leave these where they are and mark them as deprecated. Then we can remove them eventually.

@sheehan
Copy link
Contributor Author

sheehan commented Mar 16, 2016

That makes sense. The order is fixed now.

@daspilker
Copy link
Member

Cool. Can't wait to try this in production. I merged your commits manually.

@daspilker daspilker closed this Mar 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants