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

Possible optimizations in the Heap #64

Open
QGarchery opened this issue Aug 16, 2022 · 5 comments
Open

Possible optimizations in the Heap #64

QGarchery opened this issue Aug 16, 2022 · 5 comments
Assignees

Comments

@QGarchery
Copy link
Collaborator

We can also implement:

Originally posted by @QGarchery in #23 (comment)

@QGarchery QGarchery changed the title We can also implement: Possible optimizations in the Heap Aug 16, 2022
@MerlinEgalite
Copy link
Contributor

Can we close? @QGarchery

@QGarchery
Copy link
Collaborator Author

I think we should do them in the basic heap, it should not be too difficult. The arguments against them for the use case of Morpho do not hold in general

@MerlinEgalite
Copy link
Contributor

Ok tagging @makcandrov and @Tristan22400 on it

@makcandrov
Copy link
Contributor

makcandrov commented Apr 28, 2023

I'm not in favor of resolving #38 because it will increase gas consumption in general cases to save gas in a rare case where two accounts have the same value.

Regarding #36, it is correct that we can make the heap more usable by adding/replacing some functions. Which functions should be added?

  • A function update instead of increase and decrease.
  • A function pop that removes the first account of the heap.
  • A function popAndInsert that removes the first account and inserts another account.

I'm in favor of the first two functions, but not the third.

@QGarchery
Copy link
Collaborator Author

I'm not in favor of resolving #38 because it will increase gas consumption in general cases to save gas in a rare case where two accounts have the same value.

I don't think we can assume that the rare case is when two accounts have the same value. I understand that there are use cases where indeed accounts should rarely have the same value (and that's why it's not implemented for Morpho, see this comment). But this implementation is meant as a general purpose heap, so we can make no assumption about how it's used.

I like the update function: it seems to be lightweight in terms of implementation and would be useful in a lot of cases

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants