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

Replace MultiIterable usages with Sequence #2504

Merged
merged 5 commits into from
Jan 17, 2021
Merged

Replace MultiIterable usages with Sequence #2504

merged 5 commits into from
Jan 17, 2021

Conversation

FloEdelmann
Copy link
Member

As suggested in #2493 (comment).

It seems to work fine this way, but I'm not sure if I decreased the performance by unnecessary iterating/copying. If I understand Sequences in Kotlin correctly though, it should behave as wanted.

Copy link
Member

@smichel17 smichel17 left a comment

Choose a reason for hiding this comment

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

I think this is a little more kotlin-y

Comment on lines 18 to 22
var elements = sequenceOf<Element>()
if (filter.includesElementType(Element.Type.NODE)) elements += mapData.nodes
if (filter.includesElementType(Element.Type.WAY)) elements += mapData.ways
if (filter.includesElementType(Element.Type.RELATION)) elements += mapData.relations
return elements.filter { element -> filter.matches(element) }.asIterable()
Copy link
Member

Choose a reason for hiding this comment

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

I am personally allergic to if statements without braces (except when replacing the ternary operator), and using them on one liners like this makes them very busy. Here's a rewrite to avoid that, and use a more functional style:

        val elements = mapOf(
            Element.Type.NODE to mapData.nodes,
            Element.Type.WAY to mapData.ways,
            Element.Type.RELATION to mapData.relations
        ).filterKeys(filter::includesElementType).values.flatten()
        return elements.asSequence().filter(filter::matches).asIterable()

Copy link
Member

@smichel17 smichel17 Jan 17, 2021

Choose a reason for hiding this comment

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

If you don't like using the function references (understandable — they work by reflection and smell faintly of magic):

        ).filterKeys { filter.includesElementType(it) }.values.flatten()
        return elements.asSequence().filter{ filter.matches(it) }.asIterable()

Copy link
Member

Choose a reason for hiding this comment

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

Alternative version using sequence() (maybe better performance?)

        return sequence {
            mapOf(
                Element.Type.NODE to mapData.nodes,
                Element.Type.WAY to mapData.ways,
                Element.Type.RELATION to mapData.relations
            ).filterKeys(filter::includesElementType).forEach { yieldAll(it.value) }
        }.filter(filter::matches).asIterable()

Copy link
Member

@westnordost westnordost Jan 17, 2021

Choose a reason for hiding this comment

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

Whoa, really? Readability aside, you:

  1. mapOf: create a hashmap and add three values
  2. filterKeys: you create yet another new hashmap with only the keys added that return true on filter::includesElementType
  3. .values: you access the value set of the hashmap (Not sure if this requires extra processing. It would if the value set is created lazily or if it is not a view onto the hashmap but a copy of all values in it)
  4. flatten: you create a new list of these (all the values in the map) by iterating through them all and adding them all to an array list
  5. since in step 4, you already copied the complete lists, the last line doesn't really make sense

That's a lot of transforming and copying of data just to avoid few ifs.

Your last suggestion would at least work as far as I understand because you don't flatten everything into a list.

Copy link
Member

Choose a reason for hiding this comment

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

On the sequence { } syntax, I think it would look like this then. Since anyone who reads this also needs to understand sequences, this would be fine too. Not sure if flatten keeps the sequence a sequence, it should otherwise the below code doesn't make sense.

return sequence {
  if (filter.includesElementType(Element.Type.NODE)) yield(mapData.nodes)
  if (filter.includesElementType(Element.Type.WAY)) yield(mapData.ways)
  if (filter.includesElementType(Element.Type.RELATION)) yield(mapData.relations)
}.flatten().filter(filter::matches).asIterable()

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if flatten keeps the sequence a sequence

It does: https://github.com/JetBrains/kotlin/blob/8fa848bed384568a8ceb9731ec9d45663aa64521/libraries/stdlib/src/kotlin/collections/Sequences.kt#L94-L99

What about yieldAll though?

        return sequence {
            if (filter.includesElementType(Element.Type.NODE)) yieldAll(mapData.nodes)
            if (filter.includesElementType(Element.Type.WAY)) yieldAll(mapData.ways)
            if (filter.includesElementType(Element.Type.RELATION)) yieldAll(mapData.relations)
        }.filter { filter.matches(it) }.asIterable()

