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

AttributesGroups deep equality test (speed up selection) #262

Merged
merged 1 commit into from Sep 20, 2023

Conversation

alert-dan
Copy link
Contributor

The default AttributesGroups equality test is == on a List, which returns false even if the contents are equal. The impact of this is that Operator.update() will always think the AttributesGroups have changed after a selection, regardless of whether or not there was a real change.

Implementing equal value for AttributesGroups using deepCollectionEquals allows update() to return false when the graph content hasn't changed. This short-circuits re-rendering as designed.

For a complex graph that uses tooltips but does not change other Attributes on selection, this improves the effective framerate by about 8x.

Override the equalValue member to do a deep equality test
The default equality test doesn't actually look at values.
This causes a render after every selection, even if nothing has changed.
For graphs with a lot of points this caused a severe slowdown.
@entronad
Copy link
Owner

Thanks for your deep looking. I agree that AttributesGroups should use deepCollectionEquals in comparison.

A basic idea in Graphic is all collection should use deep == (Except data list, it may be too long), I haven't noticed this one.

Only one suggestion that you'd better override AttributesGroups's == operator here:

extension AttributesGroupsExt on AttributesGroups {

not in the selection op, for it is also used in other place, where a deep == is also needed.

@alert-dan
Copy link
Contributor Author

Thanks for the feedback. Trying to refactor as you suggested, but it appears that we can not use an extension to override a method that already exists on the class. Since Operator== exists on Object, it can never be a valid extension name.

Specifically, the dart tooling gives me the error

Extensions can’t declare members with the same name as a member declared by ‘Object’.

https://dart.dev/tools/diagnostic-messages?utm_source=dartdev&utm_medium=redir&utm_id=diagcode&utm_content=extension_declares_member_of_object#extension_declares_member_of_object

The example given there is that you can not override toString on String using an extension.

Digging into the Specification
https://github.com/dart-lang/language/blob/main/accepted/2.7/static-extension-methods/feature-specification.md#dart-static-extension-methods-design (Referenced from the official docs at https://dart.dev/language/extension-methods)

I find that

An extension can declare a member with the same (base-)name as a member of the type it is declared on. This does not cause a compile-time conflict, even if the member does not have a compatible signature.
You cannot access this member in a normal invocation, so it could be argued that you shouldn't be allowed to add it.

Which I interpret to mean that it's not technically a compile-time error, it would not be called, since operator== already exists.

@entronad
Copy link
Owner

Emm, that's a new knowledge, I shall accept your commit and check if anywhere else need this.

@entronad entronad merged commit a8321d1 into entronad:main Sep 20, 2023
@entronad
Copy link
Owner

This is published in v2.2.1

I have made a step forward that all operator value equality should use deep eqaulity, (It will only affect collections), that fits the original idea about operator updating checking.

Please have a try.

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

Successfully merging this pull request may close these issues.

None yet

2 participants