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

Host budget metering, cost model, calibration changes #704

Merged
merged 30 commits into from
Mar 21, 2023

Conversation

jayz22
Copy link
Contributor

@jayz22 jayz22 commented Feb 24, 2023

What

This is a set of sweeping changes/fixes/cleanups to host calibration and metering:

  1. Clean up of cost types:
  • Add HostMemAlloc/Cpy/Cmp for host memory allocation, copy, and comparison
  • Remove redundant cost types or consolidate them with more general types such as HostMem types, e.g. ScVecToHostVec, CloneEvents, HostObjAllocSlot, and the single byte cost types i.e. BytesPush/Pop/Insert/Del.
  1. Clean up unused model terms: log, quadratic

  2. Add a "baseline" calibration and subtract it from the actual measurements for more accurate results

  3. Remove sample deallocation (using std::mem::forget) from the measurement results for better accuracy.

  4. Various fixes:

  • ChargeBudget was previously not increasing the input
  • MeteredVector, MeteredMap when to charge_new.
  • metered_scan_slice_of_slices
  • test_declared_size was being too strict (using assert_eq instead of le), resulting in failure in CI but passes locally.
  1. Changes for improving debug experience
  • Add per cost per CostType to the Budget, add Debug impl for it.
  1. Update the cpu and mem cost parameters based on the newly calibrated results.

Below are the calibration output.
output6.txt

Why

[TODO: Why this change is being made. Include any context required to understand the why.]

Known limitations

[TODO or N/A]

@jayz22 jayz22 changed the title Host calibration, metering cleanups Host budget metering, cost model, calibration changes Mar 16, 2023
@jayz22 jayz22 marked this pull request as ready for review March 16, 2023 05:44
@jayz22 jayz22 requested review from graydon, sisuresh and a team as code owners March 16, 2023 05:44
Copy link
Contributor

@dmkozh dmkozh left a comment

Choose a reason for hiding this comment

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

I haven't fully reviewed this yet, 11 files are still remaining.

soroban-env-host/benches/common/cost_types/host_mem_cmp.rs Outdated Show resolved Hide resolved
soroban-env-host/benches/common/cost_types/host_mem_cpy.rs Outdated Show resolved Hide resolved
soroban-env-host/benches/common/cost_types/mod.rs Outdated Show resolved Hide resolved
soroban-env-host/benches/common/cost_types/vm_ops.rs Outdated Show resolved Hide resolved
soroban-env-host/src/budget.rs Outdated Show resolved Hide resolved
soroban-env-host/src/budget.rs Outdated Show resolved Hide resolved
soroban-env-host/src/budget.rs Outdated Show resolved Hide resolved
soroban-env-host/src/budget.rs Show resolved Hide resolved
soroban-env-host/src/cost_runner/cost_types/im_vec_ops.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@graydon graydon left a comment

Choose a reason for hiding this comment

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

Generally good! Very glad to see:

  • Number of cost types shrinking, eliminating duplication and ambiguity
  • Smaller, simpler and more precise calibration functions
  • Simpler models
  • Actual calibration results that are closer to reality

Handful of nits noted in review, and some questions and concerns, broadly speaking:

  • Might want to avoid the forget calls in favour of a pass/return protocol
  • Might want to black_box some stuff
  • Some of the byte-growth metering seems like it's overcharging
  • Calibration numbers seem like a few have noise in them or don't make sense
  • A few calibration functions don't seem like they're counting exactly what they should

@jayz22
Copy link
Contributor Author

jayz22 commented Mar 18, 2023

@graydon I have just pushed three commits that addresses your major concerns:

  • Use recycling patterns instead of std::mem::forget
  • Putting runner inside std::hint::black_box to prevent unexpected optimization.
  • Resolve bytes over-charging.
    I think this should solve all of the structural issues. I will go on to address the remaining "nit"/one-off comments.

@graydon
Copy link
Contributor

graydon commented Mar 20, 2023

@jayz22 ok I've marked most of the issues I had as resolved, and added a few very minor nits around saturating adds. I think mostly I'm good with this now, though there's still a bunch of commentary from @dmkozh to resolve as well. I'm happy to merge it whenever ready (and I concur it'd be nice to get into this release, which is starting tomorrow, so if you and @dmkozh could give it some focused time this afternoon maybe we can make it?)

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

3 participants