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

test(world): add module gas reports for setting many keys #771

Closed
wants to merge 21 commits into from

Conversation

yonadaa
Copy link
Contributor

@yonadaa yonadaa commented May 9, 2023

Tests setting 10, 100, 1000, 10000 records with KeysInTable and KeysWithValue.

@vercel
Copy link

vercel bot commented May 9, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
mud-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 11, 2023 11:30am
mud-example-minimal-react ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 11, 2023 11:30am

@yonadaa yonadaa changed the title feat: add KeysWithValue gas reports for 10 and 100 keys feat: add KeysWithValue gas reports for setting many keys May 9, 2023
@yonadaa yonadaa changed the title feat: add KeysWithValue gas reports for setting many keys feat: add module gas reports for setting many keys May 10, 2023
@yonadaa yonadaa marked this pull request as ready for review May 11, 2023 08:23
@yonadaa yonadaa requested review from alvrs and holic as code owners May 11, 2023 08:23
@alvrs alvrs changed the title feat: add module gas reports for setting many keys test(world): add module gas reports for setting many keys Jun 16, 2023
@changeset-bot
Copy link

changeset-bot bot commented Jun 16, 2023

⚠️ No Changeset found

Latest commit: 865adc9

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

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

alvrs
alvrs previously approved these changes Jun 16, 2023
@alvrs
Copy link
Member

alvrs commented Jun 16, 2023

unclear to me atm why the gas report step fails in the checks but work locally

@alvrs
Copy link
Member

alvrs commented Jun 16, 2023

alright seems like setting 10k keys was too much for the CI

@alvrs
Copy link
Member

alvrs commented Jun 16, 2023

Unfortunately this change increases the time for the Build and validate artifacts action from 5 minutes to 10 minutes (see https://github.com/latticexyz/mud/actions/runs/5294523734/jobs/9583964860 for comparison), which unfortunately is not a good tradeoff for adding these gas reports.

Because of that I'll close this PR for now, but we can look at it for reference when optimizing the KeysInTable / KeysWithValue modules.

We should also find a way to run these gas reports more efficiently, the Build and validate artifacts is already the slowest action without this change.

@alvrs alvrs closed this Jun 16, 2023
@holic
Copy link
Member

holic commented Jun 16, 2023

We should also find a way to run these gas reports more efficiently, the Build and validate artifacts is already the slowest action without this change.

Looks like gas reports are the bulk of this:

image

I'm hoping we can soon move over to Vulcan gas reports (or pull in the same strategy) once they land, which will mean the gas reports and tests are run at the same time: nomoixyz/vulcan#104

@holic holic deleted the gas-report-many-keys branch June 23, 2023 11:34
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.

4 participants