Fix lookupCoin and valueContains memory models to match the committed JSON - #7941
Open
Unisay wants to merge 1 commit into
Open
Fix lookupCoin and valueContains memory models to match the committed JSON#7941Unisay wants to merge 1 commit into
lookupCoin and valueContains memory models to match the committed JSON#7941Unisay wants to merge 1 commit into
Conversation
…tted JSON `BuiltinMemoryModels.hs` declared `ModelThreeArgumentsConstantCost 10` for `lookupCoin` and `ModelTwoArgumentsConstantCost 32` for `valueContains`, while every `builtinCostModel*.json` carries 1. `generate-cost-model` copies memory models verbatim into the JSON, so the next full regeneration would have multiplied the on-chain memory cost of both builtins by 10x and 32x. 1 is the correct value: `valueContains` returns a `Bool` and now uses the existing `boolMemModel`, and `lookupCoin` returns a quantity already stored in the `Value`, so it allocates nothing. Regenerating with this change reproduces the committed memory models exactly. Found while costing `assetCount`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
BuiltinMemoryModels.hsdeclaredModelThreeArgumentsConstantCost 10forlookupCoinandModelTwoArgumentsConstantCost 32forvalueContains, while everybuiltinCostModel*.jsoncarries 1.generate-cost-modelcopies memory models verbatim into the JSON, so the next full regeneration would have multiplied the memory cost of both builtins by 10x and 32x with nobody intending it.1 is also the right value.
valueContainsreturns aBool, so it now uses the existingboolMemModel, andlookupCoinreturns a quantity already held in theValue, so it allocates nothing. The 10 and 32 were copied from thelengthOfArrayandindexArrayentries directly above, which construct a freshIntegerand are not comparable.Verified by building
generate-cost-modelwith+with-inline-rand running it against the committedbenching-conway.csvandmodels.R. With this change its memory output matches the committed memory model of all 104 builtins in variants C and E, where before it differed on these two. The committed JSON does not change, hence theNo Changelog Requiredlabel.Found while costing
assetCount.