-
-
Notifications
You must be signed in to change notification settings - Fork 6k
#1968 compiled interpolation #1969
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
Conversation
|
Is this really a bottleneck to justify adding this type of complexity to the code? |
|
threw together a perf it does seem to offer a pretty good speed up, might make sense to make it a bit more readable or remove the surrounding closure. |
|
I'm sure it offers good relative speed up, but is the original performance a bottleneck in the first place? Lets take for example full screen Macbook 15 Retina with 2880 x 1800 resolution and divide that by 256x256 — we're getting about 80 tiles per one screen. Now we look at the benchmark above — Leaflet template gets 23,631 operations (20 template calls) per second, that means that templating one full screen load of 80 tiles is performed in 80 * 1000 ms / (23,631 * 20) = 0.16 milliseconds. 0.16 ms per full Retina screen. Or 100 full screens (8000 tiles) per one frame of 16 ms. Is this slow enough to even bother optimizing? Now we see why people say "Premature optimization is the root of all evil". |
|
so my phone can do 300 to 600 operations per second, we'll round down to 300 (we can assume i have otherfor things going on) and my phone has 45 tiles displayed at once (5 across and 7 down, counted ). That's 7.5 ms or 1 full frame if i have an overlay tileset too. |
|
i did the math wrong thats only 35 tiles so overlay and scrolling |
|
I think the main difference this would make would be in situation with slow processors not necessarily situation with high resolution |
|
OK, I'll think about this. Thanks Calvin! |
|
Some more thoughts:
|
|
cleaned up the regex also changed the expression that searched for the {variables} to be the same as the old one, switched that method name to be |
|
This looks much better. Lets also cover this with some tests, and I'll merge once I'm back from vacation. |
|
I just saw you were back from vacation, I can throw some tests together latter today. |
|
Yep! |
|
ok added tests for the template cache and the template function and made the current template test sneakier, also rebased into one commit |
|
Awesome! One last bit: I think that as |
| @@ -104,17 +104,24 @@ L.Util = { | |||
| } | |||
| return ((!existingUrl || existingUrl.indexOf('?') === -1) ? '?' : '&') + params.join('&'); | |||
| }, | |||
|
|
|||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't remove empty lines, they're good for readability :)
|
ok switched it to give the data to the compile function |
|
Thanks Calvin, merged and clean up a bit to be simpler and more consistent with other Leaflet code. Notice how the huge initial pull (before the discussion) got shrinked to just a few simple lines of code. :) |
pull for #1968 which adds compiled templates