ruby / prism

Prism Ruby parser
https://ruby.github.io/prism/
MIT License
806 stars 136 forks source link

Memory management #1337

Open kddnewton opened 1 year ago

kddnewton commented 1 year ago

At the moment, we allocate memory as we need it, making a single malloc call for every node in the tree, as well as whichever fields on those nodes need memory. When we're done with the tree, we recursively visit the tree and deallocate all at once. This is a really good candidate for arena allocation. But more than that, it would be good enough just to centralize our memory management functions, which we need to do anyway. Below are a couple of tasks that are related to memory that would improve our general memory story.

The first two of these tasks are necessary for our integration into CRuby. The last one is a nice-to-have enhancement, and is not necessary for v1.0.

flavorjones commented 1 year ago

There is some relevant information about the impact of using ruby_xmalloc on performance at https://github.com/sparklemotion/nokogiri/pull/2843

TL;DR the libxml2 parser was measured as being 1.12x - 1.34x slower when using ruby_xmalloc as compared to stdlib malloc. I'd be interested to see if there's a similar slowdown in YARP.

eregon commented 1 year ago

I think the best solution here is to only use arena allocations. And to allocate the arena itself then malloc/xmalloc can be used. That would provide the best performance and use xmalloc semantics without a significant overhead.

Since this seems performance-critical I think it should be in for 1.0. But also I don't know what is the vision/general requirements for 1.0. I think we need to review and cleanup all nodes for 1.0, as after that it will be expected the AST is fully stable.

kddnewton commented 1 year ago

Yeah @flavorjones I remember you showing me the nokogiri metrics. I think we should build it either way in order to be able to measure it on YARP for sure.

kddnewton commented 1 year ago

@HParker did you want to take a look at the first bullet here?

HParker commented 1 year ago

Sure! I might have already started looking into it. :)