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

perf: optimizing parameters sizing #207

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

perf: optimizing parameters sizing #207

wants to merge 17 commits into from

Conversation

AuHau
Copy link
Member

@AuHau AuHau commented Jan 8, 2025

I ran into yet another instance where we were using uint256 for a time interval (duration, expiry, etc.), which does not make sense and makes working with these parameters in nim-codex more annoying, so my patience ran out, and I scaled down all the time relevant params to uint64. And since I was at it, I also started to look into others like slotSize and slots. I scaled those to uint64 as well. The slots (eq. number of slots in storage request) is IMHO fine having uint64, but the slotSize gives us 16 exabytes, which is IMHO also fairly alright.

I would like to point out, that with these changes and the reshuffling of the order in Ask we are saving 3x uint256, as the 4*uint64 will fit into one "word" (eq. uint256).

Let me know what you think about these changes ;-)

@AuHau AuHau requested review from markspanbroek and emizzle January 8, 2025 14:07
@AuHau AuHau force-pushed the perf/params-sizing branch from 42b979e to 7e69a3b Compare January 8, 2025 14:08
Copy link
Member

@markspanbroek markspanbroek left a comment

Choose a reason for hiding this comment

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

Nice, thanks for fixing this @AuHau!

test/ids.js Outdated Show resolved Hide resolved
test/ids.js Outdated Show resolved Hide resolved
test/ids.js Outdated Show resolved Hide resolved
@AuHau AuHau requested a review from markspanbroek January 10, 2025 08:27
@AuHau
Copy link
Member Author

AuHau commented Jan 10, 2025

@markspanbroek I did more optimisations, could you please re-review? Since I will be going over nim-codex anyhow, I would prefer to do it in one complex sweep so we do not have to return to this later on.

Copy link
Member

@markspanbroek markspanbroek left a comment

Choose a reason for hiding this comment

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

Nice, looks good @AuHau!

contracts/Marketplace.sol Show resolved Hide resolved
Copy link
Collaborator

@emizzle emizzle left a comment

Choose a reason for hiding this comment

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

Thanks for doing this, Adam. It's a long overdue change.

Agree with Mark's comment on changed requestEnd. And two other function signature updates (both _slotPayout overloads).

Will approve as these are small changes.

contracts/Marketplace.sol Show resolved Hide resolved
@AuHau AuHau self-assigned this Jan 16, 2025
@AuHau AuHau added the Marketplace See https://miro.com/app/board/uXjVNZ03E-c=/ for details label Jan 16, 2025
@AuHau AuHau force-pushed the perf/params-sizing branch from ea30fbd to d99a897 Compare January 16, 2025 15:56
@emizzle
Copy link
Collaborator

emizzle commented Jan 23, 2025

Ready to merge @AuHau ?

@AuHau
Copy link
Member Author

AuHau commented Jan 24, 2025

No, there is no nim-codex counterpart yet.

@AuHau AuHau force-pushed the perf/params-sizing branch from 4740c86 to 93fece8 Compare January 31, 2025 11:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Marketplace See https://miro.com/app/board/uXjVNZ03E-c=/ for details
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants