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

Add Memory utility library #5189

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

Conversation

ernestognw
Copy link
Member

@ernestognw ernestognw commented Sep 4, 2024

Picked from #5094

PR Checklist

  • Tests
  • Documentation
  • Changeset entry (run npx changeset add)

Copy link

changeset-bot bot commented Sep 4, 2024

🦋 Changeset detected

Latest commit: a7e61c3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
openzeppelin-solidity Minor

Not sure what this means? Click here to learn what changesets are.

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

@ernestognw ernestognw marked this pull request as ready for review September 4, 2024 17:35

/// @dev Memory utility library.
library Memory {
type Pointer is bytes32;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we call that type type FMP is bytes32; ?

Copy link
Member Author

@ernestognw ernestognw Sep 5, 2024

Choose a reason for hiding this comment

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

I'd be better with FreePointer as FMP is not obvious at first.

EDIT: Acutally I do prefer just Pointer, since it's not always the free memory pointer. Anyone can get a point to wherever they want with asPointer

Copy link
Collaborator

Choose a reason for hiding this comment

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

would we rename the getFreePointer and setFreePointer to getFreeMemoryPointer and setFreeMemoryPointer ?

test/utils/Memory.t.sol Outdated Show resolved Hide resolved
@Amxx
Copy link
Collaborator

Amxx commented Sep 5, 2024

IMO this PR should include the usage of this lib

@ernestognw ernestognw requested a review from Amxx September 5, 2024 18:21
}
----

In this way, new memory will be allocated in the space where the `returndata` and `callData` used to be, potentially reducing memory expansion costs by shrinking the its size at the end of the transaction and resulting in gas savings.
Copy link
Collaborator

@Amxx Amxx Sep 19, 2024

Choose a reason for hiding this comment

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

Suggested change
In this way, new memory will be allocated in the space where the `returndata` and `callData` used to be, potentially reducing memory expansion costs by shrinking the its size at the end of the transaction and resulting in gas savings.
In this way, memory allocated to accomodate the `callData` and `returndata` is freed. This allows other memory operations to reuse that space, thus reducing the memory expansion costs of these operations. In particular, this allows performing many `callFoo` in a loop with limite memory expansion costs.

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.

2 participants