Modify

Opened 5 weeks ago

Closed 3 days ago

#18802 closed enhancement (fixed)

MapCSS: refactor/simplify/improve

Reported by: simon04 Owned by: simon04
Priority: normal Milestone: 20.03
Component: Core Version:
Keywords: mapcss yourkit performance Cc: taylor.smock

Description

commit 1ab7320176f8a0ab4868297bd36b375855826ed1
Author: Simon Legner <Simon.Legner@gmail.com>
Date:   2020-02-23 23:11:32 +0100

    Extract MapCSSTagCheckerAsserts class

 .../data/validation/tests/MapCSSTagChecker.java    | 162 +-------------------
 .../validation/tests/MapCSSTagCheckerAsserts.java  | 163 +++++++++++++++++++++
 .../validation/tests/MapCSSTagCheckerTest.java     |   6 +-
 3 files changed, 168 insertions(+), 163 deletions(-)

commit 7617b34a06075cd6ea41fc4da708c4f87d6be5f6
Author: Simon Legner <Simon.Legner@gmail.com>
Date:   2020-02-23 23:22:07 +0100

    MapCSSTagChecker: simplify throwError/throwWarning/throwOther parsing

 .../data/validation/tests/MapCSSTagChecker.java    | 32 +++++++---------------
 1 file changed, 10 insertions(+), 22 deletions(-)

commit e17190633b0cf657690cc853cfa73da2ad5d1cb6
Author: Simon Legner <Simon.Legner@gmail.com>
Date:   2020-02-24 20:21:08 +0100

    see #18749 - MapCSSTagChecker: no not persist assertions
    
    Check assertions immediately, when needed, but do not keep them.

 .../data/validation/tests/MapCSSTagChecker.java    | 40 +++++++----
 .../validation/tests/MapCSSTagCheckerAsserts.java  | 82 ++++++++++++----------
 .../validation/tests/MapCSSTagCheckerTest.java     | 44 +++++++++---
 3 files changed, 101 insertions(+), 65 deletions(-)

commit 30c41b2583c3b0fa82f6f7c33d479879c713ffa4
Author: Simon Legner <Simon.Legner@gmail.com>
Date:   2020-02-25 17:54:33 +0100

    see #18749 - MapCSSTagChecker.TagCheck: use unmodifiable collections

 .../data/validation/tests/MapCSSTagChecker.java    | 28 ++++++++++++++++++----
 src/org/openstreetmap/josm/tools/Utils.java        | 17 +++++++++++++
 .../org/openstreetmap/josm/tools/UtilsTest.java    | 22 +++++++++++++++++
 3 files changed, 62 insertions(+), 5 deletions(-)

commit 88775968c3e21a62fdef7fdb46016f707dc8081a
Author: Simon Legner <Simon.Legner@gmail.com>
Date:   2020-02-25 23:26:35 +0100

    Extract org.openstreetmap.josm.gui.mappaint.mapcss.Declaration

 .../data/validation/tests/MapCSSTagChecker.java    |  2 +-
 .../josm/gui/mappaint/mapcss/Declaration.java      | 65 ++++++++++++++++++++++
 .../josm/gui/mappaint/mapcss/Instruction.java      |  2 +-
 .../josm/gui/mappaint/mapcss/MapCSSParser.jj       |  2 +-
 .../josm/gui/mappaint/mapcss/MapCSSRule.java       | 60 --------------------
 5 files changed, 68 insertions(+), 63 deletions(-)

commit 01200e3a5ed79411f59158729e54de1cc7731883
Author: Simon Legner <Simon.Legner@gmail.com>
Date:   2020-02-26 00:11:37 +0100

    Add MapCSSRule.matches

 scripts/TagInfoExtract.java                                |  2 +-
 .../josm/data/validation/tests/MapCSSTagChecker.java       |  2 +-
 .../openstreetmap/josm/gui/mappaint/mapcss/MapCSSRule.java | 14 ++++++++++++++
 .../josm/gui/mappaint/mapcss/MapCSSParserTest.java         |  8 ++++----
 4 files changed, 20 insertions(+), 6 deletions(-)

commit 8dd2df58103a06f37b751ebde70509aa8ce7b1e4
Author: Simon Legner <Simon.Legner@gmail.com>
Date:   2020-02-26 00:35:14 +0100

    Add Selector.getConditions

 scripts/TagInfoExtract.java                             |  6 +-----
 .../josm/gui/mappaint/mapcss/Selector.java              | 17 +++++++++++++----
 2 files changed, 14 insertions(+), 9 deletions(-)

commit edc421183618289ad735ff4d05dc2ee84c013c31
Author: Simon Legner <Simon.Legner@gmail.com>
Date:   2020-02-26 01:08:32 +0100

    Add Selector.getBase

 .../validation/tests/MapCSSTagCheckerIndex.java    | 10 ++----
 .../gui/mappaint/mapcss/MapCSSStyleSource.java     | 41 ++++++----------------
 .../josm/gui/mappaint/mapcss/Selector.java         | 18 ++++++++--
 3 files changed, 29 insertions(+), 40 deletions(-)

commit 45a243e2df32c2eed6a6febfa6687808759d3d88
Author: Simon Legner <Simon.Legner@gmail.com>
Date:   2020-02-26 23:03:13 +0100

    Selector: merge GeneralSelector/OptimizedGeneralSelector as there is no difference
    
    - GeneralSelector extended OptimizedGeneralSelector
    - GeneralSelector.optimizedBaseCheck returned OptimizedGeneralSelector === this

 .../data/validation/tests/MapCSSTagChecker.java    | 10 +--
 .../validation/tests/MapCSSTagCheckerIndex.java    |  2 +-
 .../gui/mappaint/mapcss/MapCSSStyleSource.java     |  7 +-
 .../josm/gui/mappaint/mapcss/Selector.java         | 82 +++++-----------------
 .../gui/mappaint/MapRendererPerformanceTest.java   |  2 +-
 5 files changed, 27 insertions(+), 76 deletions(-)

commit 92e89d1bfd3d01ab7ae4ba142807210ada14f13d
Author: Simon Legner <Simon.Legner@gmail.com>
Date:   2020-02-27 00:17:24 +0100

    MapCSSRule: support list of selectors
    
    - allows to drop MapCSSTagChecker.GroupedMapCSSRule
    - remove code duplication in MapCSSStyleSource/MapCSSTagCheckerIndex w.r.t. MapCSSRuleIndex

 scripts/TagInfoExtract.java                        |   3 +-
 .../data/validation/tests/MapCSSTagChecker.java    |  93 +-----
 .../validation/tests/MapCSSTagCheckerIndex.java    | 372 +++++++++++----------
 .../josm/gui/mappaint/mapcss/MapCSSParser.jj       |   6 +-
 .../josm/gui/mappaint/mapcss/MapCSSRule.java       |  17 +-
 .../gui/mappaint/mapcss/MapCSSStyleSource.java     | 172 +++-------
 .../mappaint/mapcss/ChildOrParentSelectorTest.java |   2 +-
 7 files changed, 276 insertions(+), 389 deletions(-)

commit ebdfa3d0bb10cc0017eab130bf1f1ec9dfbc9c1a
Author: Simon Legner <Simon.Legner@gmail.com>
Date:   2020-02-27 00:29:09 +0100

    MapCSSStyleIndex: rename/move from index

 .../data/validation/tests/MapCSSTagChecker.java    | 32 +++++++++++++---
 .../mappaint/mapcss/MapCSSStyleIndex.java}         | 43 ++--------------------
 .../gui/mappaint/mapcss/MapCSSStyleSource.java     |  3 +-
 3 files changed, 31 insertions(+), 47 deletions(-)

commit 710d40fdc99fd01bf6eb8aa8237475475e7b4d0a
Author: Simon Legner <Simon.Legner@gmail.com>
Date:   2020-02-27 23:29:25 +0100

    see #18749 - KeyValueRegexpCondition: do not store value as string

 src/org/openstreetmap/josm/gui/mappaint/mapcss/ConditionFactory.java | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Attachments (1)

18802.patch (98.3 KB) - added by simon04 5 weeks ago.

Download all attachments as: .zip

Change History (34)

Changed 5 weeks ago by simon04

Attachment: 18802.patch added

comment:1 Changed 5 weeks ago by simon04

Summary: MapCSS: refactor/simplify/improve[Patch] MapCSS: refactor/simplify/improve

comment:2 Changed 4 weeks ago by simon04

In 15979/josm:

see #18802 - Extract MapCSSTagCheckerAsserts class

comment:3 Changed 4 weeks ago by simon04

In 15980/josm:

see #18802 - MapCSSTagChecker: simplify throwError/throwWarning/throwOther parsing

comment:4 Changed 4 weeks ago by simon04

In 15981/josm:

see #18802 - MapCSSTagChecker: no not persist assertions

Check assertions immediately, when needed, but do not keep them.

comment:5 Changed 4 weeks ago by simon04

In 15982/josm:

see #18802 - MapCSSTagChecker.TagCheck: use unmodifiable collections

comment:6 Changed 4 weeks ago by simon04

In 15983/josm:

see #18802 - Extract org.openstreetmap.josm.gui.mappaint.mapcss.Declaration

comment:7 Changed 4 weeks ago by simon04

In 15984/josm:

see #18802 - Add MapCSSRule.matches

comment:8 Changed 4 weeks ago by simon04

In 15985/josm:

see #18802 - Add Selector.getConditions

comment:9 Changed 4 weeks ago by simon04

In 15986/josm:

see #18802 - Add Selector.getBase

comment:10 Changed 4 weeks ago by simon04

In 15987/josm:

see #18802 - Selector: merge GeneralSelector/OptimizedGeneralSelector as there is no difference

  • GeneralSelector extended OptimizedGeneralSelector
  • GeneralSelector.optimizedBaseCheck returned OptimizedGeneralSelector === this

comment:11 Changed 4 weeks ago by simon04

In 15988/josm:

see #18802 - MapCSSRule: support list of selectors

  • allows to drop MapCSSTagChecker.GroupedMapCSSRule
  • remove code duplication in MapCSSStyleSource/MapCSSTagCheckerIndex w.r.t. MapCSSRuleIndex

comment:12 Changed 4 weeks ago by simon04

In 15989/josm:

see #18802 - MapCSSStyleIndex: rename/move

comment:13 Changed 4 weeks ago by simon04

In 15990/josm:

see #18802 - KeyValueRegexpCondition: do not store value as string

comment:14 Changed 4 weeks ago by simon04

In 15991/josm:

see #18802 - MapCSSStyleIndex: reuse rule if base is the same

comment:15 Changed 4 weeks ago by simon04

Retained size comparison:

  • r15937: org.openstreetmap.josm.data.validation.tests.MapCSSTagChecker 1_092_016
  • r15991: org.openstreetmap.josm.data.validation.tests.MapCSSTagChecker 929_656

comment:16 Changed 4 weeks ago by simon04

Keywords: yourkit added

comment:17 Changed 4 weeks ago by GerdP

In 15995/josm:

see #18802, #13165: Simplify code

comment:18 Changed 4 weeks ago by GerdP

Nice :) I tried that a while ago and failed because of the complexity.
There are a few new complains like unused private method hasSameDeclaration(), I assume this is still work in progress?
See also my new patch for #13165.

comment:19 Changed 4 weeks ago by simon04

In 15997/josm:

see #18802 - MapCSSTagChecker: remove unused private function

comment:20 Changed 4 weeks ago by simon04

