Evaluate MapCSS expression without array creation

[simon04]

All MapCSS from org.openstreetmap.josm.gui.mappaint.mapcss.Functions are evaluated using reflection and require an array to evaluate the arguments (see org.openstreetmap.josm.gui.mappaint.mapcss.ExpressionFactory.ParameterFunction#evaluate).

I propose to replace the ParameterFunction reflection magic with explicitly registered MapCSS function using lambda expressions.

Here's the patch: ​https://github.com/simon04/josm/commit/5a571a81b69c1859a98aed2cccd8117d4978bcf0

[Don-vip]

The code is easier to understand, +1 :)

[taylor.smock]

My initial concern was that someone would forget to add a new function to the map. But you already added a test for it. :)

Anyway, why isn't Factory.ofVarArgs not Factory.ofNumberVarArgs? There is ofStringVarArgs and ofObjectVarArgs, while ofVarArgs appears to be for numbers (converts to Double).

I kind of wonder how this will affect performance (I would expect positively, since it goes from an array to a HashMap).

[simon04]

Thank you for your reviews :)

Replying to taylor.smock:

Anyway, why isn't Factory.ofVarArgs not Factory.ofNumberVarArgs?

Good point. I've renamed the function.

I kind of wonder how this will affect performance (I would expect positively, since it goes from an array to a HashMap).

The ArrayList/HashMap change (only) affects MapCSS parsing. The removal of reflection is also / in particular beneficial for evaluating the functions.

Let's give it a try ...

[simon04]

In 17758/josm:

fix #20744 - Evaluate MapCSS expression without array creation

[simon04]

In 17762/josm:

see #20744 - Evaluate MapCSS pseudo classes without array creation

[gaben]

r17758 breaks rendering. In case of cycleway it renders only the right side of the way, cycleway:left=* completely ignored.
Also, the maxspeed mappaint style broken somehow, maybe unrelated.

URL:https://josm.openstreetmap.de/svn/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2021-04-12 22:43:35 +0200 (Mon, 12 Apr 2021)
Build-Date:2021-04-13 01:30:58
Revision:17763
Relative:URL: ^/trunk

