Enhance getVisibleCoordinates performance

[pakastin]

I started to profile panning/zooming performance and realized getVisibleCoordinates function is pretty heavy. Preallocating an array and using a single for-loop while adding posMatrix simultaneously makes it faster.

[pakastin]

Here's relevant jsperf: https://jsperf.com/for-loop-vs-array-prototype-map-bench

[mourner]

I doubt the JSPerf is representative here, because there are significant differences — 1) the number of coordinates is usually very low (e.g. 6 tiles is typical for a full-screen map), vs 10000 in the bench; 2) arrays of objects like in this case have very different perf characteristics than arrays of numbers, which are treated differently by JS engines. 3) Microbenchmarks can be misleading — e.g. 1, 2.

Unless you can demonstrate measurable performance difference in GL JS specifically, I'd be inclined to keep the code more terse and simple.

[pakastin]

Wonder why that function took longest scripting time inside Chrome dev tools performance tab, and after this I got it down to like 10th. I try to run some benchmarks with and without.

[pakastin]

Didn't realize the arrays are so small, so you're probably right! Maybe need to profile getRenderableIds instead.. 🤔

[mourner]

Wonder why that function took longest scripting time inside Chrome dev tools performance tab, and after this I got it down to like 10th. I try to run some benchmarks with and without.

The profiling results often fluctuate depending on what's happening in the background, what exactly you did during the profiling, etc. Here's a quick result I got zooming/panning for a few seconds:

[pakastin]

Yeah, that might be the case – i'll keep on investigating, thanks! 😎

[andycalder]

@pakastin it's interesting that you raised this because I've also noticed getVisibleCoordinates being really heavy in certain situations. I tested the satellite debug map with 60 degree pitch to maximise the number of tiles (raster maps need 4x as many tiles). Looking in the flamegraph, you can see getVisibleCoordinates taking up a significant portion of frame time. The below frame is pretty typical for me:

I don't have a solution but I agree that it might be worth investigating.

[pakastin]

Yeah, I too have quite many layers here:
https://beta.aviamaps.com/map

I was wondering did I really profile so wrong 😄

[pakastin]

Well, this PR would make it faster when there’s many iterables.. 😎

[asheemmamoowala]

@pakastin @mourner on a pitched satellite map, I am seeing a consistent reduction in getVisibleCoordinates performance from 3-7ms, to 0.3-1.2ms.

[pakastin]

Yeah, in some other circumstances that would be just micro-optimization, but when you have constant 60 fps budget, every fraction of a millisecond counts..

[mourner]

I've also made a similar change as a part of #7971 — can you check this one too?

[andycalder]

I've been testing this by panning a pitched satellite map around for a few seconds and I'm unable to see any significant performance improvement relative to master. Dev tools is telling me that almost all of the time is spent on getRenderableIds() and calculatePosMatrix() so I'm not sure how refactoring the loop is going to help.

@asheemmamoowala do you mean to say that the pitched map reduces performance relative to a flat map? Or that this PR increases performance relative to master?

[andycalder]

@mourner I did a quick test of #7971 and it seems to be the same. No significant change in performance.

[asheemmamoowala]

@asheemmamoowala do you mean to say that the pitched map reduces performance relative to a flat map? Or that this PR increases performance relative to master?

The fix here improves performance relative to master, i.e. it reduces the time spent in getVisibleCoordinates.

[pakastin]

It’s almost impossible that it would be slower after optimizations, since for-loop does less than array prototype methods.