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

feat: function ordering #2

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

ZeroEkkusu
Copy link

Gm, sers,

I've added a tip about ordering functions by Method ID!

If more contributions are welcome, I'll add new tips as I discover them.

🌸

@devtooligan
Copy link
Collaborator

devtooligan commented Feb 9, 2022

@ZeroEkkusu Awesome!! Thank you! We are still finalizing the formatting on this. I am going to prob spend some time on this over this coming weekend and hopefully we make some fwd progress and agree on a format.

As far as this tip goes in particular, I'm not super clear by your description or the example how this works or how I would save gas. Also, is this gas savings at runtime, or on deployment? When called internally or by EOA or an outside contract? Can you please clarify a little more. In your example, it would be nice to know exactly how much gas the unoptimized version costs, and then how much the second one costs so we could see the difference. We need to know exactly what to do to see the gas savings. From your example right now, the numbers you included add up to 66 on both.

One last thing, @transmissions11 would prefer to use more illustrative variable names (he explicitly said no "Foo" or "Bar") so maybe you can change the function names to mostCalled, leastCalled or something since as you mention in the example there is one called more often than another.

Also please include tests, one for the unoptimized example and one for the optimized.

@ZeroEkkusu
Copy link
Author

Will do! Thank you for the feedback.

@transmissions11
Copy link
Contributor

this is awesome stuff, agreed with @devtooligan's review, will merge once ready

🌸

* forge install: ds-test

* Reorganize repo
@ZeroEkkusu
Copy link
Author

I have clarified how the optimization works and added tests. If further improvement is needed before merging, I'll try explaining it better.

@devtooligan @transmissions11 please see if you like this repo format 👉👈

It's like a big project with separate, smaller contracts sharing the same config. They're divided into categories and I have updated the readme to be easier to navigate. You can run make compare c=<ContractName> to quickly test different optimizations. This will be useful as the repo grows. @devtooligan I created Operators.sol and Operators.t.sol placeholders for some of your optimizations, although I'm still experimenting with the ways to group them (e.g. folders, etc).

Let me know what you think, sers.

@devtooligan
Copy link
Collaborator

@ZeroEkkusu

Cool!

Ok, I know which optimization you're talking about now. Optimizing the fn signature using something like this https://emn178.github.io/solidity-optimize-name/ right?

While I don't think it would be highly desirable because of the funky fn names, I do think it's worth mentioning in this guide.

I'll take a look at the other work you did on the architecture and styling over the weekend. "make compare" sounds promising, t11 was talking about doing something similar that maybe we could include in our CI runs but I hadn't even had a chance to think on it yet.

Thanks again for all your work on this.

@ZeroEkkusu
Copy link
Author

Oke, you decide what to do with it. I'll add a few details I forgot yesterday, regardless.

Yes, those names could be a little awkward when writing code. Autocomplete worked fine for me so I didn't pay much attention to that! :P

Here's an example of code readability when the optimization is used. Also, Natspec can abstract those names away to some extent, but that's off-topic.

As for make compare, we could make a script with more complex logic, especially if we standardize names of all tests e.g. testUnoptimized<something> - to calculate gas savings, etc.

* add emojis

* fix links
@devtooligan
Copy link
Collaborator

devtooligan commented Feb 13, 2022

Hi @ZeroEkkusu.

I've reviewed this. Very nice work. Very thoughtful.

I've just updated a big pr I have in for @transmissions11 review. I would suggest you hold off on further work until he approves that one so you don't end up wasting any time formatting.

I see that you’ve created different contracts for different “groupings”. I see you have created one grouping for “Operators” – and another for “Compiler”. These ares some good ideas. But I can think of so many different ways to group them. For now I’m thinking we don’t start grouping things at all. I believe after we get a long list of optimizations together, a clear pattern will present itself and we can determine at that time. Last I checked with @transmissions11, he feels the same way.

@ZeroEkkusu
Copy link
Author

SGTM

@ZeroEkkusu ZeroEkkusu mentioned this pull request Feb 14, 2022
9 tasks
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