Modify

Opened 14 months ago

Closed 13 months ago

Last modified 12 months ago

#20744 closed enhancement (fixed)

Evaluate MapCSS expression without array creation

Reported by: simon04 Owned by: simon04
Priority: normal Milestone: 21.04
Component: Core Version:
Keywords: mapcss reflection lambda java8 Cc: Don-vip, Klumbumbus

Description

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

Attachments (0)

Change History (25)

comment:1 Changed 14 months ago by Don-vip

The code is easier to understand, +1 :)

comment:2 Changed 14 months ago by 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).

comment:3 in reply to:  2 Changed 14 months ago by 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 ...

comment:4 Changed 14 months ago by simon04

Resolution: fixed
Status: assignedclosed

In 17758/josm:

fix #20744 - Evaluate MapCSS expression without array creation

comment:5 Changed 14 months ago by simon04

In 17762/josm:

see #20744 - Evaluate MapCSS pseudo classes without array creation

comment:6 Changed 14 months ago by gaben

Resolution: fixed
Status: closedreopened

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

comment:7 Changed 14 months ago by simon04

In 17764/josm:

see #20744 - Fix MapCSS function prop

Regression of r17758.

comment:8 Changed 14 months ago by simon04

In 17765/josm:

see #20744 - Remove NO_UCD (unused code)

comment:9 in reply to:  6 Changed 14 months ago by gaben

Replying to gaben:

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

See ticket:20739#comment:6.

comment:10 Changed 14 months ago by simon04

In 17769/josm:

see #20744 - MapCSS: fix unary minus

Regression of r17758.

comment:11 Changed 14 months ago by simon04

In 17770/josm:

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

Regression of r17758.

comment:12 Changed 14 months ago by simon04

In 17774/josm:

see #20744 - Fix ConditionFactoryTest ($jacocoInit)

comment:13 Changed 14 months ago by skyper

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

Last edited 14 months ago by skyper (previous) (diff)

comment:14 Changed 14 months ago by Klumbumbus

Cc: Klumbumbus added

comment:15 Changed 14 months ago by simon04

In 17780/josm:

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

comment:16 Changed 14 months ago by simon04

In 17781/josm:

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

comment:17 Changed 14 months ago by 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

comment:18 Changed 13 months ago by skyper

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

comment:19 Changed 13 months ago by simon04

In 17792/josm:

see #20744 - MapCSSTagCheckerAsserts: remove unused code

comment:20 Changed 13 months ago by simon04

In 17797/josm:

see #20744 - Fix MapCSS function is_prop_set

comment:21 Changed 13 months ago by simon04

In 17803/josm:

see #20744 - @SuppressWarnings("unchecked")

comment:22 Changed 13 months ago by skyper

Thanks, r17809 seems to work as expected, again.

comment:23 Changed 13 months ago by simon04

Resolution: fixed
Status: reopenedclosed

Thank you for your extensive testing!

comment:24 Changed 13 months ago by simon04

In 17832/josm:

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

comment:25 Changed 12 months ago by simon04

In 17916/josm:

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

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain simon04.
as The resolution will be set.
The resolution will be deleted.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.