Skip to content

Conversation

kinwahlai
Copy link

This is my attempt to use Nashorn script engine for the plugin.
I haven't use it in production yet.

Let me know if this is useful.

hmalphettes and others added 2 commits March 3, 2014 16:05
Requires JDK8
Settings script.javascript.engine: rhino/nashorn (default: rhino)
@kimchy
Copy link
Member

kimchy commented Mar 5, 2014

this will only work with Java 8..., I would love to have a nashorn one, I hope we will simply provide it as a build in option in core elasticsearch.

@hmalphettes
Copy link
Contributor

@kimchy we were trying to make a single build that supports both rhino and nashorn.
With a little more efforts we can probably hide the 2 references to nashorn's classes completely:

  • compile with jdk6
  • support jdk6+ / rhino
  • support jdk8 / nashorn

Once that is done this work could move into core and nashorn scripting would be enabled when jdk8 is detected.

Let us know if it is useful to continue that here or how we can help with javascript support in general.

@kimchy
Copy link
Member

kimchy commented Mar 5, 2014

if you can wait a bit, I would love to get you involved when we properly start the effort of supporting javascript. I still don't have a clear view on how to properly do it. For example, Java 6/7 comes with Rhino built in, but I found that using the rhino API directly and enabling some feature flags made the performance much better. I would love to know if that is the case with nashorn, and maybe it makes sense to use nashorn not through the ScriptEngine interface.

Other thoughts that we had was possible using the back ported nashorn that works on Java 7 as well... . Its all kindda up in the air right now. Maybe you can share some perf numbers if you have them between current rhino impl and the nashorn one when used through the ScriptEngine interface?

@kimchy
Copy link
Member

kimchy commented Mar 5, 2014

btw, supporting rhino for Java 6/7, and nashorn for 8 sounds like a very interesting path to take as well, potentially by using the rhino engine embedded in Java if its better perf wise compared to what I checked a couple of years ago :)

@dadoonet
Copy link
Contributor

dadoonet commented Mar 6, 2014

btw, supporting rhino for Java 6/7, and nashorn for 8 sounds like a very interesting path to take as well, potentially by using the rhino engine embedded in Java if its better perf wise compared to what I checked a couple of years ago :)

@kimchy Does it mean that we will have javascript embedded in elasticsearch core code and we won't need anymore a plugin?

@kinwahlai
Copy link
Author

@kimchy OK, keep us in the loop and at the mean time we will continue to refine this PR along those lines.

When the setting "script.javascript.engine" is not specified or equal to
"auto", the engine selected depends on the Java version: Nashorn for Java
8 and Rhino for Java 6 and 7.

Also take extra care to not load the nashorn or rhino classes
at all if the engine is not selected.
@hmalphettes
Copy link
Contributor

OK: this code compiles with Java-6.
It should run without rhino.jar with Java-6/7 selecting Rhino automatically.
It runs Nashorn with Java-8.

If the approach is interesting to you, we'll actually try it for real.
And rebase on the top of the master branch.

We also made some effort to lazily bind and wrap the script variables for Nashorn.
If you find that interesting we can do the same optimisation for Rhino.

@kimchy
Copy link
Member

kimchy commented Mar 8, 2014

Seems like @jprante just went and created a nashorn plugin, have no idea why to be honest, its here: https://github.com/jprante/elasticsearch-lang-javascript-nashorn. Be careful with using it, I haven't verified that using thread local to cache the script engine actually works at scale. Also, I have seen that trying to use the script engine interface proved to be very costly perf wise, this is why there isn't one that is based on it.

@jprante
Copy link

jprante commented Mar 8, 2014

@kimchy I wasn't aware that another Nashorn effort was underway, my coding started last December. The plugin is useful for my projects, so I can use scripting even if MVEL breaks under Java 8.
Thread local caching was a method to get somehow pass the multi thread tests, without synchronization (will be removed, I had a bug in my code). I'm still wrestling with bugs in my code and JSR 223, it does not make any assumption about multi threading, for whatever reason. Yes, performance is miserably right now.

@hmalphettes
Copy link
Contributor

Oh! Sorry I missed your development Jörg. I have re-enabled the SimpleBench
and got really sad at the tragic performance.
It is getting closer to what Rhino is doing but not there yet.
I'll finish what I have been doing for performance, push it and then will
await to see what we should do.

