[WIP] use base64 over base64-bytestring #1631

chessai · 2023-04-20T05:39:46Z

I added SIMD to base64 (See PR).

Per profiling, chainweb-node spends about 6% of its time performing base64 decode on blocks. According to the microbenchmarks, this should hopefully offer a speedup of ~15x on decode of things that are the average block size.

The other nice thing about the base64 library is that it has the option of being more type-safe.
All these assertions I've added here are unsafe, but, they were just implicit before.

jwiegley · 2023-04-20T06:03:15Z

Does this pass replay?

chessai · 2023-04-20T07:06:22Z

Does this pass replay?

No; I think that we actually do need to keep base64-bytestring for certain operations where we care about the on-chain error messages. Right now, a replay fails immediately because of one such instance.

larskuhtz

We should double check that all use of base64 is the URL variant without padding.

Regarding replay: this should be fine since chainweb doesn't put base64 encoded things on-chain. Base64 is used only as textual representation of binary data (mostly in JSON). Merkle hashes of on-chain data are computed on the plain binary representation.

However, I am not 100% certain about the use of base64 within pact service. In theory it shouldn't affect on-chain data (in particular errors in pact service shouldn't be included on-chain), but we should double check that.

larskuhtz · 2023-04-20T07:07:22Z

chainweb.cabal

-        , base64-bytestring >= 1.0 && < 1.1
-            -- due to change in error message that is inlined in pact


👍 the upper bound is tracked in the cabal.project file.

larskuhtz · 2023-04-20T07:08:54Z

chainweb.cabal

@@ -325,8 +325,7 @@ library
        , attoparsec >= 0.13
        , base >= 4.12 && < 5
        , base16-bytestring >= 0.1
-        , base64-bytestring >= 1.0 && < 1.1
-            -- due to change in error message that is inlined in pact
+        , base64 >= 0.5 && < 0.6


Is this known to break with base64 >=0.6? Otherwise please remove, since we don't manually track "soft" upper bounds.

Suggested change

, base64 >= 0.5 && < 0.6

, base64 >= 0.5

larskuhtz · 2023-04-20T07:10:39Z

src/Chainweb/Mempool/Mempool.hs

@@ -746,7 +750,7 @@ getDecimal = do

 mockDecode :: ByteString -> Either String MockTx
 mockDecode s = do
-    s' <- B64.decode s
+    s' <- first T.unpack $ B64U.decodeBase64 $ B64.assertBase64 @'B64.UrlPadded @_ s


I am a bit concerned about B64.UrlPadded. Does this mean the it uses padding? All use of base64 throughout chainweb should be base64Url without padding.

larskuhtz · 2023-04-20T07:14:32Z

src/Chainweb/Utils.hs

-    . first (Base64DecodeException . T.pack)
-    . B64.decode
+    . first Base64DecodeException
+    . B64.decodeBase64
+    . B64.assertBase64 @'B64.StdPadded @_


Are the functions for the unpadded (and non-url) variants used anywhere in the code? If not we should consider removing them, so that nobody uses them. This is to keep used encodings simple and consistent.

larskuhtz · 2023-04-20T07:26:07Z

chainweb.cabal

@@ -325,8 +325,7 @@ library
        , attoparsec >= 0.13
        , base >= 4.12 && < 5
        , base16-bytestring >= 0.1
-        , base64-bytestring >= 1.0 && < 1.1
-            -- due to change in error message that is inlined in pact
+        , base64 >= 0.5 && < 0.6


It seems that version 0.5 of base64 hasn't yet been released. The latest version on Hackage is 0.4.2.4. 0.5 is only available as source repository from Github. If 0.5 is actually required, we would have to ask the maintainer (@emilypi?) to release 0.5 on Hackage or include it as source repository package in cabal.project.

It hasn't been released and I"m still working on the PR, which will significantly affect this PR.

emilypi · 2023-04-20T19:34:35Z

