Next steps ? (Re: An attempt of a CSR draft ... (Re: A new WIP (PR # 192) (Re: WIP version with PI compile (Re: A WIP for JDK-8238080 for review/discussion (Re: "Internal review ID 9063426: "FXMLLoader: if script engines implement javax.script.Compilabel compile scripts"
Kevin Rushforth
kevin.rushforth at oracle.com
Sat May 9 15:16:46 UTC 2020
Hi Rony,
I'm finally getting back to this. I took a look at
https://github.com/openjdk/jfx/pull/192 and I like that as the direction
for this enhancement.
The initial CSR you have is a good start.
Next steps are:
1. Update the "Introduction to FXML" specification (see my comment in
the PR)
2. Update PR 192 with the draft CSR as a comment, modifying it to
include the above additions to "Introduction to FXML"
3. Remove WIP from the title
You can then close the other two PRs (129 and 187).
Thanks.
-- Kevin
On 4/28/2020 6:15 AM, Rony G. Flatscher wrote:
>
> Hi Kevin,
>
> what should be the next steps?
>
> Should I remove "WIP" from the title in
> <https://github.com/openjdk/jfx/pull/192> and add the CSR draft text
> of my last e-mail as a "CSR" comment with PR # 192, thereby requesting
> it to be added to
> <https://bugs.java.com/bugdatabase/view_bug.do?bug_id=JDK-8238080>?
>
> Please advise.
>
> TIA,
>
> ---rony
>
> P.S.: This is the RFE:
>
> * RFE (2020-01-24):
> <https://bugs.java.com/bugdatabase/view_bug.do?bug_id=JDK-8238080>
>
> These are the three versions (all with appropriate unit tests) that I
> came up with chronologically to implement the RFE, would prefer the
> latest version (PR # 192):
>
> * Compile if Compilable implemented (2020-02-28):
> <https://github.com/openjdk/jfx/pull/129>
> * Compile if compile PI and Compilable is implemented (2020-04-11):
> <https://github.com/openjdk/jfx/pull/187>
> * Compile with fallback, if Compilable is implemented, compile PI
> for fine-grained control (2020-04-14):
> <https://github.com/openjdk/jfx/pull/192>
>
>
> On 22.04.2020 20:01, Rony G. Flatscher wrote:
>> Hi Kevin,
>>
>> as I am not able to file a CSR with the issue you suggested to come up with a draft, so here it goes:
>>
>> Summary
>> =======
>> Have javafx.fxml.FXMLLoader compile FXML scripts before evaluating them, if the script engine
>> implements the javax.script.Compilable interface to speed up execution. In case compilation
>> throws a javax.script.ScriptException fall back to evaluating the uncompiled script. Allow
>> control of script compilation with a "compile" PI for FXML files.
>>
>> Problem
>> =======
>> javafx.fxml.FXMLLoader is able to execute scripts in Java script languages
>> (javax.script.ScriptEngine implementations) referred to or embedded in a 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 javax.script.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 repetitively invoked and evaluated.
>>
>> Solution
>> ========
>> Before evaluating the script code test whether the javax.script.ScriptEngine implements
>> javax.script.Compilable. If so, compile the script to a javax.script.CompiledScript object first
>> and then use its eval() method to evaluate the script, otherwise continue to use the
>> javax.script.ScriptEngine's eval() method instead. Should compilation of a script yield
>> (unexpectedly) a javax.script.ScriptException then fall back to using the
>> javax.script.ScriptEngine's eval() method. A new process instruction "compile" allows control of
>> the compilation of scripts ("true" sets compilation on, "false" to set compilation off) in FXML
>> files.
>>
>> Specification
>> =============
>> If a javax.script.ScriptEngine implements the javax.script.Compilable interface, then use its
>> compile() method to compile the script to a javax.script.CompiledScript object and use its
>> eval() method to run the script. In the case that the compilation throws (unexpectedly) a
>> javax.script.ScriptException log a warning and fall back to using the
>> javax.script.ScriptEngine's eval() method instead.
>> To allow setting this feature off and on while processing the FXML file a "compile" process
>> instruction ("<?compile false?>" or "<?compile true?>") gets defined that allows to turn
>> compilation off and on throughout a FXML file.
>>
>> Having never seen a real CSR I hope that this matches what is expected and is helpful for
>> assessment. If not please advise (got the name of these fields from [1]).
>>
>> ---
>>
>> Also added brief information about the respective test units (what they test and yield) in the WIP [2].
>>
>> ---rony
>>
>> [1] "CSR-FAQ":<https://wiki.openjdk.java.net/display/csr/CSR+FAQs>
>>
>> [2] "WIP: Script compilable+compile PI+fallback: 8238080: FXMLLoader: if script engines implement
>> javax.script.Compilable compile scripts #192":<https://github.com/openjdk/jfx/pull/192>
>>
>>
>> On 20.04.2020 14:58, Rony G. Flatscher wrote:
>>> There is a new WIP at<https://github.com/openjdk/jfx/pull/192>:
>>>
>>> This WIP adds the ability for a fallback in case compilation of scripts fails, in which case a
>>> warning gets issued about this fact and evaluation of the script will be done without
>>> compilation. Because of the fallback scripts get compiled with this version by default. It
>>> extends PR 187 #187.
>>>
>>> To further ease spotting scripts that cause a ScriptException a message in the form of
>>> "filename: caused ScriptException" gets added to the exception handling in either of the three
>>> locations: an error message, a stack trace or a wrap-up into a RuntimeException (having three
>>> different kinds of reporting ScriptExceptions may be questioned, however none of these tear down
>>> the FXML GUI).
>>>
>>> This WIP comes with proper test units as well. As per Kevin's suggestion a warning gets logged
>>> whenever a script cannot be compiled and the fallback gets used.
>>>
>>> It is suggested to use this WIP as it includes the compilation by default with a safe fallback to
>>> evaluate the uncompiled script, if compilation (unexpectedly) fails.
>>>
>>> Again, any feedback, discussion welcome!
>>>
>>> ---rony
>>>
>>> P.S.: In the log history there is a commit message "Make message more pregnant.", it should have
>>> read "Make messages more terse." instead|.||
>>> |
>>>
>>>
>>> On 17.04.2020 19:37, Rony G. Flatscher wrote:
>>>> There is a new WIP at<https://github.com/openjdk/jfx/pull/187> which adds a compile PI (process
>>>> instruction) for turning on and off script compilation if the script engine implements the
>>>> Compilable interface.
>>>>
>>>> By default compilation is off (no compilation), such that one needs to add a compile PI
>>>> ("<?compile?>") at the top to activate this feature. Supplying "true" (default) or "false" as the PI
>>>> data turns this feature on and off.
>>>>
>>>> The WIP comes with adapted test units that test "compile on" for an entire fxml file, "compile off",
>>>> alternating using "compile on and off", and alternating using "compile off and on". This will test
>>>> all variants of applying the compile PI for all categories of scripts.
>>>>
>>>> Any feedback appreciated!
>>>>
>>>> ---rony
>>>>
>>>> P.S.: FXML files that contain unknown PIs do not cause a runtime error by FXMLLoader, they just get
>>>> ignored. Therefore one could apply the compile PI to FXML files that are used in older JavaFX runtimes.
>>>>
>>>> P.P.S.: In the next days I will also add Kevin's idea in a separate version that will have a
>>>> fallback solution in case a compilation is (unexpectedly) not successful, reverting to
>>>> (interpretative) evaluation/execution of the script. In that version it is planned to have
>>>> compilation on by default as in the case of a compilation failure there will be a safe backup solution.
>>>>
>>>>
>>>> On 14.04.2020 19:52, Kevin Rushforth wrote:
>>>>> Yes, I agree that enough time has gone by. Go ahead with your proposal. I would wait a bit to
>>>>> create the CSR until the review is far enough along to know which direction we intend to go.
>>>>>
>>>>> Unless there is a real concern about possible regressions if scripts are compiled by default, I
>>>>> think "enabled by default" is the way to go. Your argument that such script engines are broken
>>>>> seems reasonable, since this only applies to script engines that implement javax.script.Compilable
>>>>> in the first place. We still might want to add way to turn compilation off for individual scripts.
>>>>> One other thing to consider is that if compilation fails, it might make sense to log a warning and
>>>>> fall back to the existing interpreted mode.
>>>>>
>>>>> Does anyone else have any concerns with this?
>>>>>
>>>>> -- Kevin
>>>>>
>>>>>
>>>>> On 4/14/2020 9:48 AM, Rony G. Flatscher wrote:
>>>>>> 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
>
More information about the openjfx-dev
mailing list