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

refactor(store): refactor tightcoder, optimize record encoder #1183

Closed
wants to merge 14 commits into from

Conversation

dk1a
Copy link
Contributor

@dk1a dk1a commented Jul 19, 2023

This addresses the major structural issue with tightcoder and full record encoding. It's not everything and I'll PR minor tightcoder changes later so they don't obfuscate gas effects here.
Depends on #1190 for a correct gas diff
Fixes parts of #1132

@changeset-bot
Copy link

changeset-bot bot commented Jul 19, 2023

🦋 Changeset detected

Latest commit: 819a36b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 26 packages
Name Type
@latticexyz/cli Patch
@latticexyz/common Patch
@latticexyz/store Patch
@latticexyz/std-client Patch
@latticexyz/block-logs-stream Patch
@latticexyz/config Patch
@latticexyz/dev-tools Patch
@latticexyz/network Patch
@latticexyz/protocol-parser Patch
@latticexyz/store-cache Patch
@latticexyz/store-sync Patch
@latticexyz/utils Patch
@latticexyz/world Patch
@latticexyz/react Patch
@latticexyz/ecs-browser Patch
@latticexyz/phaserx Patch
@latticexyz/recs Patch
create-mud Patch
@latticexyz/gas-report Patch
@latticexyz/noise Patch
@latticexyz/schema-type Patch
@latticexyz/services Patch
@latticexyz/solecs Patch
solhint-config-mud Patch
solhint-plugin-mud Patch
@latticexyz/std-contracts Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@holic
Copy link
Member

holic commented Jul 20, 2023

while we're here, can we rename TightCoder to BytePack?

@dk1a
Copy link
Contributor Author

dk1a commented Jul 20, 2023

why BytePack? that sounds like we are doing new custom byte packing
These libs are hacky workarounds for missing abi functions
The decoder part isn't going away any time soon, but for the encoder ideally I'd PR abi.encodeTightlyPacked when I have a couple weeks to understand solidity sources

@dk1a dk1a changed the title WIP refactor tightcoder refactor(store): refactor tightcoder, optimize record encoder Jul 25, 2023
@dk1a dk1a marked this pull request as ready for review July 25, 2023 13:12
@dk1a dk1a requested review from alvrs and holic as code owners July 25, 2023 13:12
@@ -34,7 +34,7 @@
}
},
"scripts": {
"build": "pnpm run build:mud && pnpm run build:abi && pnpm run build:typechain && pnpm run build:js",
"build": "pnpm run generate-tightcoder && pnpm run build:mud && pnpm run build:abi && pnpm run build:typechain && pnpm run build:js",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should this be done differently? (the only other precedent of custom codegen is in cli, but that is test-only)

@holic
Copy link
Member

holic commented Jul 26, 2023

why BytePack? that sounds like we are doing new custom byte packing These libs are hacky workarounds for missing abi functions The decoder part isn't going away any time soon, but for the encoder ideally I'd PR abi.encodeTightlyPacked when I have a couple weeks to understand solidity sources

Sorry, meant to come back around to this! @alvrs and I had been doing a bunch of brainstorming/chatting around naming (both in terms of self-describing names and "branding") and both liked BytePack for this collection of tools/libraries.

Comment on lines +175 to +198
bytes32 _encodedLengths = PackedCounterLib.pack(_counters).unwrap();

uint256 _resultLength;
unchecked {
_resultLength = 32 + value.length * 4;
}

bytes memory _result;
uint256 _resultPointer;

/// @solidity memory-safe-assembly
assembly {
// allocate memory
_result := mload(0x40)
_resultPointer := add(_result, 0x20)
mstore(0x40, add(_resultPointer, and(add(_resultLength, 31), not(31))))
mstore(_result, _resultLength)

mstore(add(_resultPointer, 0), shl(0, _encodedLengths))

_resultPointer := add(_resultPointer, 32)
}
EncodeArray.encodeToLocation((value), _resultPointer);
return _result;
Copy link
Member

Choose a reason for hiding this comment

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

one general thought: having complex / low level logic like this in code that is produced by codegeneration (even if it's literally just a template string) is gonna be a red flag for folks during the audit. Is there any way you can think of how we could encapsulate this in a general function outside of codegen that can be audited separately, so that codegen code looks as simple as possible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah this is an interesting/unusual situation. Essentially I'm doing what solidity does with its IR codegen, but it's scuffed because I don't have access to all the solidity's yul utils. This part of MUD is more a language compiler / addition to solidity, than just a contract.
The encapsulated version would be what we had before this PR. This is not strictly better than before and I have considered this as a reason to reject this PR if you don't think we should do compiler-like yul codegen in MUD

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TLDR no, this is a critical part of the PR. You can just reject it, I'll make a much simpler PR refactoring ejs to typescript and some other minor things

Copy link
Member

Choose a reason for hiding this comment

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

I'd say if this is causing dramatic improvements in gas we should consider it, if the improvements are modest we should optimize for simplicity in the generated code

@dk1a
Copy link
Contributor Author

dk1a commented Jul 27, 2023

I think it's not worth it. The gas effects are certainly not dramatic. For now I'll split this up and PR the less contentious stuff, and consider the scary assembly after some more gas tests

(this PR should primarily affect big dynamic data, and we don't even have any gas tests for that)

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.

3 participants