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

Improve gas efficiency of EnumerableMap #2518

Merged
merged 23 commits into from Feb 18, 2021

Conversation

Amxx
Copy link
Collaborator

@Amxx Amxx commented Feb 9, 2021

EnumerableSet:

  • use of inline assembly to optimise add
  • use of inline assembly to optimise remove

EnumerableMap:

  • Using an EnumerableSet for keys management + simple key => value mapping
  • Optimizes for key lookup rather then index lookup

Gas report
Note: gas report doesn't look to be such a good indication. tests on remix show the assembly to be more efficient, but its difficult to quantify.

Function Min Max Average
EnumerableSetMock.add 23630 → 23630 85403 → 85403 65740 → 65740
EnumerableSetMock.remove 22460 → 21450 26666 → 25656 25193 → 24471
EnumerableMapMock.set 25809 → 25038 105902 → 106023 87441 → 87400
EnumerableMapMock.remove 23592 → 24023 32502 → 28229 29053 → 26577

PR Checklist

  • Tests
  • Changelog entry

@Amxx Amxx force-pushed the refactor/new-enumerablemap branch from 7741735 to de4b0ae Compare February 9, 2021 10:40
@Amxx Amxx force-pushed the refactor/new-enumerablemap branch from de4b0ae to 27a0a89 Compare February 9, 2021 10:47
@Amxx Amxx marked this pull request as ready for review February 17, 2021 17:55
Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

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

I have concerns about the use of assembly but the changes to EnumerableMap look good.

contracts/utils/EnumerableMap.sol Show resolved Hide resolved
contracts/utils/EnumerableSet.sol Outdated Show resolved Hide resolved
contracts/utils/EnumerableSet.sol Outdated Show resolved Hide resolved
contracts/utils/EnumerableSet.sol Outdated Show resolved Hide resolved
Amxx and others added 2 commits February 17, 2021 22:47
@frangio frangio changed the title Improve gas efficiency of Enumerable structures Improve gas efficiency of EnumerableMap Feb 18, 2021
@frangio
Copy link
Contributor

frangio commented Feb 18, 2021

This is good but needs a changelog.

Copy link
Contributor

@frangio frangio 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. Thank you!

@frangio frangio enabled auto-merge (squash) February 18, 2021 15:15
@frangio frangio merged commit e66e3ca into OpenZeppelin:master Feb 18, 2021
@Amxx Amxx deleted the refactor/new-enumerablemap branch February 18, 2021 16:21
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.

None yet

2 participants