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

Fix race conditions in symbol querying #6461

Closed
wants to merge 4 commits into from
Closed

Conversation

ChrisLoer
Copy link
Contributor

@ChrisLoer ChrisLoer commented Apr 5, 2018

Addresses hover flicker from issues #5887 and #5506.
Moves all data necessary for symbol querying into FeatureIndex.
Retains old FeatureIndexes if they're associated with a bucket that was used in the current placement.

The approach to fixing these bugs is unfortunately a bit complicated: because GL JS supports "incremental placement", it's possible that one CollisionIndex (which we use for querying rendered symbols) is built with data from multiple different versions of the same tile. Placement can't be interrupted for a single bucket of a single tile, so what we do here is hold on to all the data we need to query each bucket that gets included in a given placement. We piggy-back on the existing bucketInstanceId mechanism in CrossTileSymbolIndex that guarantees a unique ID number for each new version of a bucket.

In the case where one tile has multiple FeatureIndexes (because tile updates have arrived in the middle of an incremental placement), we run our symbol query against each of those indices separately, filtering CollisionIndex results against the set of bucket ids associated with each FeatureIndex.

Although this is a pretty inefficient way to store/query what is for the most part a bunch of duplicate data, I'm not too worried about it because it's just a temporary added cost during the period of time between a tile update and the next placement. This PR does introduce a lot of shared pointers to data, though -- one way it could go wrong would be to start leaking that data.

I've manually tested this branch against the repro case in #5506, but I haven't come up with any automated tests yet. Although they'll be a bit contrived, I think I can at least add some cases to the Tile unit test that try to exercise the functionality.

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • document any changes to public APIs
  • Ran benchmarks, nothing jumped out. How do we post now that anonymous gists are broken?
  • manually test the debug page

/cc @ansis @malwoodsantoro @ryanbaumann @lilykaiser

@ChrisLoer
Copy link
Contributor Author

I added a hover effect to four bus icons in debug/debug.html to make it easier to manually test this functionality. I also set up verbose local logging of the bucket retention/pruning behavior and through that found a problem in my initial implementation: data that had arrived while one placement was in progress but wasn't included in that placement would get used successfully in the next placement, but it would get pruned from retainedFeatureIndexes before that placement happened, which would make it disappear from future query results.

Addresses hover flicker from issues #5887 and #5506.
Moves all data necessary for symbol querying into FeatureIndex.
Retains old FeatureIndexes if they're associated with a bucket that was used in the current placement.
Unfortunately the interesting logic is coupled with CrossTileSymbolIndex and Placement.
…bucket that's waiting to be used in the next placement.

Even though the data will still be held by the tile, it won't be re-added to the retention list the next time the CrossTileSymbolIndex updates, which means it'll be invisible for querying purposes after the next placement completes.
@ChrisLoer
Copy link
Contributor Author

I've been working on addressing issue #5475 (stop doing queryRenderedSymbols on a per-tile basis) which looks like it may make a fair bit of this code (the tile-based querying and feature index pruning) obsolete. The approach that looks promising to me is to double down on bucketInstanceId as the way to track which data should be retained and included in queries, but with all the ownership managed by CrossTileSymbolIndex or some other mostly-global data structure. I think it should still be worthwhile to merge this as-is while I work on that larger refactoring.

@ChrisLoer ChrisLoer mentioned this pull request Apr 11, 2018
5 tasks
@ChrisLoer
Copy link
Contributor Author

Looks like this will be superseded by PR #6497.

@ChrisLoer ChrisLoer closed this Apr 13, 2018
@ChrisLoer ChrisLoer deleted the anti-hover-flicker branch April 13, 2018 00:25
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.

1 participant