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

Add Dataset & Element equality methods, greatly speeding up tests that rely on dataset comparisons. #280

Merged
merged 4 commits into from
Aug 26, 2023

Conversation

suyashkumar
Copy link
Owner

@suyashkumar suyashkumar commented Aug 26, 2023

This change introduces well-defined equality methods for Dataset, Element, and some other data structures. This greatly speeds up tests that rely on checking equality of datasets (that previously needed reflection). For example, it reduces the total test suite from 1m24s to 10s on GitHub actions. Right now this speed up is from one sanity test in particular that compares data structures on the order of MB (50s -> 500ms), as most other tests still use go-cmp which is appropiate for small data structure comparisons. This also gives users the ability to quickly check equality of Datasets (e.g. in their own business logic or tests).

However, this does mean that if new fields are added to any of these structs it is important for the Equals method to be updated as well. For now this will be enforced during code review (helped by the fact most of these structs should not and do not change often), but we should investigate lint rules or some auto-generated reflection based tests (which would do orders of magnitudes less reflection than the current tests) that can help catch when this doesn't happen (see #281). This is also fairly verbose, however maintaining and adding to these isn't that much more incremental overhead since we already need to maintain MarshalJSON and String methods on Elements anyway (note: json serialization for equality was considered as well, but is still fairly slow as it partially relies on reflection).

Finally, there may also be a reflection based way to code-gen an equals method for each data structure of interest, which could be of interest.

This change also makes a change to rely on pointers for []*frame.Frame in the PixelDataInfo.

Screenshot 2023-08-26 at 7 04 33 PM Screenshot 2023-08-26 at 7 05 02 PM

@suyashkumar suyashkumar changed the title Add Dataset and element equality methods, greatly speeding up tests that rely on dataset comparisons. Add Dataset & Element equality methods, greatly speeding up tests that rely on dataset comparisons. Aug 26, 2023
@suyashkumar suyashkumar merged commit 8f1f151 into main Aug 26, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant