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

Skip style validation when loading a style from mapbox.com #3152

Closed
jfirebaugh opened this issue Sep 6, 2016 · 9 comments · Fixed by #3224
Closed

Skip style validation when loading a style from mapbox.com #3152

jfirebaugh opened this issue Sep 6, 2016 · 9 comments · Fixed by #3224
Labels
performance ⚡ Speed, stability, CPU usage, memory usage, or power usage

Comments

@jfirebaugh
Copy link
Contributor

In looking into #2991, I noticed that overall, style validation consumes a non-neglible amount of resources at startup. In addition to #3149 and #3151, for styles loaded via mapbox.com APIs, we should skip validation entirely, because the APIs themselves enforce validity.

@jfirebaugh jfirebaugh added the performance ⚡ Speed, stability, CPU usage, memory usage, or power usage label Sep 6, 2016
@ekelleyv
Copy link

ekelleyv commented Sep 6, 2016

Could this be implemented as a setting (rather than caring about tile domain)?

Add something like skip_style_validation to map instantiation that defaults to False, but can be set to True if the user is certain their styles are valid.

@lucaswoj
Copy link
Contributor

lucaswoj commented Sep 6, 2016

Perhaps we could sniff for a value like style.metadata.skipValidation

@averas
Copy link
Contributor

averas commented Sep 8, 2016

I like @lucaswoj's proposal. That way an application (such as my own) that switches client side styles frequently can skip style validation as well. Perhaps it even would be possible to have the style validation process cache its own outcome via a property on the style in the line of style.metadata.isValid which would be removed upon any mutation on the style. That way style validation wouldn't have to be carried out on non-mutated styles.

@averas
Copy link
Contributor

averas commented Sep 8, 2016

I realised my own proposal wouldn't work for any mutations you would carry out on the style object directly, but I still like the idea that clients can skip validations at their own will.

@jfirebaugh
Copy link
Contributor Author

How about making it an API option, rather than a metadata option? map.setStyle(..., {validate: false})

@averas
Copy link
Contributor

averas commented Sep 8, 2016

@jfirebaugh That would work, but as I understood your initial proposal you would like to have official Mapbox styles skip validation automatically, which perhaps is trickier if not part of the style definition but part of the API.

@lucaswoj
Copy link
Contributor

lucaswoj commented Sep 8, 2016

Another approach would be to have "development mode" (validate everything) and "production mode" (skip all validations) settings at the global level.

@jfirebaugh
Copy link
Contributor Author

I think we want a granular option like {validate: false}.

  • The default for setStyle (if no validate option is present) can depend on the style URL (disabled for mapbox:// URLs).
  • ...but setStyle can pass {validate: false} to the methods to which it delegates, fixing Eliminate duplicate style validation during style loading #3151.
  • ...but subsequent user-driven calls to those same methods can validate by default, no matter what the style URL is.
  • Users can pass {validate: false} for performance-critical calls they are sure are valid, and keep it in place in general for safety.
  • Workers can always pass {validate: false}, fixing Don't redo style validation in workers #3149.

@Bernoulli-IT
Copy link

Could this be implemented as a setting (rather than caring about tile domain)?

Add something like skip_style_validation to map instantiation that defaults to False, but can be set to True if the user is certain their styles are valid.

The other way around I assume? ;) Set to false if the user is uncertain...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance ⚡ Speed, stability, CPU usage, memory usage, or power usage
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants