Skip to content

Conversation

@nathreed
Copy link
Contributor

@nathreed nathreed commented May 30, 2024

This PR cherry-picks @xabbu42's work from mapbox/vector-tile#56. Discussion in the issue mapbox/vector-tile#55 indicated that mapbox/vector-tile#53 also fixed the issue while making somewhat unrelated changes.

There was discussion in the issue about the performance implications of this fix. It seemed like it was mostly in hypotheticals, although @xabbu42 got some real-world data and found that this fix should be at worst slightly slower and in some cases faster than the previous implementation. I decided to use this fix instead of mapbox/vector-tile#53 as a more conservative approach because it fixes the specific issue at hand without introducing other changes to behavior.

Previous maintainers had some concern about doing linear search in getValue, but it's unclear how much of this was just trying to have the most optimal algorithmic performance vs. a real-world issue they were aware of. It seems like the numbers we are dealing with here are typically small in absolute terms, so it's unclear how much difference it would make to have an algorithmically more optimal solution.

I suggest that we merge this, and if this change ends up causing performance problems in MapLibre Native, we evaluate the impact of merging mapbox/vector-tile#53 and reverting this change.


Note: In the process of cherry-picking the changes from the outstanding upstream PR, I noticed that there was one commit from the previous maintainers last year, that MapLibre Native hadn't updated the submodule reference to include. It appears to fix this issue in a different way, though I haven't tested to confirm. I wasn't aware of this fix until I started making this PR, because nobody mentioned it on any of the originally reported issues nor replied to comments on the PRs after a week (that's why I suggested MapLibre fork it). They also didn't tag a new version with this fix, so I'm not sure if they considered it complete or not.

I reverted the upstream change to apply this one because they conflicted, and I chose this change because it's been discussed more in the various issues so it seems better understood, and it was authored by people (xabbu42) who seem more interested in the MapLibre community (also because since a new version wasn't tagged, I wasn't sure if they considered the fix complete, though it does appear to be).

However, it does seem like we could simply bump the submodule reference in MapLibre Native to the latest commit on this repo, without merging this PR, and the issue would likely be fixed with the change that occurred in the original repo. I still think it's good for MapLibre to have control over this dependency given the very low level of activity/responsiveness on the upstream, but we might not need to apply this particular PR to fix the issue.

Copy link
Member

@louwers louwers left a comment

Choose a reason for hiding this comment

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

Thank you for the detailed description of events.

I agree with your reasoning. I'll pin MapLibre Native to this fork after merging, and see if this has a noticeable impact on the benchmarks.

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