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

improve performance when scanning for quests #4469

Merged
merged 3 commits into from
Oct 6, 2022

Conversation

Helium314
Copy link
Collaborator

OsmFilterQuestType quests always uses element tags, so there is no need to check whether an element without tags is applicable.
This change results in ~15-30% faster creation of quests in a bbox.

@westnordost
Copy link
Member

westnordost commented Oct 6, 2022

I am surprised that this results in that much of a performance increase. After all, this should just be the reduction of iterations, since the ElementFilterExpression already skips elements without no tags if the expression allows it (see mayEvaluateToTrueWithNoTags). I guess, Kleinvieh macht auch Mist... with close to 100 filter quest types...

But anyway, from the technical point of view, the PR is kind of a hack. There is nothing inherent in OsmFilterQuestType that guarantees that there will never be a quest that also creates quests for elements with no tags.

However, this can be done in a less hacky way by making mayEvaluateToTrueWithNoTags public and then checking if(questType is OsmFilterQuestType && !questType.filter.mayEvaluateToTrueWithNoTags) doTheShortcut()

Copy link
Member

@westnordost westnordost left a comment

Choose a reason for hiding this comment

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

see previous comment. Also, better add a short comment that explains what and why it is being done. I'd suggest to also rename elementsWithTags to onlyElementsWithTags

@westnordost westnordost added the feedback required more info is needed, issue will be likely closed if it is not provided label Oct 6, 2022
@westnordost westnordost removed the feedback required more info is needed, issue will be likely closed if it is not provided label Oct 6, 2022
Copy link
Member

@westnordost westnordost left a comment

Choose a reason for hiding this comment

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

👍 Nice find and implementation by the way! Such a performance gain for just 10 lines of code is pretty neat!

@westnordost westnordost merged commit 9cf6485 into streetcomplete:master Oct 6, 2022
@westnordost westnordost deleted the Helium314-patch-2 branch October 6, 2022 20:55
@FloEdelmann FloEdelmann added the hacktoberfest-accepted pull request that should be treated as eligible for Hacktoberfest event label Oct 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted pull request that should be treated as eligible for Hacktoberfest event
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants