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

Remove extraneous clones in TypeNamed and ViolationChild #296

Merged
merged 1 commit into from
Nov 13, 2023

Conversation

popematt
Copy link
Contributor

Issue #, if available:

None.

Description of changes:

I changed the implementation of ViolationChild so that it's not unnecessarily creating clones of IonValues.
Also changed the implementation of TypeNamed so that it doesn't create a new struct and clone the entire type definition every time it validates a value.

My informal performance testing shows a significant improvement. I created a representative schema:

type::{ name: A, type: int }
type::{ name: B,  type: string }
type::{ name: C, type: $any }
type::{
  name: abc,
  fields: {
    a: A,
    b: B,
    c: C,
  }
}

And tested it using a set of values that were all valid for the type abc and another set that were all invalid. These results were measured by running warmups until the time was stable, using an "autorange" approach to determine the number of invocations in each sample, and then collecting and averaging the ops/s for 30 samples.

Dataset Before (ops/s) After (ops/s) Change
valid 1375113 2097905 52%
invalid 865061 1065709 23%

Related PRs in ion-schema, ion-schema-tests, ion-schema-schemas:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copy link

codecov bot commented Nov 13, 2023

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (e80edaa) 83.15% compared to head (f942da5) 83.13%.

Files Patch % Lines
...src/main/kotlin/com/amazon/ionschema/Violations.kt 44.44% 4 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #296      +/-   ##
==========================================
- Coverage   83.15%   83.13%   -0.02%     
==========================================
  Files         159      159              
  Lines        3770     3766       -4     
  Branches      908      905       -3     
==========================================
- Hits         3135     3131       -4     
- Misses        357      360       +3     
+ Partials      278      275       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@desaikd desaikd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@popematt popematt merged commit e80ee4b into amazon-ion:master Nov 13, 2023
3 of 5 checks passed
@popematt popematt deleted the typenamed branch November 13, 2023 23:16
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.

4 participants