Identification: JOSM/1.5 (17763 hu) Windows 10 64-Bit
OS Build number: Windows 10 Pro for Workstations 2009 (19042)
Memory Usage: 958 MB / 1820 MB (314 MB allocated, but free)
Java version: 1.8.0_281-b09, Oracle Corporation, Java HotSpot(TM) 64-Bit Server VM
Look and Feel: com.sun.java.swing.plaf.windows.WindowsLookAndFeel
Screen: \Display0 1920×1200 (scaling 1,00×1,00)
Maximum Screen Size: 1920×1200
Best cursor sizes: 16×16→32×32, 32×32→32×32
System property file.encoding: Cp1250
System property sun.jnu.encoding: Cp1250
VM arguments: [-Djava.security.manager, -Djava.security.policy=file:<java.home>\lib\security\javaws.policy, -DtrustProxy=true, -Djnlpx.home=<java.home>\bin, -Djnlpx.origFilenameArg=%UserProfile%\AppData\LocalLow\Sun\Java\Deployment\cache\6.0\31\583aa85f-30a40c63, -Djnlpx.remove=false, -Djava.util.Arrays.useLegacyMergeSort=true, -Djnlpx.heapsize=NULL,2048m, -Djnlpx.splashport=64835, -Djnlp.application.href=https://josm.openstreetmap.de/download/josm-latest.jnlp, -Djnlpx.jvm=<java.home>\bin\javaw.exe]
Dataset consistency test: No problems found

Plugins:
+ OpeningHoursEditor (35640)
+ PicLayer (2a9aa7a)
+ apache-commons (35524)
+ buildings_tools (35669)
+ continuosDownload (91)
+ ejml (35458)
+ geotools (35458)
+ gridify (1606242219)
+ jaxb (35543)
+ jts (35458)
+ log4j (35458)
+ opendata (35640)
+ pbf (35720)
+ pt_assistant (2.1.10-80-g7d9bba3)
+ reverter (35732)
+ tageditor (35640)
+ turnlanes-tagging (288)
+ utilsplugin2 (35691)
+ wikipedia (1.1.4)

Tagging presets:
+ https://josm.openstreetmap.de/josmfile?page=Presets/LaneAttributes&zip=1
+ https://josm.openstreetmap.de/josmfile?page=Presets/Healthcare&zip=1

Map paint styles:
- https://josm.openstreetmap.de/josmfile?page=Styles/Maxspeed&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/Lane_and_Road_Attributes&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/Surface&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/Surface-DataEntry&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/PublicTransport&zip=1

[simon04]

In 17764/josm:

see #20744 - Fix MapCSS function prop

Regression of r17758.

[simon04]

In 17765/josm:

see #20744 - Remove NO_UCD (unused code)

[gaben]

Replying to gaben:

Also, the maxspeed mappaint style broken somehow, maybe unrelated.

See ticket:20739#comment:6.

[simon04]

In 17769/josm:

see #20744 - MapCSS: fix unary minus

Regression of r17758.

[simon04]

In 17770/josm:

see #20744, fix #20757 - MapCSS: fix NPE

Regression of r17758.

[simon04]

In 17774/josm:

see #20744 - Fix ConditionFactoryTest ($jacocoInit)

[skyper]

Styles/Lane_and_Road_Attributes is still not working with r17770 nor r17774. It works with r17756.

[simon04]

In 17780/josm:

see #20744, fix #20764 - MapCSS: fix NPE

[simon04]

In 17781/josm:

see #20744, see #20764 - MapCSS: robustness in Selector.matches

[Don-vip]

There are 7 compiler warnings:

    [javac] C:\SVN\josm\core\src\org\openstreetmap\josm\gui\mappaint\mapcss\ExpressionFactory.java:192: warning: [unchecked] unchecked method invocation: method of in interface Factory is applied to given types
    [javac]         FACTORY_MAP.put("join_list", Factory.of(String.class, List.class, Functions::join_list));
    [javac]                                                ^
    [javac]   required: Class<T>,Class<U>,BiFunction<T,U,?>
    [javac]   found:    Class<String>,Class<List>,BiFunction<String,List,Object>
    [javac]   where T,U are type-variables:
    [javac]     T extends Object declared in method <T,U>of(Class<T>,Class<U>,BiFunction<T,U,?>)
    [javac]     U extends Object declared in method <T,U>of(Class<T>,Class<U>,BiFunction<T,U,?>)
    [javac] C:\SVN\josm\core\src\org\openstreetmap\josm\gui\mappaint\mapcss\ExpressionFactory.java:230: warning: [unchecked] unchecked method invocation: method of in interface Factory is applied to given types
    [javac]         FACTORY_MAP.put("sort_list", Factory.of(List.class, Functions::sort_list));
    [javac]                                                ^
    [javac]   required: Class<T>,Function<T,?>
    [javac]   found:    Class<List>,Function<List,Object>
    [javac]   where T is a type-variable:
    [javac]     T extends Object declared in method <T>of(Class<T>,Function<T,?>)
    [javac] C:\SVN\josm\core\src\org\openstreetmap\josm\gui\mappaint\mapcss\ExpressionFactory.java:249: warning: [unchecked] unchecked method invocation: method of in interface Factory is applied to given types
    [javac]         FACTORY_MAP.put("trim_list", Factory.of(List.class, Functions::trim_list));
    [javac]                                                ^
    [javac]   required: Class<T>,Function<T,?>
    [javac]   found:    Class<List>,Function<List,Object>
    [javac]   where T is a type-variable:
    [javac]     T extends Object declared in method <T>of(Class<T>,Function<T,?>)
    [javac] C:\SVN\josm\core\src\org\openstreetmap\josm\gui\mappaint\mapcss\ExpressionFactory.java:251: warning: [unchecked] unchecked method invocation: method of in interface Factory is applied to given types
    [javac]         FACTORY_MAP.put("uniq_list", Factory.of(List.class, Functions::uniq_list));
    [javac]                                                ^
    [javac]   required: Class<T>,Function<T,?>
    [javac]   found:    Class<List>,Function<List,Object>
    [javac]   where T is a type-variable:
    [javac]     T extends Object declared in method <T>of(Class<T>,Function<T,?>)
    [javac] WARNING: An illegal reflective access operation has occurred
    [javac] WARNING: Illegal reflective access by com.google.errorprone.util.ErrorProneTokens$CommentSavingTokenizer (file:/C:/Users/vippy/.ivy2/cache/com.google.errorprone/error_prone_check_api/jars/error_prone_check_api-2.5.1.jar) to field com.sun.tools.javac.parser.JavaTokenizer.reader
    [javac] WARNING: Please consider reporting this to the maintainers of com.google.errorprone.util.ErrorProneTokens$CommentSavingTokenizer
    [javac] WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
    [javac] WARNING: All illegal access operations will be denied in a future release
    [javac] C:\SVN\josm\core\src\org\openstreetmap\josm\data\validation\tests\MapCSSTagCheckerAsserts.java:128: warning: [UnusedVariable] The parameter 'insideMethod' is never read.
    [javac]     private static Optional<String> getFirstInsideCountry(MapCSSTagCheckerRule check, Method insideMethod) {
    [javac]                                                                                              ^
    [javac]     (see https://errorprone.info/bugpattern/UnusedVariable)
    [javac]   Did you mean 'Optional<String> inside = getFirstInsideCountry(check);'?
    [javac] C:\SVN\josm\core\src\org\openstreetmap\josm\gui\layer\geoimage\ImageDisplay.java:1015: warning: [UnusedVariable] The local variable 'oldEntry' is never read.
    [javac]         ImageEntry oldEntry;
    [javac]                    ^
    [javac]     (see https://errorprone.info/bugpattern/UnusedVariable)
    [javac]   Did you mean to remove this line?
    [javac] 7 warnings

[skyper]

So I am still using r17756 as I need the Styles/Lane_and_Road_Attributes quite often.

[simon04]

In 17792/josm:

see #20744 - MapCSSTagCheckerAsserts: remove unused code

[simon04]

In 17797/josm:

see #20744 - Fix MapCSS function is_prop_set

[simon04]

In 17803/josm:

see #20744 - @SuppressWarnings("unchecked")

[skyper]

Thanks, r17809 seems to work as expected, again.

[simon04]

Thank you for your extensive testing!

[simon04]

In 17832/josm:

see #20744, fix #20810 - MapCSS: fix NPE

[simon04]

In 17916/josm:

fix #20957, see #20744 - Fix MapCSS function round