Vectors in UPLC #5816
Merged
Vectors in UPLC #5816
Conversation
This file contains 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
|
/benchmark plutus-benchmark:nofib |
|
Click here to check the status of your benchmark. |
|
Comparing benchmark results of 'plutus-benchmark:nofib' on '1f9b73f94' (base) and 'f221fec18' (PR) Results table
|
|
/benchmark plutus-benchmark:marlowe |
|
Click here to check the status of your benchmark. |
|
Comparing benchmark results of 'plutus-benchmark:marlowe' on '1f9b73f94' (base) and 'f221fec18' (PR) Results table
|
|
/benchmark marlowe |
|
Click here to check the status of your benchmark. |
|
Comparing benchmark results of 'marlowe' on '1f9b73f94' (base) and 'f221fec18' (PR) Results table
|
|
/benchmark marlowe |
|
Click here to check the status of your benchmark. |
|
Comparing benchmark results of 'marlowe' on '1f9b73f94' (base) and '86bedde12' (PR) Results table
|
|
@michaelpj I don't think we need vectors in |
Yes, you're right, I was just being overly zealous there. |
|
We also don't need them in |
michaelpj
force-pushed
the
mpj/vector-cases
branch
from
86bedde
to
6b57736
Compare
|
/benchmark plutus-benchmark:nofib |
|
Click here to check the status of your benchmark. |
|
Comparing benchmark results of 'plutus-benchmark:nofib' on 'cb91fa6fb' (base) and '6b5773645' (PR) Results table
|
|
Still looks a bit better. |
michaelpj
force-pushed
the
mpj/vector-cases
branch
from
6b57736
to
4c4f04b
Compare
michaelpj
commented
Apr 3, 2024
plutus-core/untyped-plutus-core/src/UntypedPlutusCore/Core/Instance/Eq.hs
Outdated
Show resolved
Hide resolved
michaelpj
commented
Apr 3, 2024
| -- s , case _ (C0 ... CN, ρ) ◅ constr i V1 .. Vm ↦ s , [_ V1 ... Vm] ; ρ ▻ Ci | ||
| returnCek (FrameCases env cs ctx) e = case e of | ||
| (VConstr i args) -> case cs ^? wix i of | ||
| -- TODO: handle word/int conversion better |
There was a problem hiding this comment.
Also unsure what to do here. Check if the Integer is in range, otherwise assume it's a miss?
There was a problem hiding this comment.
It's probably fine, because you're only going to get a negative integer if an overflow occurs and that will be caught by (V.?!), but this really does need some extensive manual testing.
You could also do what I did in the SmallArray PR:
VConstr i args
| i < unsafeCoerce (sizeofSmallArray cs) ->
computeCek (transferArgStack args ctx) env . indexSmallArray cs $ unsafeCoerce i
where indexSmallArray is unsafe lookup and sizeofSmallArray stupidly returns the length of the array rather than its byte size (man it pisses me off that they literally gave up on the usual camelCasing and stole the name of a C primitive just to assign it a completely different meaning). The logic here is
- we know that the length of the array is a non-negative
Int, so it's safe tounsafeCoerceit to aWord64 - once we know that
iis within the array bounds, it's safe tounsafeCoerceit to anInt, unless the array is somehow larger thanfromIntegral (maxBound :: Int) :: Word64, which isn't something we could deserialize
Please note that I absolutely do not propose to have unsafeCoerce in there, just explaining the reasoning. Replacing both with fromIntegral should do the job just fine.
There was a problem hiding this comment.
It's probably fine, because you're only going to get a negative integer if an overflow occurs and that will be caught by (V.?!), but this really does need some extensive manual testing.
Yeah no, you can still wrap around to positive again.
I'll add some kind of bounds check.
There was a problem hiding this comment.
Okay, so it can in fact never trigger, because i is a Word64 and the largest Word64 wraps to only -1 as an Int64, so can't produce an "apparently good" value. I've left the check in out of caution.
I wanted to write a conformance test for this, but I was blocked by the fact that the parser doesn't do range checks, so in fact already wraps a "too large" number around problematically 😂 but that's a parser bug.
effectfully
reviewed
Apr 3, 2024
plutus-core/untyped-plutus-core/src/UntypedPlutusCore/Core/Instance/Eq.hs
Outdated
Show resolved
Hide resolved
plutus-core/untyped-plutus-core/src/UntypedPlutusCore/Evaluation/Machine/Cek/Internal.hs
Outdated
Show resolved
Hide resolved
| -- s , case _ (C0 ... CN, ρ) ◅ constr i V1 .. Vm ↦ s , [_ V1 ... Vm] ; ρ ▻ Ci | ||
| returnCek (FrameCases env cs ctx) e = case e of | ||
| (VConstr i args) -> case cs ^? wix i of | ||
| -- TODO: handle word/int conversion better |
There was a problem hiding this comment.
It's probably fine, because you're only going to get a negative integer if an overflow occurs and that will be caught by (V.?!), but this really does need some extensive manual testing.
You could also do what I did in the SmallArray PR:
VConstr i args
| i < unsafeCoerce (sizeofSmallArray cs) ->
computeCek (transferArgStack args ctx) env . indexSmallArray cs $ unsafeCoerce i
where indexSmallArray is unsafe lookup and sizeofSmallArray stupidly returns the length of the array rather than its byte size (man it pisses me off that they literally gave up on the usual camelCasing and stole the name of a C primitive just to assign it a completely different meaning). The logic here is
- we know that the length of the array is a non-negative
Int, so it's safe tounsafeCoerceit to aWord64 - once we know that
iis within the array bounds, it's safe tounsafeCoerceit to anInt, unless the array is somehow larger thanfromIntegral (maxBound :: Int) :: Word64, which isn't something we could deserialize
Please note that I absolutely do not propose to have unsafeCoerce in there, just explaining the reasoning. Replacing both with fromIntegral should do the job just fine.
Hm, maybe I should try |
michaelpj
force-pushed
the
mpj/vector-cases
branch
from
98836ee
to
ce76e65
Compare
|
/benchmark plutus-benchmark:nofib |
|
Click here to check the status of your benchmark. |
michaelpj
force-pushed
the
mpj/vector-cases
branch
from
ce76e65
to
4b5d6c1
Compare
|
/benchmark plutus-benchmark:nofib |
|
Click here to check the status of your benchmark. |
|
Comparing benchmark results of 'plutus-benchmark:nofib' on '191103257' (base) and '4b5d6c15a' (PR) Results table
|
|
I'm unsure if that's a real result or if the changes to |
|
/benchmark plutus-benchmark:nofib |
1 similar comment
|
/benchmark plutus-benchmark:nofib |
|
Click here to check the status of your benchmark. |
|
Click here to check the status of your benchmark. |
michaelpj
force-pushed
the
mpj/vector-cases
branch
from
4b5d6c1
to
eb4780a
Compare
|
/benchmark plutus-benchmark:nofib |
|
Click here to check the status of your benchmark. |
|
Comparing benchmark results of 'plutus-benchmark:nofib' on 'd95daf39c' (base) and 'eb4780afd' (PR) Results table
|
|
Okay, close enough that I do doubt that the constr change was adding anything. We can always do it on its own later. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Don't look here