In 15998/josm:

see #18802 - MapCSSTagChecker: use Stream API

comment:21 Changed 4 weeks ago by simon04

In 15999/josm:

see #18802 - MapCSSTagChecker: fix "Variable is already assigned to this value"

comment:22 Changed 4 weeks ago by simon04

Test org.openstreetmap.josm.gui.mappaint.MapCSSRendererTest.testRender[relation-linkselector] broken (most likely) due to r15986 (found using git bisect)

comment:23 Changed 4 weeks ago by simon04

In 16000/josm:

see #18802 - fix MapCSSRendererTest.testRender[relation-linkselector]

comment:24 Changed 4 weeks ago by Don-vip

In 16003/josm:

see #18802 - fix javadoc error

comment:25 Changed 4 weeks ago by GerdP

I wonder when ruleToCheckMap should be cleared. IIGTR there might be a problem when rules are reloaded.

comment:26 Changed 4 weeks ago by GerdP

Not a big problem, but it is a memory leak. I think it should be cleared in endTest()

comment:27 Changed 4 weeks ago by simon04

In 16039/josm:

see #18802 - MapCSSTagChecker: clear ruleToCheckMap cache

comment:28 Changed 4 weeks ago by Don-vip

Cc: taylor.smock added

Test failure: org.openstreetmap.josm.gui.preferences.map.MapPaintPreferenceTestIT.testStyleValidity[MapWithAI - https://josm.openstreetmap.de/wiki/Styles/MapWithAI]

junit.framework.AssertionFailedError: [java.lang.IllegalArgumentException: Unknown settings group: show_all]
[]
	at org.openstreetmap.josm.gui.preferences.map.MapPaintPreferenceTestIT.testStyleValidity(MapPaintPreferenceTestIT.java:110)

Is it linked to this ticket?

comment:29 in reply to:  28 Changed 4 weeks ago by taylor.smock

Replying to Don-vip:

Test failure: org.openstreetmap.josm.gui.preferences.map.MapPaintPreferenceTestIT.testStyleValidity[MapWithAI - https://josm.openstreetmap.de/wiki/Styles/MapWithAI]

junit.framework.AssertionFailedError: [java.lang.IllegalArgumentException: Unknown settings group: show_all]
[]
	at org.openstreetmap.josm.gui.preferences.map.MapPaintPreferenceTestIT.testStyleValidity(MapPaintPreferenceTestIT.java:110)

Is it linked to this ticket?

I have no clue -- I cannot reproduce (the test is passing in Eclipse, via command line (full test run in a clean josm source directory), and via a Ubuntu VM using my working josm directory, with JUnit 5).

Note: I ran the tests with r16048, which had the test fail on Jenkins.

EDIT: I just made some modifications to the MapWithAI paint style. Based off of the feedback from the test (Unknown settings group: show_all), I removed the @supports (min-josm-version: 15289) just for testing purposes. It doesn't make a major difference for the paint style, since its initial revision was 15229.

My guess is that the test doesn't understand @supports(min-josm-version: VERSION) when running under Jenkins CI. Just to double check, I am rerunning the whole test suite in an Ubuntu VM, which should (largely) replicate the Jenkins environment (this time, in a clean working directory).

Last edited 4 weeks ago by taylor.smock (previous) (diff)

comment:31 Changed 3 weeks ago by simon04

Keywords: performance added
Summary: [Patch] MapCSS: refactor/simplify/improveMapCSS: refactor/simplify/improve

comment:32 Changed 2 weeks ago by taylor.smock

I forgot to look at this bug report the day after I made the modification to the MapWithAI paint style, and Jenkins looks like it doesn't keep much more than the past week of commits.

That being said, the Integration Test is now passing. I'm going to go ahead and revert the change I made to the paint style. I hope I remember to look at the tests tomorrow.

comment:33 Changed 3 days ago by simon04

Resolution: fixed
Status: assignedclosed

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.