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

Costing of BuiltinArray functions #6950

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

Unisay
Copy link
Contributor

@Unisay Unisay commented Mar 13, 2025

@Unisay Unisay force-pushed the yura/costing-builtin-array branch from 3865759 to 66501da Compare March 14, 2025 10:06
@Unisay Unisay force-pushed the yura/costing-builtin-array branch from 66501da to c891dcc Compare March 17, 2025 14:06
@Unisay Unisay force-pushed the yura/costing-builtin-array branch from c891dcc to 7b9c8a9 Compare March 17, 2025 17:10
@Unisay Unisay self-assigned this Mar 18, 2025
benchWith params name term = bench name $ whnf (evaluateCekNoEmit params) term
benchWith params name term =
bench name $
whnf (unsafeSplitStructuralOperational . evaluateCekNoEmit params) term
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes a benchmark fail if evaluation fails (wasn't the case before)

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe you could add a comment to remind us why unsafeSplitStructuralOperational is there, since it's not immediately obvious what it's doing.

@Unisay Unisay requested a review from kwxm March 19, 2025 10:19
@Unisay Unisay marked this pull request as ready for review March 19, 2025 10:19
Comment on lines +176 to +178
, paramLengthOfArray = Id $ ModelOneArgumentConstantCost 10
, paramListToArray = Id $ ModelOneArgumentLinearInX identityFunction
, paramIndexArray = Id $ ModelTwoArgumentsConstantCost 32
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kwxm 10 and 32 here are magic numbers I chose because they were chose in similar builtin functions. This is not based on any measurements I did whatsoever.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what we should put for the memory costs. I guess for ListToArray we'll need something with a non-zero constant term to account for some header data and then one pointer's worth of memory for each entry. @effectfully Do you remember how much room a pointer takes up?

For LengthOfArray we'll need enough memory to hold one integer, so maybe a constant cost of 1 would do, and for IndexArray it'll again be one pointer's worth.

But then again, the current numbers are pretty similar to those for other things like LengthOfByteString, so maybe these are OK. We charge 100 memory units for each CEK step anyway, so that dominates the memory cost and and extra 10 or 20 bytes for a builtin won't make a lot of difference.

Copy link
Contributor

@kwxm kwxm left a comment

Choose a reason for hiding this comment

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

This looks mostly OK, although I think that the costing benchmarks need to have ArrayCostedByLength wrappers. Apart from that it looks fine.

Comment on lines +176 to +178
, paramLengthOfArray = Id $ ModelOneArgumentConstantCost 10
, paramListToArray = Id $ ModelOneArgumentLinearInX identityFunction
, paramIndexArray = Id $ ModelTwoArgumentsConstantCost 32
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what we should put for the memory costs. I guess for ListToArray we'll need something with a non-zero constant term to account for some header data and then one pointer's worth of memory for each entry. @effectfully Do you remember how much room a pointer takes up?

For LengthOfArray we'll need enough memory to hold one integer, so maybe a constant cost of 1 would do, and for IndexArray it'll again be one pointer's worth.

But then again, the current numbers are pretty similar to those for other things like LengthOfByteString, so maybe these are OK. We charge 100 memory units for each CEK step anyway, so that dominates the memory cost and and extra 10 or 20 bytes for a builtin won't make a lot of difference.

benchWith params name term = bench name $ whnf (evaluateCekNoEmit params) term
benchWith params name term =
bench name $
whnf (unsafeSplitStructuralOperational . evaluateCekNoEmit params) term
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe you could add a comment to remind us why unsafeSplitStructuralOperational is there, since it's not immediately obvious what it's doing.


benchLengthOfArray :: StdGen -> Benchmark
benchLengthOfArray gen =
createOneTermBuiltinBench LengthOfArray [tyArrayOfInteger] listOfArrays
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to use the create<N>TermBudgetingBenchWithWrappers here to wrap the array arguments, like this, although that will only really matter for listToArray since the others are constant time. We have to use the same size measures here as are used in the denotations.

Try adding the wrappers and re-running the benchmarks and see if the cost of listToArray changes significantly. If it does, you'll have to rebuild the cost model with the new results.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm thinking that we should probably make the costed-by-length measure the default one and maybe do the same for lists (although maybe then we'll need a wrapper that measures the entire memory footprint for some of the other things involving lists). I'll make an issue to look at that separately.

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