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
