Hi Kevin, in the meantime I have tried to come up with a formulation for the "Introduction to FXML" specification about the new compile processing instruction which is brief and complete. While being there I fixed a typo in the document and added a missing language processing instruction to an existing script example, hope that was o.k. as I have not filed an explicit bug for it.
Also closed the PR 129 and 187, removed the WIP from PR 192, and supplied the CSR draft as a comment. If there is anything else I should do, please let me know. Cheers, ---rony On 11.05.2020 14:12, Rony G. Flatscher wrote: > Hi Kevin, > > On 09.05.2020 17:16, Kevin Rushforth wrote: >> 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. > Thank you! > >> 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). > will do (may take a little bit). > > Cheers > > ---rony > > >> 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
