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

Don't create duplicate or unnecessary Proj4 objects #54

Closed
hampelm opened this issue May 24, 2013 · 6 comments
Closed

Don't create duplicate or unnecessary Proj4 objects #54

hampelm opened this issue May 24, 2013 · 6 comments
Assignees

Comments

@hampelm
Copy link

hampelm commented May 24, 2013

(Was: Projector performance: handle creating Proj4js.Proj objects better)

Mostly a note so I remember to deal with this.

If you enter the projection cascade (FeatureCollection=>Feature=>...Point) after FeatureCollection, you risk creating a new Proj4js.Proj object twice for every point. Turns out that adds a LOT of overhead.

@Mr0grog
Copy link
Member

Mr0grog commented May 24, 2013

Hmmm, there were originally supposed to be separate public entry points where we checked for/created Proj4 instances vs. private implementations where we could assume a Proj4 instance for exactly this purpose. Don't remember what happened with that.

@prashtx
Copy link

prashtx commented May 24, 2013

If we document that the projector API can accept Proj4js.Proj objects, then the caller can pass in those objects instead of the projection strings. But a naive consumer might not realize the performance hit, so maybe caching the Proj4js.Proj objects makes sense.

@Mr0grog
Copy link
Member

Mr0grog commented May 24, 2013

The fact that projector uses Proj4 is and should be an implementation detail and I don't believe it's meaningfully beneficial to any public API. This is especially meaningful because Proj4 is not always faster. For example, projector explicitly bypasses it and uses its own routines in the common and mathematically simple case of 4326 <-> 900913 because Proj4 is dramatically slower in that case.

@Mr0grog
Copy link
Member

Mr0grog commented Jun 9, 2013

Renamed this for some clarity. Things that need to happen here:

  1. Cache Proj4 objects so we don't create a new one for every point or for every projection call.
  2. Don't create Proj4 objects we won't use (we currently always have a Proj4 object, but we don't use it when converting from 4326 <--> 900913).
  3. (Potentially) allow Proj4 objects to be passed into the public API and be clear that this is allowed. It should currently work, but we aren't very clear that that's OK. Not convinced that this is super-important, but it could give a slight performance edge over looking them up from a cache (1)

@Mr0grog
Copy link
Member

Mr0grog commented Jun 9, 2013

Added Proj4 caching in 708a4df. Big perf. boost:

FeatureCollection     x 4.34 ops/sec ±3.95% (15 runs sampled)
Feature               x 3.69 ops/sec ±3.36% (14 runs sampled)
Old FeatureCollection x 4.21 ops/sec ±4.38% (15 runs sampled)
Old Feature           x 0.72 ops/sec ±3.91% (6 runs sampled)

@Mr0grog
Copy link
Member

Mr0grog commented Jun 9, 2013

Even better in fc38aae!

FeatureCollection     x 4.38 ops/sec ±3.99% (15 runs sampled)
Feature               x 5.38 ops/sec ±3.32% (18 runs sampled)
Old FeatureCollection x 4.27 ops/sec ±4.31% (15 runs sampled)
Old Feature           x 0.75 ops/sec ±2.86% (6 runs sampled)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants