A WIP for JDK-8238080 for review/discussion (Re: "Internal review ID 9063426: "FXMLLoader: if script engines implement javax.script.Compilabel compile scripts"

Sat Apr 4 16:03:37 UTC 2020

Hi Kevin,

On 03.04.2020 01:21, Kevin Rushforth wrote:
> I see that you updated the PR and sent it for review.
>
> Before we formally review it in the PR, let's finish the discussion as to whether this is a useful
> feature, and if so, what form this feature should take.
>
> From my point of view, this does seem like a useful feature. Would other users of FXML benefit
> from it?

Script code should be executed faster after compilation, so any FXML page that hosts script code may
benefit.

The benefits depend on the type of script (and maybe its size and its complexity) and also on the
types of event handlers the scripts serve, e.g. move or drag event handlers may benefit
significantly. This is because repeated invocation of compiled script event handlers do not cause
the reparsing of that script's source and interpreting it on each invocation, which may be expensive
depending on the script engine, but rather allows the immediate evaluation/execution of the compiled
script by the script engine.

> I'm not certain whether we want it to be implicit, compiling the script if the script engine in
> question implements Compilable, or via a new keyword or tag. What are the pros / cons?

In principle there are three possibilities:

    1) If a script engine implements javax.script.Compilable, compile the script and execute the
    compiled version. In the case of event handlers compile and buffer the compiled script and
    execute the compiled script each time its registered event fires. 

      o Pro: immediately benefits all existing FXML pages that host scripts
      o Con: it is theoretically possible (albeit quite unlikely) that there are scripts that fail
        compiling but have been employed successfully in interpreted mode

    2) Introduce some form of an optional attribute (e.g. "compile={true|false}") to the FXML
    language PI that switches on compilation of scripts hosted in FXML definitions if the script
    engine implements the javax.script.Compilable interface. If missing it would default to "false".
    (Alternatively, add a "compile" PI, that if present causes the compilation of scripts, if the
    script engine supports it. It would be an error if the "compile" PI was present, but the
    "language" PI was not.)

      o Pro: compilation of FXML hosted scripts is done only, if the FXML definition of the language
        PI gets changed
      o Con: benefit not made available automatically to existing FXML pages that host scripts

    3) Another possibility would be to define a boolean attribute/property "compile" for script and
    node elements and only compile the scripts, if the property is set to true

      o Pro: compilation of FXML hosted scripts is done only, if the FXML definition gets changed
        accordingly
      o Con: potential benefit not made available automatically to existing FXML pages that host scripts

2 and 3 could be combined, where 2 would define the default compilation behavior that then could be
overruled individually by 3.

The question would be whether 2 or/and 3 are really necessary as it can be expected that compilation
of scripts by the script engines would find the same errors as while interpreting the very same
scripts (if not, the script engine is badly broken and it could be argued that then the
interpretation part of the script engine might be expected to be broken as well which would be quite
dangerous from an integrity POV; the same consideration applies as well if the execution of the
compiled script would behave differently to interpreting the very same script by the same script
engine).

The current WIP implements 1 above and includes an appropriate test unit.

> What do others think?

> In either case, we would need to make sure that this doesn't present any security concerns in the
> presence of a security manager. As long as a user-provided class is on the stack, it will be fine,
> but we would need to ensure that.

The compilation of scripts needs to be done by the Java script engines (implementing both,
javax.script.Engine and javax.script.Compilable) as well as controlling the execution of compiled
scripts ([javax.script.CompiledScript]
(https://docs.oracle.com/en/java/javase/14/docs/api/java.scripting/javax/script/CompiledScript.html)).

---rony

> On 4/2/2020 10:41 AM, Rony G. Flatscher wrote:
>> After merging master, applying some fixes and changing the title to reflect the change from WIP to a
>> pull request I would kindly request a review of this pull request!
>>
>> Here the URL: <https://github.com/openjdk/jfx/pull/129>, title changed to: "8238080: FXMLLoader: if
>> script engines implement javax.script.Compilable compile scripts".
>>
>> ---rony
>>
>>
>> On 28.02.2020 19:22, Rony G. Flatscher wrote:
>>> Here is a WIP [1] implementation of [2]. The WIP is based on [3], which is currently in review, and
>>> has an appropriate test unit going with it as well, please review.
>>>
>>> There should be no compatibility issue with this implementation.
>>>
>>> Discussion: another solution could be to not compile by default. Rather compile, if some new
>>> information is present with the FXML file which cannot possibly be present in existing FXML files.
>>> In this scenario one possible and probably simple solution would be to only compile scripts if the
>>> language process instruction (e.g. <?language rexx?>) contains an appropriate attribute with a
>>> value
>>> indicating that compilation should be carried out (e.g.: compile="true"). This way only FXML with
>>> PIs having this attribute set to true would be affected. If desired I could try to come up with a
>>> respective solution as well.
>>>
>>> ---rony
>>>
>>> [1] "Implementation and test unit": <https://github.com/openjdk/jfx/pull/129>
>>>
>>> [2] "JDK-8238080 : FXMLLoader: if script engines implement javax.script.Compilable compile
>>> scripts":
>>> <https://bugs.java.com/bugdatabase/view_bug.do?bug_id=JDK-8238080>
>>>
>>> [3] "8234959: FXMLLoader does not populate ENGINE_SCOPE Bindings with FILENAME and ARGV":
>>> <https://github.com/openjdk/jfx/pull/122/commits>
>>>
>>>
>>> On 24.01.2020 16:26, Kevin Rushforth wrote:
>>>
>>>> Thank you for filing this enhancement request. As an enhancement it should be discussed on this
>>>> list before proceeding with a pull request (although a "WIP" or Draft PR can be used to illustrate
>>>> the concept).
>>>>
>>>> For my part, this seems like a reasonable enhancement, as long as there are no compatibility
>>>> issues, but it would be good to hear from application developers who heavily use FXML.
>>>>
>>>> -- Kevin
>>>>
>>>>
>>>> On 1/24/2020 7:21 AM, Rony G. Flatscher wrote:
>>>>> Just filed a RFE with the following information:
>>>>>
>>>>>     * Component:
>>>>>         o JavaFX
>>>>>
>>>>>     * Subcomponent:
>>>>>         o fxml: JavaFX FXML
>>>>>
>>>>>     * Synopsis:
>>>>>         o "FXMLLoader: if script engines implement javax.script.Compilabel compile scripts"
>>>>>
>>>>>     * Descriptions:
>>>>>         o "FXMLLoader is able to execute scripts in Java script languages
>>>>> (javax.script.ScriptEngine
>>>>>           implementations) if such a Java script language gets defined as the controller
>>>>> language in
>>>>>           the FXML file.
>>>>>
>>>>>           If a script engine implements the javax.script.Compilable interface, then such scripts
>>>>> could
>>>>>           be compiled and the resulting javax.script.CompiledScript could be executed instead
>>>>> using
>>>>>           its eval() methods.
>>>>>
>>>>>           Evaluating the CompiledScript objects may help speed up the execution of script
>>>>> invocations,
>>>>>           especially for scripts defined for event attributes in FXML elements (e.g. like
>>>>> onMouseMove)
>>>>>           which may be repetitevly invoked and evaluated."
>>>>>
>>>>>     * System /OS/Java Runtime Information:
>>>>>         o "All systems."
>>>>>
>>>>> Received the internal review ID: "9063426"
>>>>>
>>>>> ---rony