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

Consider Nashorn ScriptEngine instance scope in ScriptTemplateView [SPR-13487] #18065

Closed
spring-projects-issues opened this issue Sep 23, 2015 · 13 comments
Assignees
Labels
in: web type: enhancement
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

@spring-projects-issues spring-projects-issues commented Sep 23, 2015

chanwook park opened SPR-13487 and commented

Spring MVC mechanism is create one instance for View (View = URL).
And ScriptViewTemplate has one Nashorn's ScriptEngine (SpringTemplateView#engine), or more ScriptEngine instances because using ThreadLocal (ScriptTemplateView#engineHolder).
For example, app has 1000 view(URL), then create 1000 ScriptTemplateView instance and at the same time create 1000 Nashorn’s ScriptEngine instance(or more +). And one ScriptEngine is loaded only one template HTML.

In my case, I have been used like that caused memory consumption. (I've been used JDK6+Rhino+Dust)

I hope to more careful managing view instance and Nashorn's ScriptEngine instance.
Maybe not consider share and DI to SpringEngine instance?
Of course, aspect of concurrency and resource management.

Thanks.


Affects: 4.2.2

Issue Links:

  • #18069 Revise script engine retrieval for better error reporting

Referenced from: commits c7fd4cc, cffad9d

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Sep 23, 2015

Sébastien Deleuze commented

Currently ThreadLocal<ScriptEngine> is a view field and is not static, so it could indeed result in too much ScriptEngine instances. We should ensure that the number of ScriptEngine can not exceed the number of threads.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Sep 23, 2015

chanwook park commented

I'am afraid that Nashorn is not Thread-safe. So we can not enroll plain Spring beans and share with views.
(ref: http://docs.oracle.com/javase/8/docs/api/javax/script/ScriptEngineFactory.html#getParameter-java.lang.String- , test result is 'null')

For managing ScriptEngine instance number, could we consider like that (1) pooling ScriptEngine like JDBC ConnectionPool.
Or (2) more concurrency handling logic with Nashorn?

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Sep 23, 2015

Sébastien Deleuze commented

Yes we are already aware of that. Just updating the ThreadLocal<ScriptEngine> engineHolder field to be static should limit the number of ScritpEngine to the number of threads, regardless of the number of view instances, while avoiding any concurrency issue with Nashorn.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Sep 23, 2015

chanwook park commented

Yes. you are correct.
'ThreadLocal<ScriptEngine> + static' is limit of ScriptEngine, but still create ScriptEngine instance to the number of 'View(=URL)'.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Sep 23, 2015

Sébastien Deleuze commented

chanwook park Issue fixed by making this field static. If you could test the latest 4.2.2.BUILD-SNAPSHOT when this build is finished and send us you feedback here as a comment, that would be very useful.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Sep 23, 2015

Juergen Hoeller commented

Hmm, what about different script engines in use? E.g. one ScriptTemplateView set up for JavaScript, another one for Jython... They couldn't share a ScriptEngine, could they? But that's exactly what they'd end up doing with a static ThreadLocal<ScriptEngine here...

Admittedly, this is not a common scenario, so I guess we could simply verify that the ScriptEngine that we encounter in the ThreadLocal is actually of the right type; if not, we could simply replace it.

Juergen

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Sep 23, 2015

Juergen Hoeller commented

I'm currently tackling the related #18069, so I can have a look at such an engine validation check as well...

Juergen

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Sep 23, 2015

Sébastien Deleuze commented

Juergen Hoeller Indeed, this is not common but it could occur, thanks for raising this.

Another option could be to create a ScriptEngineHolder class to encapsulate the ScriptEngine engine and ThreadLocal<ScriptEngine> engineHolder fields. In that case, engineHolder should be not static, and a single ScriptEngineHolder instance will be created by each ScriptTemplateConfigurer. That should avoid mixing multiple ScriptEngine implementation.

Do you think that make sense or since this is a not common scenario it's not worth to go on that way?

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Sep 23, 2015

Sébastien Deleuze commented

Reopening this issue after latest Juergen Hoeller feedback.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Sep 23, 2015

chanwook park commented

thanks comment Juergen and Sébastien.

I've been more test and some understanding for that.
I don't comment for mixing multiple script engine, using one script type (e.g. handlebars with JavaSciprt)

I means that below...

  1. in some case, create many number of ScriptEngine instance (when exist many view name...)
    In nashorn, if this point (many SE instance) do not problem, that's ok.

  2. always ScriptEngine has only one template HTML (or more with partial HTML).
    So @Sébastien Deleuze 's slide 27 page code, 'templates' variable has always value of size '1'. Because one of ScriptTemplateView instance has one of ScriptEngine instance and one of Template HTML. Is not that efficient problem?

By the way, if no concurrency issue, My first and second question is resolved by ScriptTemplateConfigurer.setEngine(scriptEngine).

ScriptTemplateConfigurer configurer = new ScriptTemplateConfigurer();
ScriptEngine engine = new ScriptEngineManager().getEngineByName("nashorn");
configurer.setEngine(engine);

If no concurrency issue, this code is enough for that. right?
If right, more correctly describe in documentation with caution(e.g. concurrency).

Finally when we don't use configurer.setSharedEngine(false) case, more managing like @Sébastien comment, change to static ThreadLocal<ScriptEngine> engineHolder.

thanks your working and comments.
Regards,

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Sep 23, 2015

chanwook park commented

I'm read in 4.2.2-BUILD-SNAPSHOT guide is already added for concurrency description.(y)

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Sep 23, 2015

Juergen Hoeller commented

In the course of #18069, I've changed the ThreadLocal to hold a Map<Object, ScriptEngine>, with the key either being the specified engineName or a composed EngineKey consisting of engineName and scripts.

This allows for configuration-specific ScriptEngine instances per thread, reused across all ScriptTemplateView instances in an application. If the configuration is identical across all views, only one ScriptEngine will ever be created per thread (and reused by all views).

Juergen

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Sep 24, 2015

chanwook park commented

Wow! Real good deal with that.
Thanks for your good working..

Regards,
chanwook

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web type: enhancement
Projects
None yet
Development

No branches or pull requests

2 participants