On Sat, Mar 8, 2014 at 10:11 AM, Jörg Prante notifications@github.comwrote:

@kimchy https://github.com/kimchy I wasn't aware that another Nashorn
effort was underway, my coding started last December. The plugin is useful
for my projects, so I can use scripting even if MVEL breaks under Java 8.
Thread local caching was a method to get somehow pass the multi thread
tests, without synchronization (will be removed, I had a bug in my code).
I'm still wrestling with bugs in my code and JSR 223, it does not make any
assumption about multi threading, for whatever reason. Yes, performance is
miserably right now.

Reply to this email directly or view it on GitHubhttps://github.com//pull/13#issuecomment-37104925
.

Add Nashorn benchmarking to SimpleBenchmark.
Configure Nashorn to reuse the same GlobalDefinition of
ecma script builtin objects.
Try to make Nashorn don't look too slow compared to Rhino.
@jprante
Copy link

jprante commented Mar 9, 2014

@hmalphettes I have to say sorry, I pushed the code out this week without previously investigating other projects ... but now we can team up?

I think we should consider to travel the bumpy road and dive into Nashorn private API if there is a chance to gain more performance out of it. Would be sad if JSR 223 was the only culprit here.

@kimchy
Copy link
Member

kimchy commented Mar 9, 2014

when I wrote the initial javascript implementation here, in rhino, the usage pattern was just so different from what Java script engine provided, that the performance was very bad.

@jprante if you know the performance is bad, and that its an alpha project, can you at least denote it in the README of the plugin project? This is unfair for users who would try your project and think it magically makes javascript faster.

The javascript requires a lot more work, to check and properly move to a good javascript based implementation. Will it be nashorn, back ported to 1.7 potentially? Maybe dynajs? I don't know... .

@hmalphettes
Copy link
Contributor

@jprante: no problem. I have used your codebase to experiment with the
performance issues without the extra complexity of introspection: it feels
like we are already teaming up :-)

I think I am getting closer to nailing the performance issue.
A script must be compiled once per thread: each compiled script keeps a
reference to Nashorn's internal Global object.
That object is managed with a ThreadLocal inside NashornScriptEngine.

When a Nashorn CompiledScript is evaluated, the first thing Nashorn does is
to check that the Global object it references is the same than the one used
by the engine. When that is not the case it re-compiles the script on every
execution.
Crap performance and obviously not thread-safe.

So here is my next try: ThreadLocal ScriptEngine combined with a
ThreadLocal cache of the Nashorn's CompiledScript.

@kimchi: I think we can afford to experiment with the 2 codebases: one of
them is cleaner and leaner focuses only on Nashorn. The other one is trying
to cover all Java version with a single built plugin at the cost of some
extra internal complexity.
We will have more options when the day comes to choose.

On Sun, Mar 9, 2014 at 9:39 AM, Shay Banon notifications@github.com wrote:

when I wrote the initial javascript implementation here, in rhino, the
usage pattern was just so different from what Java script engine provided,
that the performance was very bad.

@jprante https://github.com/jprante if you know the performance is bad,
and that its an alpha project, can you at least denote it in the README of
the plugin project? This is unfair for users who would try your project and
think it magically makes javascript faster.

The javascript requires a lot more work, to check and properly move to a
good javascript based implementation. Will it be nashorn, back ported to
1.7 potentially? Maybe dynajs? I don't know... .

Reply to this email directly or view it on GitHubhttps://github.com//pull/13#issuecomment-37131312
.

@kimchy
Copy link
Member

kimchy commented Mar 9, 2014

what I found a couple of years ago is that it was a shame to try and hack around specific ScriptEngine impls, and just integrate directly with the source (as was done with rhino in this case).

I think that its possible to have automatic integration of some sort, without having to go through ScriptEngine, for example, based on what is found in the classpath. But thats the simple part, the more interesting part would be to have the tight integration with nashorn,

@hmalphettes
Copy link
Contributor

Yes I never believed in JSR223 either: wrapping unwrapping makes the code
specific to the engine and we need better than the common denominator.

But Nashorn does not leave us much choice in that front:
The only public API to setup the engine is the JSR223 engine with a few
extensions... and I am using them.
For example: the class where compilation and evaluation of a String takes
place is NashornScriptEngine.
It is final and its constructors are friendly.
The only way to call it is via NashornScriptEngineFactory which implements
JSR223's ScriptEngineFactory.
The low level runtime of Nashorn requires to be passed JSR223 ScriptContext
and Bindings.

On Sun, Mar 9, 2014 at 2:16 PM, Shay Banon notifications@github.com wrote:

what I found a couple of years ago is that it was a shame to try and hack
around specific ScriptEngine impls, and just integrate directly with the
source (as was done with rhino in this case).

I think that its possible to have automatic integration of some sort,
without having to go through ScriptEngine, for example, based on what is
found in the classpath. But thats the simple part, the more interesting
part would be to have the tight integration with nashorn,

Reply to this email directly or view it on GitHubhttps://github.com//pull/13#issuecomment-37139936
.

Nashorn has some constraints to be used in a multi-threaded environment
The easiest is to create a new Global object on every execution
but that slows down by an order of magnitude each execution.

What we do is configure one engine per thread.
Each engine has its own version of Global and reuses it across
executions.

We also need to compile the scripts for each thread.
@jprante
Copy link

jprante commented Mar 10, 2014

@hmalphettes that is a challenging nashorn implementation situation. I was also about to examine how to streamline the ThreadLocals, with the global script state, and the ScriptEngine. I'm quite skeptical if it is worth to stay with JSR 223, I'd rather like to forget about the limitations in JSR 223.

What I found is a blog post https://blogs.oracle.com/nashorn/entry/nashorn_multi_threading_and_mt

This reveals there is a "workers" library planned, but no ETA. First, this sounds promising, it is aimed for executing Nashorn by using a pool.

The challenges so far:

  • Nashorn performance with multi thread (maybe with single thread too?)
  • Nashorn Java 7/8 compat
  • a solution for a future version of the ES Javascript plugin, supporting both Rhino and Nashorn

This should be preferably ready for integration into the ES core, or something like a drop-in replacement for the current lang-javascript plugin.

Since Nashorn comes with GPL license, a tight integration has to be considered well. Right now I'm thinking about everything, even forking the Nashorn code, and implement the missing API plugs for better performance, also along JDK7 and JDK8 branches. This GPL code could then be included into a GPLed ES nashorn plugin. GPL is not an issue for my projects, but apparently for ES core.

Also I'm trying to make a clearer picture for me how others solved the known JSR 223 issues. Scripturian is advertising with concurrent execution performance and scalability: https://code.google.com/p/scripturian/

I hacked a first (mavenized) backport of the current Nashorn code to JDK7, it is not difficult at all, and it is not very different to the bitbucket project from Sep 2013 https://bitbucket.org/ramonza/nashorn-backport

@kimchy
Copy link
Member

kimchy commented Mar 10, 2014

Just a side note, Lucene 4.8 is moving to 1.7, which means ES will as well when integrated. In that case, it makes the decision simpler for anything invokedynamic based. I would also love to run a benchmark (for simple math based logic to start, most common use case) between nashorn and dynjs, dynjs might end up being a good solution.

@jprante
Copy link

jprante commented Mar 10, 2014

@kimchy thanks for dynjs - this looks great. It's definitely time to launch some benchmarks between Rhino, dynjs, and Nashorn.

@hmalphettes
Copy link
Contributor

All right! All noted.

On the Nashorn front: 1 Thread -> 1 Engine -> 1 CompiledScript.
Performance is almlost as good as Rhino's and the multi-threaded tests are passing. All those ThreadLocals are smelling as strong as my favorite cheese... and it breaks another test.
I am travelling so I did not get to review with @kinwahlai.
I feel we can have another shot at Nashorn following @kimchy's suggestion to purposefully avoid all JSR223 interfaces and hopefully avoid all ThreadLocal by the same shot.
We can also wait for Nashorn to deliver the Web-Worker like support. It would be a slow cook if we need to wait for the next jdk build.

@dadoonet
Copy link
Contributor

dadoonet commented Feb 1, 2017

Closing as this plugin doe snot live here anymore and we highly recommend using painless instead.

@dadoonet dadoonet closed this Feb 1, 2017
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.

5 participants