Fix: labelDimCache

[maxkfranz]

Cross-references to related issues. If there is no existing issue that describes your bug or feature request, then create an issue before making your pull request.

Associated issues:

• Node label flickering when animating font-size #3270

Notes re. the content of the pull request. Give context to reviewers or serve as a general record of the changes made. Add a screenshot or video to demonstrate your new feature, if possible.

• Work in progress
• Currently uses a single value

Checklist

Author:

• The proper base branch has been selected. New features go on unstable. Bug-fix patches can go on either unstable or master.
• Automated tests have been included in this pull request, if possible, for the new feature(s) or bug fix. Check this box if tests are not pragmatically possible (e.g. rendering features could include screenshots or videos instead of automated tests).
• The associated GitHub issues are included (above).
• Notes have been included (above).

Reviewers:

• All automated checks are passing (green check next to latest commit).
• At least one reviewer has signed off on the pull request.
• For bug fixes: Just after this pull request is merged, it should be applied to both the master branch and the unstable branch. Normally, this just requires cherry-picking the corresponding merge commit from master to unstable -- or vice versa.

[maxkfranz]

@likeamike, does this address #3270 for you?

[maxkfranz]

@mikekucera, do you think this would be worth merging in for the next patch release?

I'd like to have it decided which PRs we're going to include so I can coordinate with the upcoming PR for the build system. I don't want to deal with lots of merge conflicts, so applying or (reapplying) my changes on top of everything we want included would be best. Thanks

[maxkfranz]

Just a note: This issue might be more widespread than just the use case indicated in #3270. If the old approach can generate invalid cache values for the label dimensions, then it could mess up all the label texture calculations for canvas and WebGL. So I suspect it would be better to include this PR in the upcoming release, unless you have any reservations.

[mikekucera]

Hi Max,

The labelDimCache used to have one entry for every label (when there's no animation). That way label dimensions are only calculated once per label.
The proposed replaces the labelDimCache with an object with a single entry. Won't this result in label dimensions being recalculated when they used to be cached?
I assumed that a fix for this issue would change how the cache keys are calculated as to avoid collisions.

[maxkfranz]

Yes, that's a good point.

Here's the approach in the PR:

(1)

• Uses one cached value per element (32 bit int cache key).
• Collides erroneously only if the hash of the label for the element accidentally matches the previous label's hash for the same element.

The old approach:

(2)

• Uses a global cache.
• Grows unbounded over time.
• More likely to erroneously collide than (1). Gets worse as the cache size grows.
• Uses one 32 bit int for the cache key per label.

Another approach:

(3)

• Uses a global cache like (2) but use two hash algorithms with two hash values. The hash tuple (h1, h2) is used as the key.
• Uses two 32 bit ints for the cache key.
• Far less likely to erroneously collide than (2).
• Still has the problem of unbounded growth. Would need something like an LRU cache.
• More complicated than (1).

An expensive approach:

(4)

• Use a global cache.
• Use a string as the cache key, including the label itself.
• Won't collide but uses much more memory.
• Might still be cheaper than recalculating the bounds each frame, at least in terms of CPU.

What do you think?

[mikekucera]

I think using one cached value per element in (1) would probably be the simplest and most reliable, even if it’s not the most efficient. (Do you even need a key if the bounds are stored on the element itself?) It has the disadvantage that if different nodes have the same label style then the bounds are recalculated for each instance, but I think that’s probably ok. The need is to avoid recalculating bounds on successive frames, not necessarily avoiding recalculating the same labels on the same frame. Lots of networks have unique labels for each node anyway.

I do think this solution is more addressing the symptom than the root cause. Hash keys shouldn’t be colliding so easily. The hash function should be producing keys with a uniform distribution. But changing the hashing is high risk and I shied away from it myself.

P.S. Edges actually need up to 3 labels right?

[maxkfranz]

I think using one cached value per element in (1) would probably be the simplest and most reliable, even if it’s not the most efficient. (Do you even need a key if the bounds are stored on the element itself?) It has the disadvantage that if different nodes have the same label style then the bounds are recalculated for each instance, but I think that’s probably ok. The need is to avoid recalculating bounds on successive frames, not necessarily avoiding recalculating the same labels on the same frame. Lots of networks have unique labels for each node anyway.

Looking at this a bit more, I think the simplest approach along the lines of (1) would be to remove the caching/hashing in this function altogether.

It looks like it may be a carryover from when we didn't have higher-level caching / rendered style invalidation. Currently, this function is only called in two ways:

(a) For calculating the label dimensions for rendering. These values are (already) stored/cached per-element and used in higher-level functions. (In the case of edges, 3 -- source, target, and mid labels)

(b) For calculating multi-line labels (wrapping, automatic ellipsis).

For the normal flow (a), removing the caching (i.e. labelDimCache) would only incur a performance penalty when the style of an element changes but the label happened to remain exactly the same in text and styling.

For multi-line calculations (b), you can't use a per-element cached value anyway. You have to check based on the words. I'm not sure caching the intermediate multi-line values would be worth the space/time trade-off either.

So what do you think about just removing the caching from this function (BRp.calculateLabelDimensions)?

I do think this solution is more addressing the symptom than the root cause. Hash keys shouldn’t be colliding so easily. The hash function should be producing keys with a uniform distribution. But changing the hashing is high risk and I shied away from it myself.

Yes. The hash function also has to be cheap and work with the limitations of JS (e.g. bitwise ops treat a number as a 32-bit int).

P.S. Edges actually need up to 3 labels right?

Yes

[maxkfranz]

Or the dimensions of the labels should be cached only in the higher-level applyPrefixedLabelDimensions function.

[maxkfranz]

Ref:

cytoscape.js/src/extensions/renderer/base/coord-ele-math/labels.js

Lines 298 to 320 in 5623dea

	BRp.applyPrefixedLabelDimensions = function( ele, prefix ){
	let _p = ele._private;
	let text = this.getLabelText( ele, prefix );
	let labelDims = this.calculateLabelDimensions( ele, text );
	let lineHeight = ele.pstyle('line-height').pfValue;
	let textWrap = ele.pstyle('text-wrap').strValue;
	let lines = util.getPrefixedProperty( _p.rscratch, 'labelWrapCachedLines', prefix ) || [];
	let numLines = textWrap !== 'wrap' ? 1 : Math.max(lines.length, 1);
	let normPerLineHeight = labelDims.height / numLines;
	let labelLineHeight = normPerLineHeight * lineHeight;
	let width = labelDims.width;
	let height = labelDims.height + (numLines - 1) * (lineHeight - 1) * normPerLineHeight;
	util.setPrefixedProperty( _p.rstyle, 'labelWidth', prefix, width );
	util.setPrefixedProperty( _p.rscratch, 'labelWidth', prefix, width );
	util.setPrefixedProperty( _p.rstyle, 'labelHeight', prefix, height );
	util.setPrefixedProperty( _p.rscratch, 'labelHeight', prefix, height );
	util.setPrefixedProperty( _p.rscratch, 'labelLineHeight', prefix, labelLineHeight );
	};