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

Performance characteristics and memory use of exec_recursive #15363

Open
straight-shoota opened this issue Jan 21, 2025 · 2 comments
Open

Performance characteristics and memory use of exec_recursive #15363

straight-shoota opened this issue Jan 21, 2025 · 2 comments

Comments

@straight-shoota
Copy link
Member

Reading the code for #exec_recursive and #exec_recursive_cline in #15361 I'm wondering about a couple of things regarding this recursion detection mechanism. They're unrelated to the current refactor, so I'm starting a separate discussion.

  • What happens when the block raises? This must be expected because the mechanism is used for writing to an IO. Apparently the entries will never get cleared. We should probable handle exceptions explicitly via ensure.
  • Should we release the hash when it's been emptied? We don't need to keep occupying the memory if no recursive operation is ever used in that same fiber. On the other hand, if there is another recursive operation we could reuse the previous allocation. This could be significant if there's a lot of use. However, AFAIK hash memory never shrinks, so this could become a memory leak.
  • Finally, Hash might be an easy to use tool for this, but I would imagine it's also quite heavy with regard to heap allocations. In typical use cases there probably won't be that many elements in the hash anyways. Its size is defined by the nesting depth of container objects. I'd bet in the majority of cases this won't exceed 16 items which is the size up to which Hash does a linear scan anyway. So a more lightweight data structure, such as stack-allocated linked list, could perform similarly.

Performance considerations might not be too critical for this use case, but it's still worth to think about.
Potential memory leaks are always a relevant concern.

@ysbaddaden
Copy link
Contributor

ysbaddaden commented Jan 21, 2025

Raising: oh my, yes, definitely 😱

The Hash is indeed too complex.

Keeping a linked list of nodes allocated on the stack while we're dealing with recursion, means we won't have full benefit of the contiguous memory, but should still benefit from locality since the nodes shouldn't be too far apart on the stack. Nice 👍

Additional perks: no allocation, no memory leak, automatic cleanup 👍

@ysbaddaden
Copy link
Contributor

Even though p or pp aren't too important to optimize, #inspect could be used in production, and #clone even more!

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

2 participants