Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Perf improvement - matchTags - locationsAt #8761

Merged
merged 2 commits into from
Oct 21, 2021
Merged

Conversation

mbrzakovic
Copy link
Collaborator

@mbrzakovic mbrzakovic commented Oct 20, 2021

Background

Preset matching is the computational logic that returns a strong type from entity tags.
My research is showing that the core method of this logic (matchTags) is a widely used method in iD, is being executed for majority of entities and the reasons are the following:

drawing:

  • drawPoints (to figure out POI icons)
  • drawLabels (to figure out if item should be labeled)
  • drawArea

validator:

  • otherMissmatchIssue
  • oldTagIssues

Optimizing the performance of this method is impactful task to overall iD performance.

Motivation

According to perf measurements and benchmarking linked bellow, the bottle-neck for this method is currently locationAt method.

  • Once location index is bootstraped (~10-20s after the app starts) doing locationAt is a heavy operation.

I've observed that performing locationsAt is not needed for majority of matchings as the best match does not have locationID dependency.

Change being made

This PR does simple reorganization of matchTag method in such a way that the locationsAt is only executed if needed.

  • Additionally and less important, I've also made a small change in mismatched-geometry validation, that should reduce the number of calls it makes to matchTag.

Results

Summary: Improved matchTags perf dramatically, and therefore reduces scripting time by ~30%
[edit: For densely populated areas, overall rendering response and scripting is more than x2 faster. Benchmark test here]

As benchmark test I did the following:

  • map=21.50/46.97156/-122.15791, wait 10s
    Start perf recording
  • map=18.10/47.60477/-122.32632, wait untill load
  • map=18.10/47.60656/-122.32914, wait untill load
  • map=17.50/47.61527/-122.32251
  • wait untill load
    End perf recording

The results are the following:

Before:

Total start-end recording time: 80s
dom rendering: 7s
scripting: 67s out of which:

  • 42s - _this.matchTags
    -- 30s - this.locationsAt
    -- 12s - matchScore and others

After (with this change):

Total start-end recording : 60s
scripting: 42s out of which:

  • 16s - _this.matchTags
  • <1s - this.locationsAt
    --15s - matchScore and others

Moving forward:

Notice the iD v2.19.6 results:
Total start-end recording time: 40s
dom rendering: 7s
scripting: 30s out of which:
- 4s - _this.matchTags

Why is this version of iD matchTags even faster?
The answer is twofold:

  1. The index for matching has greatly increased. Some numbers I've collected are saying that the geometryIndex is in current version x2.5 times larger (was: 25k items, now: 65k items) , which on average slows down the matching process by x2.
  2. As mentioned in 'Background' some validators are clients of preset matchings. In v2.20.2, we have more validations and they are running on a bit more entities so => the matching is being triggered more frequently.

Room for improvement:
It is very interesting hypothesis (and an amazing engineering problem :)) that the speed of preset matching logic can be further improved. Currently for a Shop entity, for each tag, code is doing matching with each possible strong type for that geometry type. e.g. point + amenity tag has >5k matchings to do. I will raise a brainstorming issue on this topic.

closes #8612

mismatched_geometry - small optimization
@mbrzakovic mbrzakovic added this to the 2.20.2 milestone Oct 20, 2021
@mbrzakovic mbrzakovic added the performance Optimizing for speed and efficiency label Oct 20, 2021
@mbrzakovic mbrzakovic requested a review from bhousel October 20, 2021 13:41
@westnordost
Copy link
Contributor

What is locationAt and what is it used for? It doesn't seem to be a property of a preset(?), at least I didn't find anything when searching for that string in the name-suggestion-index.

@mbrzakovic
Copy link
Collaborator Author

What is locationAt and what is it used for? It doesn't seem to be a property of a preset(?), at least I didn't find anything when searching for that string in the name-suggestion-index.

It is property of some presets. (look at locationSet ). Example I frequently use for explanation is 'Irish pub'. This preset should not be used if entity is in Ireland, and that is what the additional location matching logic is doing here.

@westnordost
Copy link
Contributor

(I by the way can't say/review anything about the PR as I don't know my way around the iD code. I just saw the the thread on slack and thought I can contribute something from my point of view since I wrote the osmfeatures library which (also) has some optimizations for looking up presets implemented.)

Copy link
Member

@bhousel bhousel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good - just one thing to check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Optimizing for speed and efficiency
Projects
None yet
Development

Successfully merging this pull request may close these issues.

iD performance issues since version 2.20.0
3 participants