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: normalize for loop declarations #476

Merged
merged 3 commits into from
Apr 5, 2024

Conversation

0xJabberwock
Copy link
Contributor

@0xJabberwock 0xJabberwock commented Apr 4, 2024

Description

Previously, I raised a concern about the possible gas savings that could be achieved by prefix incrementing or decrementing the iterator variable within declarations of for loops (see #468 (comment); #475). Anyways, after having implemented the suggestion, the gas snapshots didn't change.

Consequently, I researched and found out that compiling via_ir = true already applies this optimization and so equals the gas expenditures, but the same doesn't necessarily happen when using the legacy pipeline (see ethereum/solidity#14595). Another reason for the unaltered gas snapshots may be that the savings are negligible (less than 1 % change).

Moreover, explicitly setting the iterator variable to its default value while declaring it may also be a cause of increased gas consumption. Again, I haven't seen modifications in the gas snapshots after having applied this locally, so I opted for leaving such explicit initialization momentarily as it appeared to be enforced for every loop, despite it could be not. Has this been done on purpose?

Ultimately, in relation with the for comparisons, I tried cacheing the length of the arrays being iterated over and, similarly, the gas savings were either negligible or none.

Apart from that, this PR aims to standardize every Solidity for loop declaration across the codebase. For so, the iterator variable has been named i rather than the less frequent and longer index, and prefix increments and decrements have been enforced where applicable. Normalizing the comparisons by cacheing didn't seem feasible due to the ad hoc nature of each logic and stack too deep concerns.

Type of change

  • Bug fix
  • New feature
  • Breaking change
  • Dependency changes
  • Code refactor / cleanup
  • Documentation or wording changes
  • Other

Checklist:

  • The diff is legible and has no extraneous changes
  • Complex code has been commented, including external interfaces
  • Tests have 100% code coverage
  • The base branch is either main, or there's a description of how to merge

Issue Resolution

Closes #475.

@0xJabberwock 0xJabberwock self-assigned this Apr 4, 2024
Copy link
Collaborator

@wei3erHase wei3erHase left a comment

Choose a reason for hiding this comment

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

looks good, i don't have strong opinions on ++i vs i++, but i do prefer standarizing i over index.

Copy link
Collaborator

@EndymionJkb EndymionJkb left a comment

Choose a reason for hiding this comment

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

Not sure how the "index" ones snuck in there; we did have a policy to always use "i", "j", ..., unless the name is semantic (e.g., pathIndex, stepIndex or something), and would be confusing to be a single letter. A straight-up iterator should always be i,j,k,...

@0xJabberwock 0xJabberwock changed the title refactor: normalize for loop declarations refactor: normalize for loop declarations Apr 5, 2024
@0xJabberwock 0xJabberwock merged commit a1ca25a into main Apr 5, 2024
10 checks passed
@0xJabberwock 0xJabberwock deleted the refactor/prefix-increments-and-decrements branch April 5, 2024 17:57
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.

Gas optimizations within declarations of for loops
3 participants