Copy link
Member

Choose a reason for hiding this comment

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

Right, that should work too. (Disclaimer: I am not an expert on Kotlin sequences)

Copy link
Member

@smichel17 smichel17 Jan 17, 2021

Choose a reason for hiding this comment

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

Well, I did phrase it as a question (maybe better performance? ). Disclaimer: I am also not an expert on Kotlin sequences, although I re-read most of the documentation before writing my version, so some details are fresh in my mind.

4) This is mostly what I was talking about — that my second version was more efficient than the first version, because it avoids flatten() on a list, which will evaluate eagerly.

5) Objects are copied by reference in java, so while there is a performance impact, it may be smaller than it seems. Thus, a lazy filter may still provide a meaningful performance impact. It depends on the cost of running filter.matches.

1-3) Because of the small number of elements (3 max), this is essentially a fixed cost. I don't know exactly how big that cost (2x hash map creation, up to 6 hash map insertions, up to 3 hash map accesses) is. I did not consider it in my performance analysis, but it may have a meaningful impact.

However, the main reason I say "maybe" is that I don't know the implementation of elements += mapData.nodes. In particular, **I do not know if it is eagerly copying the nodes into an array inside the elements sequence (which would then be evaluated lazily), or just holding a reference to mapData.nodes. I have the same question about yieldAll(), although I would guess that it is more likely to be optimized in this way. This is the key answer needed to analyze the efficiency.

The block that you pass to sequence() is a suspend function (docs here). Each time you ask for values in the sequence, the block runs only up until you yield enough values, then suspends. There is some cost to suspending (should be low, since coroutines are lightweight, but I don't know how lightweight), and a memory cost to capture the closure.


So this is why I am uncertain. There are many different unknown variables.

I suspect that the most efficient approach is either the latest @FloEdelmann wrote, with ifs and yieldAll(), or

        return sequenceOf(
            Pair(Element.Type.NODE, mapData.nodes),
            Pair(Element.Type.WAY, mapData.ways),
            Pair(Element.Type.RELATION, mapData.relations)
        )
            .filter { (type, _) -> filter.includesElementType(type) }
            .flatMap { (_, elements) -> elements }
            .filter { filter.matches(it) }
            .asIterable()

I considered writing it this way initially, but I thought the map version was more readable than a list of pairs.

@FloEdelmann
Copy link
Member Author

@westnordost Your opinion on @smichel17's suggestions?

@westnordost
Copy link
Member

I think they make it harder to read. To know about what all those kotlin flow functions (or how'd you call them) do requires additional expertise.
Chaining such functions makes it harder to understand for each transformation of the data added.

To know that an if statement does not need braces if there is just one statement that follows, not - to boot, this (if statements don't always need braces) is how the code in all of the app is written.

Co-Authored-By: smichel17 <github@smichel.me>
@smichel17
Copy link
Member

to boot, this (if statements don't always need braces) is how the code in all of the app is written.

Yes, I know… not being happy just sticking to an existing style for consistency's sake is a weakness of mine as a programmer.

To know about what all those kotlin flow functions (or how'd you call them) do requires additional expertise.

In my opinion, if statements without braces are easier to miss or misinterpret when scanning to understand the control flow. Thus, it is easier to accidentally change them to something unintended. Also, if you want to add a second statement after the if, you need to change the entire line, making the git commit history messier. Yes, it does require additional expertise. But so do static types, and I think you would agree they are useful. So it is just a difference of opinion about whether the trade-off is worth it.

I think we've had this conversation before, so I will not say anything more on the topic. (You are still welcome to respond — I don't want to insist on myself having the final word.)

Copy link
Member

@smichel17 smichel17 left a comment

Choose a reason for hiding this comment

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

Oop, forgot to comment as a "review"

@westnordost westnordost merged commit b509b39 into streetcomplete:master Jan 17, 2021
@FloEdelmann FloEdelmann deleted the iterable branch January 17, 2021 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants