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
