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

Prioritize performance in vtile decoding #194

Merged
merged 6 commits into from
Apr 12, 2016
Merged

Prioritize performance in vtile decoding #194

merged 6 commits into from
Apr 12, 2016

Conversation

springmeyer
Copy link
Contributor

This moves the previously hardcoded v1->v2 upgrade and full tile recursion for vtile schema validation to opt-in (disabled by default).

This will unlock being able to render both existing v1 and future v2 tiles to images with AGG without an extra performance cost compared to node-mapnik v3.4.x (for this code path the spec version and v2 polygon validity don't matter).

  • When validation and JIT migration of v1->v2 matter these options can be turned back on by the calling application.
  • Layer version validation moved one level up because the validation code is currently agnostic to the spec version. The one case that was previously unchecked for v1 (LAYER_HAS_NO_FEATURES) makes just as much sense to check for v1 now that validation is an opt-in valid-add rather than something blocking the ability to decode an otherwise valid tile.

/cc @yhahn @jakepruitt @flippmoke @mapsam @GretaCB

Before merging:

t.z(),
true));
}
else if (validate && version != 1)

Choose a reason for hiding this comment

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

@springmeyer did you mean upgrade && version != 1? If so, I think we should probably just quietly ignore the situation of upgrade && version == 2, but probably throw or warn when upgrade && version == 3 or something along those lines comes up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jakepruitt - good eye, yes I did mean upgrade && version != 1 and therefore this code currently will throw when you try to upgrade a v2 tile, since that is pointless. But I agree that quietly ignoring this situation would likely be more convenient for calling applications. Otherwise they'd need to check the version themselves and toggle the option to avoid errors in mixed tile cases, which would be painful. So I will change to be quiet. Thanks!

@pnorman
Copy link
Contributor

pnorman commented Apr 11, 2016

Looking forward to when v3 comes out, is this the best API?

If we have the same scenario where the vector tiles can be upgraded, there's no way to know if the calling application wants a v1->v2 upgrade or wants everything upgraded to v3.

@springmeyer
Copy link
Contributor Author

Looking forward to when v3 comes out, is this the best API?

@pnorman - I've had the same thought during this code review: that this API is currently not a robust one for allowing fine grained control over upgrades going forward. But that is not something I'm working on currently, so I've attempted to make the minimal changes necessary in the pull to get the upgrade and validation behavior exposed as options.

@flippmoke flippmoke merged commit 8c0a27e into master Apr 12, 2016
@flippmoke flippmoke deleted the upgrade-opt-in branch April 12, 2016 19:02
@springmeyer springmeyer restored the upgrade-opt-in branch April 12, 2016 19:07
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.

None yet

4 participants