eclipse-birt / birt

Eclipse BIRT™ The open source reporting and data visualization project.
http://www.eclipse.org/birt
Eclipse Public License 2.0
451 stars 389 forks source link

Upgrade Rhino to GraalJS #732

Closed SteveSchafer-Innovent closed 4 months ago

SteveSchafer-Innovent commented 2 years ago

I think it would be great to upgrade the ancient Rhino javascript library to GraalJS. There would be many benefits to doing this including full ECMA compliance, improved performance, and the possibility of using languages other than javascript. At first I thought this would be a huge project, but maybe it won't be so bad.

I'm not really sure how Rhino is included in BIRT. I can't seem to find it in any of the POM's or in the .target file.

It would not necessarily require GraalVM but the performance would be better if GraalVM was used. Issue 613 is about running BIRT in GraalVM and is still open.

hvbtup commented 2 years ago

I think it is very important that >99.9% of the scripts in existing reports will work as before.

A newer version of Javascript might break existing functionality. I'm not a Javascript expert, though. Does anyone know if ancient scripts will still work as before?

wimjongman commented 2 years ago

Scripts may not fail in existing reports. This should be covered by the tests. It does seem that Rhino is still active:

https://github.com/mozilla/rhino

SteveSchafer-Innovent commented 2 years ago

It looks like it would be compatible, but of course it would need to be tested. The way strings are treated might be a problem.

I created this issue because of the persistent irritation of having to work with old JS syntax in BIRT code while using modern JS syntax in browser code. In my work there's a lot of both, often in the same report (browser JS usually in Text components).

hvbtup commented 2 years ago

The way strings are treated might be a problem.

I think this is one of the general problems with using Java from Javascript and vice versa.

Of course this already is a problem with the existing JS integration.

In many cases it is not quite clear if an object is a Javscript string or a Java string. Then what is the meaning of the replace method, for example?

Quite often, I have some logic which uses string objects which may be JS or Java (some of them columns from a data set, some are the result of some other logic, and some are Java objects (results columns of direct JDBC access). As a best practice(?), I tend to use the construct "" + x to convert x to a JS string object or new java.lang.String(x) to convert x to a Java string object.

Does anyone know about backward compatibility of Rhino releases?

Should a report contain a property specifying the intended ECMAScript release? It seems like Rhino can be configured somehow in this regard, see the remark "requires VERSION_ES6 or -version 200 flag" at the top of https://mozilla.github.io/rhino/compat/engines.html.

patric-r commented 2 years ago

I agree that backward compatibility should be top priority when thinking about javascript engine modernization.

Therefore, +1 for a configuration option to let the report designer choose, with still having the ancient rhino as a first class (and default by now) citizen - or to stick at rhino (for now).

We are happy with Rhino (we're not using javascript that much, even for complex reports), having a more recent Javascript engine is therefore not important for us, especially when comparing to other BIRT issues/feature requests.

perNyfelt commented 2 years ago

Nashorn (https://github.com/openjdk/nashorn) is actively developed and would be a good alternative to Rhino as well.

wimjongman commented 2 years ago

Is this going to be worked on? If not please close it.

speckyspooky commented 1 year ago

Hi there, I know this thread isn't the newest and I'm not a specialist of Rhino-integraton on BIRT.

But in addition to SteveSchafer an idea. Would it be possible to have both JS-engines Rhino & GraalJS on BIRT. The additional idea would be that we would have 2 configuration options of compatibility of existing reports. 1 configuration define the default engine on global level to use "Rhino" or "GraalJS" and 1 configuration option on report level so that the report developer can switch on report level between the both engines and overwrite the global settings with this configuration.

So we would have a solution of the current reports and a modern engine for new / modernised reports.

Would it be an option to go this double-engine-way? And have we there a developer which could implements this change (I know this isn't so easy)?

hvbtup commented 1 year ago

Note: I'm not an expert on this topic. I found this information on https://github.com/oracle/graaljs/blob/master/docs/user/RunOnJDK.md:

As GraalVM JavaScript is a Java application, it is possible to execute it on a stock Java VM like OpenJDK. When executed without the GraalVM compiler, JavaScript performance will be significantly worse. ... JSR 223 GraalVM JavaScript can be started via ScriptEngine when js-scriptengine.jar is included on the classpath.

Besides the option to choose which engine to use, I think it is important to specify the ECMAScript version as well.

wimjongman commented 1 year ago

I looked at the code, and it looks doable.

Re-exporting

The original authors of BIRT used re-exporting dependencies. This is now considered not ideal. Each plugin should define its own dependencies and not inherit them through another dependency. For example, the BIRT core plugin re-exports org.eclipse.core.runtime. This means that any client of birt core does not have to depend on org.eclipse.core.runtime because it gets it for free from birt core. Re-exporting makes the dependency chain unclear.

RHINO is re-exported by birt.core

One of the re-exported plugins is org.mozilla.javascript (the RHINO engine). It is re-exported by o.e.birt.core and o.e.birt.data. This means that BIRT is hooked into this implementation.

However, when removing the re-export, only a handful of plugins and around 20 classes showed errors. This means that we should be able to refactor this with some effort.

Refactoring

To refactor this so we can use any scripting engine, we should not rely on org.mozilla.javascript directly but acquire it through the standard script engine classes of Java.

Rhino

ScriptEngine engine = new ScriptEngineManager().getEngineByName("rhino"); engine.eval("print('Hello World!');");

Graal

ScriptEngine engine = new ScriptEngineManager().getEngineByName("graal.js"); engine.eval("print('Hello World!');");

Nashorn

ScriptEngine engine = new ScriptEngineManager().getEngineByName("nashorn"); engine.eval("print('Hello World!');");

Getting started

  1. Startup a birt development environment.
  2. Remove the Mozilla dependencies in org.eclipse.birt.core and org.eclipse.birt.data
  3. Fix the errors
hvbtup commented 1 year ago

Updating Rhino (maybe even switching to the fork https://github.com/Servoy/rhino/tree/servoy) or switching to a different Javascript engine could probably solve issues with Rhino and Java 17 (see #1138).

speckyspooky commented 1 year ago

We talked in this thread to avoid changes which change the behavior of the ECMAScript handling when we would implements alternatives engines. I prefer the swith options like my comment before.

But we have another point hvbtup noted the relevant detail and his discussion with #1138 and I had same problem with another context #1160.

The topic is till JDK 8 the usage of "javax"- , "com sun.*"- classes are no problem with rhino and the usage on report context. Since JDK 9 till 11 the usage is possible but with warnings. Since JDK 16 the usage of such classes is not longer possible and the reports will crash.

The reason is the JVM-module-system which was started with JDK 9. Our used BIRT rhino engine in version 1.17.10 isn't able to use it correctly. The newest version 1.17.14 fix sume issues on it (but not all).

So it seems that we will get an issue with older reports based on the historical Java usage if we cannot solve the problem through a new rhino version or our switch handling of alternative Script-engines.

hvbtup commented 1 year ago

I saw that in Rhino, one can specify which Ecmascript-Version the compiler uses by calling Context.setLanguageVersion. GraalVS allows to select the ES version as well (https://www.graalvm.org/latest/reference-manual/js/JavaScriptCompatibility/)

I also saw that Rhino only supports a small (but very useful) subset of the ES 6 features.

I still think it is important to take the ES version into account. So, in rptdesign and rptlibrary file, there should be an option to specify the ES version that this module uses. The default value would be ES 5. When a module loads a library, it should be checked that their ES versions match, otherwise there should be an error.

Regarding switching to a different JS implementation, e.g. GraalVM or Nashorn, I'm unopinionated. However, https://www.graalvm.org/latest/reference-manual/js/RhinoMigrationGuide/ says that probably the JS source code has to be changed here and there in existing reports, so this would be a breaking change.

wimjongman commented 4 months ago

Closing as WONTFIX for now. Please reopen if you want to work on this.