I like your enthusiasm @chessai! However, this is jumping the gun alittle bit. The PR that is going in to serve as the basis for base64-0.5 is going to streamline alot of the functionality changed in this PR that does alot of redundant work (adding pads, removing pads, converting to/from text etc), so I ask that we hold off on it here until I'm confident I can nail down the api.

That said, with the addition of SIMD support, I do endorse this change, and I would like to revert from base64-bytestring at some point, because it is very suboptimal (by design, per Bos + HVR's request).

Does this pass replay?

It should @jwiegley, since we're going in at the chainweb level, and the problems with replay w.r.t. Base64 are with the Pact natives wholesale committing to the error format returned by base64-bytestring. I'm working on reverting those changes in the latest base64 major version so that we can switch to that in Pact, but the changes here do not affect Pact, so they shouldn't affect block hashes.

chessai · 2023-04-20T19:47:52Z

I like your enthusiasm @chessai! However, this is jumping the gun alittle bit. The PR that is going in to serve as the basis for base64-0.5 is going to streamline alot of the functionality changed in this PR that does alot of redundant work (adding pads, removing pads, converting to/from text etc), so I ask that we hold off on it here until I'm confident I can nail down the api.

That said, with the addition of SIMD support, I do endorse this change, and I would like to revert from base64-bytestring at some point, because it is very suboptimal (by design, per Bos + HVR's request).

Does this pass replay?

It should @jwiegley, since we're going in at the chainweb level, and the problems with replay w.r.t. Base64 are with the Pact natives wholesale committing to the error format returned by base64-bytestring. I'm working on reverting those changes in the latest base64 major version so that we can switch to that in Pact, but the changes here do not affect Pact, so they shouldn't affect block hashes.

Thanks,

Yeah, after talking to you yesterday/digging around in your PR/the codebase more, it seems like this is indeed jumping the gun.
I just saw those speedups and I was really excited.

I'll leave this open for now, because I do think it's something we should return to.

chessai self-assigned this Apr 20, 2023

chessai force-pushed the chessai/base64-simd branch from 831561e to 11b6cce Compare April 20, 2023 05:44

jwiegley requested review from emilypi, edmundnoble and larskuhtz April 20, 2023 06:02

chessai marked this pull request as draft April 20, 2023 06:10

use base64 over base64-bytestring

5eb5591

chessai force-pushed the chessai/base64-simd branch from 11b6cce to 5eb5591 Compare April 20, 2023 06:30

larskuhtz requested changes Apr 20, 2023

View reviewed changes

larskuhtz reviewed Apr 20, 2023

View reviewed changes

chessai closed this Jan 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] use base64 over base64-bytestring #1631

[WIP] use base64 over base64-bytestring #1631

chessai commented Apr 20, 2023

jwiegley commented Apr 20, 2023

chessai commented Apr 20, 2023

larskuhtz left a comment

larskuhtz Apr 20, 2023

larskuhtz Apr 20, 2023

larskuhtz Apr 20, 2023

larskuhtz Apr 20, 2023

larskuhtz Apr 20, 2023

emilypi Apr 20, 2023

emilypi commented Apr 20, 2023

chessai commented Apr 20, 2023

		, base64-bytestring >= 1.0 && < 1.1
		-- due to change in error message that is inlined in pact

[WIP] use base64 over base64-bytestring #1631

[WIP] use base64 over base64-bytestring #1631

Conversation

chessai commented Apr 20, 2023

jwiegley commented Apr 20, 2023

chessai commented Apr 20, 2023

larskuhtz left a comment

Choose a reason for hiding this comment

larskuhtz Apr 20, 2023

Choose a reason for hiding this comment

larskuhtz Apr 20, 2023

Choose a reason for hiding this comment

larskuhtz Apr 20, 2023

Choose a reason for hiding this comment

larskuhtz Apr 20, 2023

Choose a reason for hiding this comment

larskuhtz Apr 20, 2023

Choose a reason for hiding this comment

emilypi Apr 20, 2023

Choose a reason for hiding this comment

emilypi commented Apr 20, 2023

chessai commented Apr 20, 2023