Improve zoom/pan performance by avoiding redundant tag class and label width recomputation

[1-navneet]

This PR addresses the zoom and pan performance issue described in #11832.

Problem

Profiling during zoom/pan interactions showed significant time spent repeatedly:

• Computing SVG tag class strings for entities whose tags were unchanged
• Measuring SVG text widths during label redraws

These operations were executed on every redraw, even when the underlying data did not change.

Solution

This change reduces redundant work during redraws by:

• Reusing cached tag class strings when entity tags are unchanged
• Reusing a persistent SVG text measurer per font size to avoid repeated DOM create/remove cycles

Result

• Reduced time spent in drawEditable during zoom/pan interactions
• Hotspots such as tagClasses.getClassesString no longer dominate redraw time
• No visual or behavioral changes observed

Testing

• Tested locally using Chrome DevTools Performance profiling
• Reproduced using the same location and zoom level as the issue report
  (zoom level 18 near 53.382847, -1.470167)

[gralp-1]

This is certainly better but I'm still getting some 400ms+ zoom calls. The culprit seems to be indexing an entity's tags through v = t[k] on lines 81, 106 and 119. It maybe seems like the cache is mostly missing?

[1-navneet]

@gralp-1,

Thanks for taking a closer look — glad to hear this is an improvement.
That’s a good catch. The cache key is currently based on the computed tag value, so if many entities have distinct tag objects or values, the hit rate may indeed be low.

I’ll take a closer look at whether we can avoid repeatedly indexing into the entity tags during redraws, or restructure the cache to operate at the entity or tag-set level instead. I’ll report back once I have a clearer picture.

[1-navneet]

Thanks for the detailed profiling — that was very helpful.

I’ve pushed a follow-up commit that adds a WeakMap cache keyed by the entity’s tag object, so getClassesString() can return early without repeatedly indexing t[k] during redraws.
This reduces cache misses and further smooths zoom interactions, while keeping behavior unchanged and the changes confined to tag_classes.js.

Let me know if you’d like me to try a different cache key or gather additional traces.

[tordans]

Could you add some guidance on how to test this properly? And also add some test results here. Is this a case that will be visible in flame charts?

[1-navneet]

@tordans,
Thanks for the questions — happy to clarify.

How to test:
•Run npm start
•Open http://localhost:8080/#map=18/53.382847/-1.470167
•Open Chrome DevTools → Performance (Screenshots enabled)
•Record ~6–8 seconds while repeatedly zooming, then panning
•Stop the recording and inspect the Bottom-Up / Call Tree views

What to look for:
In the flame chart / bottom-up view, tagClasses.getClassesString previously appeared as a dominant hotspot during zoom redraws. With this change, its self/total time is significantly reduced and no longer dominates redraw time.

Observed results:
Zoom interactions are smoother and cache misses are reduced. getClassesString still appears in flame charts, but with much lower cost and fewer expensive calls. No visual or behavioral changes were observed.

Let me know if you’d like screenshots attached or traces collected for a specific scenario.

[k-yle]

this is a great start, nice work. some thoughts below:

[k-yle]

currently your cache (WeakMap<Tags, Map<string, string>>) represents this hierarchy: tags → oldClassName → newClassName.

This leads to a few issues:

• the cache hit rate will be very low, because whenever the user hovers or selects a feature, the class name is updated (.selected or .hover is added/removed)
• your cache will grow in size quickly, and store many no-op transformations:

---

To solve these issues, I think you could only cache a weak-mapping of tags → className (i.e. WeakMap<Tags, string>)

This will require some refactoring, so that we have a pure function which is simply f(tags) → className.

for example, something like this:

let cache = new WeakMap();

tagClasses.getClassesString = function(t, oldClassName) {
	const oldClassNamestoKeep = oldClassName.split(' ').filter(…).join(' ');
   
    let newClassNames = cache.get(t);
    if (newClassNames === undefined) {
    	newClassNames = getClassesStringPure(t); // refactor everything else into this new function
        cache.set(t, newClassNames);
    }

    return oldClassNamestoKeep + ' ' + newClassNames;
}

[1-navneet]

@k-yle

Thanks for the detailed feedback — that makes sense.
You’re right about the cache hierarchy and scope concerns. I’ll refactor this to:

• move the cache to module scope
• reduce it to a pure tags → className mapping
• separate base-class handling from tag-derived classes
  I’ll push an updated revision shortly.

[k-yle]

if it helps, 556fbb2 is one way to implement the suggestions above

[1-navneet]

Thanks for the pointer to 556fbb2 — that helped.

I’ve updated the implementation to:

• move the cache to module scope
• cache only tag-derived classes (tags → class list)
• keep base class handling separate from the cached values

Let me know if this matches what you were suggesting, or if you'd prefer it structured differently.

[k-yle]

thanks, this works really well now !

@gralp-1: I see you did your own profiling, do you want to take a look at the latest version of this PR?

@1-navneet: last suggestion from me, can you please update the CHANGELOG.md file?

[1-navneet]

Updated the CHANGELOG as suggested and rebased onto latest develop.
Thanks again for the review!

[gralp-1]

I still seem to be getting some redraw calls up to 2.5s
openstreetmap iD trace.gz
(to see the traces in chrome, press f12 on any website, go to the performance tab and press load trace on the top bar)

[gralp-1]

although these redraws seem to be spending ~50% of their time doing layout and style calculations

[k-yle]

I've been testing this with CPU Throttling set to 'Mid Tier Mobile', and there's noticably less¹ time spent in iD's own code (getClassesString), with the bottleneck now moving to reflows, as you noticed, which is tracked by #7710

for future reference, this is the trace from my laptop

Footnotes

1. it's hard to put an exact number on it, but it seems to be at least 30% faster (based on my very unscientific testing) ↩

[1-navneet]

Thanks for testing with CPU throttling — that’s helpful.
I’m also seeing the bottleneck shift toward layout/reflow costs now.
Since that aligns with #7710, I’m considering this PR an incremental improvement focused on reducing getClassesString overhead.
Happy to follow up separately on the reflow work if that makes sense.

[1-navneet]

Hi @k-yle — just a quick follow-up on this when you have time.
The branch is rebased onto latest develop, conflicts resolved, and checks are passing.
Happy to make further adjustments if needed.