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

Tue Apr 14 16:48:54 UTC 2020

Hi there,

as there was probably enough time that has passed by I would intend to create a CSR in the next days
with the PR as per Kevin's suggestion.

(For the case that this feature should not be active by default, the CSR will suggest to define a
new "compile" PI in the form <?compile [true|false] ?> (default, if no PI data given: true), which
is independent of the existence of a language PI (this way it becomes also possible to allow
compilation of external scripts denoted with the script-element, which do not need a page language
to be set as the file's extension allows the appropriate script engine to be loaded and used for
execution). A compile-PI would allow for turning compilation of scripts on by just adding the PI
<?compile?> or <?compile true?>  to FXML files (and <?compile false?> to turn off), which seems to
be simple and self-documentary. In general employing such compile PIs allows for setting compilation
of scripts on and off throughout an FXML file.)

---rony

On 04.04.2020 18:03, Rony G. Flatscher wrote:

> 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
>