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

Constant time struct field access #676

Merged
merged 3 commits into from
Nov 7, 2023
Merged

Conversation

zslayton
Copy link
Contributor

@zslayton zslayton commented Nov 6, 2023

Issue #, if available:
#672

Description of changes:

This PR modifies the TemplateCompiler to maintain a field_name => field_value_addresses index for each template struct that it encounters. This allows LazyStruct::find(name: &str) to perform a single HashMap lookup any time its backing data is a template. If its backing data is a value literal, it will still need to perform a linear scan.

Commit guide:

  • f986c14 contains the meat of the change
  • 576fb0b removes a now-superflous context parameter from a few MacroEvaluator methods; MacroEvaluator stores its own context that can be used instead as needed.
  • 91f2d25 Incorporates clippy suggestions

Here are some timings from of the included benchmark from my 2019 Intel MBP:

Data Version Scan all fields Read single field Read all fields
Ion 1.0 118ms 123ms 138ms
Ion 1.1 40ms 42ms 67ms

The same benchmarks, this time running on an m5.4xlarge:

Data Version Scan all fields Read single field Read all fields
Ion 1.0 133ms 139ms 157ms
Ion 1.1 46ms 48ms 75ms

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

@zslayton zslayton requested a review from popematt November 6, 2023 21:36
Copy link
Contributor

@popematt popematt left a comment

Choose a reason for hiding this comment

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

Looks great! I am curious though, do you have any before and after benchmarks for this change (as opposed to Ion 1.0 vs Ion 1.1)? I'm curious to know how much of the difference between 1.0 and 1.1 is attributable to this change.

@zslayton
Copy link
Contributor Author

zslayton commented Nov 7, 2023

Looks great! I am curious though, do you have any before and after benchmarks for this change (as opposed to Ion 1.0 vs Ion 1.1)? I'm curious to know how much of the difference between 1.0 and 1.1 is attributable to this change.

On my laptop:

Change Scan all fields Read single field Read all fields
Before PR 40ms 50ms 67ms
After PR 40ms 42ms 67ms

@zslayton zslayton merged commit ccae133 into main Nov 7, 2023
@zslayton zslayton deleted the constant-time-struct-fields branch November 7, 2023 14:29
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.

2 participants