Disable for wasm everything that is disabled for windows and address issues with KnownTypeAst and wasm#7362
Disable for wasm everything that is disabled for windows and address issues with KnownTypeAst and wasm#7362palas wants to merge 6 commits into
wasm everything that is disabled for windows and address issues with KnownTypeAst and wasm#7362Conversation
palas
force-pushed
the
disable-exes-and-tests-in-wasm
branch
2 times, most recently
from
512e49a to
17c284e
Compare
palas
changed the title
wasm everything that is disabled for windows and address issues with KnownTypeAst and wasm
| toBuiltinMeaning _semvar SliceByteString = | ||
| let sliceByteStringDenotation :: Int -> Int -> BS.ByteString -> BS.ByteString | ||
| sliceByteStringDenotation start n xs = BS.take n (BS.drop start xs) | ||
| let sliceByteStringDenotation :: Int64 -> Int64 -> BS.ByteString -> BS.ByteString |
There was a problem hiding this comment.
If that is going to be run on a 32-bit system where correctness matters, then no: fromIntegral is going to silently truncate the given Int64s into Int32s.
|
/benchmark validation |
|
Click here to check the status of your benchmark. |
|
@zliu41 There is another issue that doesn't show in the tests but it is making reading the protocol parameters from wasm fail (because of cost models apparently), and I think it is related to this: This is the error I get: Not sure why we just got this error with the latest changes? Some new cost param that exceeds 32bit int? |
@palas That's not a recent change. Neither is any of the other usage of |
|
Comparing benchmark results of 'validation' on 'b7e75c9807' (base) and '0ddb89848c' (PR) Results table
|
There was a problem hiding this comment.
It is not safe to run UPLC on a 32-bit machine. This is discussed in Note [Integral types as Integer]. There's also
{-|
The old implementation relied on this function which is safe
*only* for 64-bit systems. There were previously safety checks to fail compilation
on other systems, but we removed them since we only test on 64-bit systems afterall. -}
naturalToWord64Maybe :: Natural -> Maybe Word64
naturalToWord64Maybe n = fromIntegral <$> naturalToWordMaybe nNot to mention that SatInt is Int and not Int64.
There was also something recent that breaks on a 32-bit system, but I don't remember what (UPD: it was this).
Basically, if you're doing this for shits and giggles, go ahead, but if you actually want to run UPLC on a 32-bit systems, you absolutely can't do that.
| toBuiltinMeaning _semvar SliceByteString = | ||
| let sliceByteStringDenotation :: Int -> Int -> BS.ByteString -> BS.ByteString | ||
| sliceByteStringDenotation start n xs = BS.take n (BS.drop start xs) | ||
| let sliceByteStringDenotation :: Int64 -> Int64 -> BS.ByteString -> BS.ByteString |
There was a problem hiding this comment.
If that is going to be run on a 32-bit system where correctness matters, then no: fromIntegral is going to silently truncate the given Int64s into Int32s.
| let readBitDenotation :: BS.ByteString -> Int -> BuiltinResult Bool | ||
| readBitDenotation = Bitwise.readBit | ||
| let readBitDenotation :: BS.ByteString -> Int64 -> BuiltinResult Bool | ||
| readBitDenotation bs = Bitwise.readBit bs . fromIntegral |
| case uni of | ||
| DefaultUniArray arg -> do | ||
| case vec Vector.!? n of | ||
| case vec Vector.!? fromIntegral n of |
|
Oh, and the whole bitwise builtins business relies on the architecture being 64-bit, e.g. let bigSrcPtr :: Ptr Word64 = castPtr srcPtrCan it be safe to still use it on a 32-bit platform? I dunno, but I bet it wasn't written with that in mind, so again, if you're going to use this for something that matters, then you can't. |
|
|
@effectfully @zliu41. In order to know whether
It seems for UPLC, using
At least compilation used to work fine until 1.57 (included). It is no longer the case as of 1.58. But we don't know if there may be runtime issues anywhere. Maybe you can advise on this. If we can get everything but UPLC execution to work in
If no 32bit support can be guaranteed then we probably need to discontinue In any case, I cannot really help with any of these because I don't have the expertise, this PR was just best-effort hoping it would be some small issue, but obviously it isn't. So, which level of 32bit support can you feasibly guarantee? Are there any other levels I am not considering? |
Right now, 32-bit is explicitly precluded and you simply cannot run UPLC on anything 32-bit if you at all care about getting the correct behavior (I'm not talking about efficiency here). This is an assumption that has been built-in for a very long time, so backtracking on that now would be a massive effort, someone will have to review basically every single line of critical code to verify that it's safe to run on a 32-bit system. If you only want things to build, I think it's fine. The benchmark results are almost certainly nonsense, those
Well, I no longer work on the project, but the answer is 0. "It builds but running it will result in undefined behavior" |
What else is useful to you, if you can't use the UPLC evaluator in wasm?
It can continue but you'll need a different evaluator implementation. There are already several other UPLC evaluator implementations in other languages (e.g., talk to Lucas Rosa), some of which may support 32-bit systems better.
I'm surprised the compilation was working. I still don't know what it is in 1.58 that broke it. We can do a bisect, but given what Roman said above, do you think it's still worth doing? @palas |
I think it is mainly the fact that it is integrated in the types of ledger. So, inspecting UPLC code, parsing of cost models within protocol parameters. There may be more things. The risk is that it may mangle some of those values in 32bits, if it makes assumptions that only hold for 64bit.
Maybe in the long term it won't. But if we break the compilation and tests of It is probably an issue with Is the current patch mergeable so far? I think the concerns about |
|
Nice! The last commit I pushed passes the I guess the implementation of multiplication may be less efficient than the previous one, but maybe even that can be fixed, there are several techniques to check for overflow, it would be a matter of measuring |
|
/benchmark validation |
|
Click here to check the status of your benchmark. |
|
Comparing benchmark results of 'validation' on 'b7e75c980' (base) and '259154a19' (PR) Results table
|
| let a' = toInteger a | ||
| b' = toInteger b | ||
| r = a' * b' | ||
| in if r > maxBoundInteger |
There was a problem hiding this comment.
Yeah you can't do this, it's very inefficient.
There was a problem hiding this comment.
I probably can do much better, this was just to know if it fixed the issue
There was a problem hiding this comment.
I think it's okay:
original core:
timesSI :: SatInt -> SatInt -> SatInt
timesSI
= \ (ds :: SatInt) (ds1 :: SatInt) ->
case ds `cast` <Co:1> :: ... of { I# x# ->
case ds1 `cast` <Co:1> :: ... of { I# y# ->
case mulIntMayOflo# x# y# of {
__DEFAULT ->
case andI# (># x# 0#) (># y# 0#) of {
__DEFAULT ->
case andI# (># x# 0#) (<# y# 0#) of {
__DEFAULT ->
case andI# (<# x# 0#) (># y# 0#) of {
__DEFAULT ->
case andI# (<# x# 0#) (<# y# 0#) of {
__DEFAULT -> overflowError;
1# -> maxInt `cast` <Co:2> :: ...
};
1# -> minInt `cast` <Co:2> :: ...
};
1# -> minInt `cast` <Co:2> :: ...
};
1# -> maxInt `cast` <Co:2> :: ...
};
0# -> (I# (*# x# y#)) `cast` <Co:2> :: ...
}
}
}
new core:
timesSI :: SatInt -> SatInt -> SatInt
timesSI
= \ (ds :: SatInt) (ds1 :: SatInt) ->
case ds `cast` <Co:1> :: ... of { I64# a ->
case ds1 `cast` <Co:1> :: ... of { I64# b ->
let {
b_native :: Int#
b_native = int64ToInt# b } in
let {
a_native :: Int#
a_native = int64ToInt# a } in
case mulIntMayOflo# a_native b_native of {
__DEFAULT ->
case ==# (># a_native 0#) (># b_native 0#) of {
__DEFAULT -> $fBoundedInt64_$cminBound `cast` <Co:2> :: ...;
1# -> $fBoundedInt64_$cmaxBound `cast` <Co:2> :: ...
};
0# ->
(I64# (intToInt64# (*# a_native b_native))) `cast` <Co:2> :: ...
}
}
}
| then maxBound | ||
| else | ||
| if isTrue# ((x# <=# 0#) `andI#` (y# ># 0#)) | ||
| if a < 0 && b > 0 && r > 0 |
There was a problem hiding this comment.
This may also be significantly less efficient than before.
palas
force-pushed
the
disable-exes-and-tests-in-wasm
branch
from
1ddc59e to
11a71c8
Compare
|
/benchmark validation |
palas
force-pushed
the
disable-exes-and-tests-in-wasm
branch
from
05643eb to
2a8d568
Compare
|
/benchmark validation |
|
/benchmark nofib |
|
Click here to check the status of your benchmark. |
|
no clue where +10.1% bench result is coming from. It's about 5% faster on my local machine |
|
let's see if the rebased version does better |
|
Comparing benchmark results of 'validation' on 'c8f962ae75' (base) and '2a8d568e71' (PR) Results table
|
|
Click here to check the status of your benchmark. |
|
Comparing benchmark results of 'nofib' on 'c8f962ae75' (base) and '2a8d568e71' (PR) Results table
|
…`SatInt` on 64-bit
|
/benchmark validation |
|
/benchmark nofib |
|
Click here to check the status of your benchmark. |
|
I think the culprit was |
|
Comparing benchmark results of 'validation' on 'c8f962ae75' (base) and 'b1d89d98f7' (PR) Results table
|
|
Click here to check the status of your benchmark. |
|
Comparing benchmark results of 'nofib' on 'c8f962ae75' (base) and 'b1d89d98f7' (PR) Results table
|
|
/benchmark data |
|
/benchmark nofib |
|
Click here to check the status of your benchmark. |
|
Comparing benchmark results of 'data' on 'c8f962ae75' (base) and 'b1d89d98f7' (PR) Results table
|
|
Click here to check the status of your benchmark. |
|
Still hate this PR. It pretends that UPLC can run on a 32-bit platform, which it absolutely cannot, see this message and the one below. What's the point to build on a 32-bit platform if you can't run there? And complicate actual critical-path code for that. Like, this project isn't a property of Intersect, it belongs to the Cardano community, and so I as a member ask: why are you complicating critical-path code to make unrunnable thing buildable? What's the motivation there? |
|
Comparing benchmark results of 'nofib' on 'c8f962ae75' (base) and 'b1d89d98f7' (PR) Results table
|
|
Honestly, I think it's best to just remove |
Even if wasm doesn't support UPLC, there is a lot more to Cardano than UPLC, and we cannot use any of it if Plutus doesn't even compile to 32 bit. I am no longer asking for full 32 bit compatibility. Also Plutus is the only component in Cardano that doesn't compile to 32 bit. Not sure why it doesn't too, because Cardano is meant to be a universal platform, so it seems a very arbitrary decision to assume 64 bit words for the implementation of an abstract machine, but never mind that. Haskell is a platform independent language and the only reason I see why this project is not working in 32 bits (at least for basic functionalities) is because it is using a machine dependent type Also, it should be possible to have all the changes necessary compiled conditionally to 32bit and this shouldn't affect the 64bit compilation at all. The changes I did in the 64bit Ultimately, it is not my choice, but not allowing Plutus to acknowledge 32bit just gives me a lot of work each time a new release of Plutus comes out, because I have to keep rebasing this PR and resolving the conflicts. And, like I say, Plutus is the only component in Cardano that has this problem. Unfortunately, your suggestion of using an alternative implementation of Plutus doesn't help at all with compiling |
Ah, it seems @SeungheonOh already basically did that: so basically calling I could go even further and do the types conditional too, but that would clutter much more 🤷♂️ |
4 participants
In the same way that they are disabled for windows, this PR disables tests and executables for
wasm, since they currently don't work inwasm.We also specify the bits of some
Ints andWords so that it works when they are 32 bit by default, and thus solve an issue withwasmandKnownTypeAst.Pre-submit checklist: