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

Self reference in ReadAccountMapEntry #34786

Closed
Lichtso opened this issue Jan 15, 2024 · 5 comments · Fixed by #35351
Closed

Self reference in ReadAccountMapEntry #34786

Lichtso opened this issue Jan 15, 2024 · 5 comments · Fixed by #35351
Assignees

Comments

@Lichtso
Copy link
Contributor

Lichtso commented Jan 15, 2024

Problem

This single use of ouroboros::self_referencing (which was added in #12787) causes ~4.2% of all heap allocation operations during replay:

#[self_referencing]

Self referencing structs are an anti-pattern and ouroboros should not be used casually. The reason is not that the Rust borrow checker is missing a feature or anything like that, but that structs can be moved and thus their address change. The only way to prevent that is to box (heap allocate) and pin the structure, effectively forbidding it to be reallocated (which is what ouroboros is doing under the hood).

In this case here the structure is tiny (it falls in the 16 byte size class, the smallest) and it is constructed a lot. Thus, it does not contribute much to the memory pressure, but it does to the memory management overhead.

Proposed Solution

Refactor the way these structures reference each other to avoid self references.

@brooksprumo
Copy link
Contributor

@Lichtso Can you share how to do the profiling, so that I can compare fixes? Thanks in advance.

@carllin
Copy link
Contributor

carllin commented Jan 20, 2024

yaasss fix my bugs king @brooksprumo

@jeffwashington
Copy link
Contributor

I did some digging.
on mnb, we are creating about 30k of these per slot. They do not get called during tx processing, but during new_from_parent type activities. I'll keep digging some more.

@jeffwashington
Copy link
Contributor

see: #34918

@jeffwashington
Copy link
Contributor

There is also great value in simplifying the client. getting rid of this use of self-ref does simplify things. Over time, the accounts index has drastically changed in its scalability, implementation, and binning. The result is the behavior is very different than it was before and the self-ref struct is complicated and no longer the best way to do this.

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 a pull request may close this issue.

4 participants