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

Using pool to avoid stack and frames memory allocation #413

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AprilFiv
Copy link

@AprilFiv AprilFiv commented Feb 19, 2023

@geseq
Copy link
Collaborator

geseq commented Mar 18, 2023

I see this being useful for extremely short running scripts (anything that's small enough to run less than double digit microseconds), but the larger a script the less the impact of this as the GC is likely to clean up the pool. Also for runs that take over double digit microseconds the allocation cost should be minimal compared to the runtime itself.

perhaps try your benchmarks with a longer running example like:

fib := func(x) {
	if x == 0 {
		return 0
	} else if x == 1 {
		return 1
	}

	return fib(x-1) + fib(x-2)
}
out := fib(20)

Given that, I'm not entirely convinced that this adds much real value in real world usage.

The code does look safe, but I'm leery against reusing the stack from past runs without any cleanup in future runs and the possibility of that being exploitable. The tradeoff might be worth it if the performance improvement is significant in real world usage, but as it is, I'm not convinced.

The other possibility might be that it's the pointer usage that's giving part of the performance gains. If possible also benchmark this in a long running case to see if there's any difference. If that is partly faster without the usage of pools, it would be a safer bet without having to reuse the same objects.

@d5 thoughts?

@d5
Copy link
Owner

d5 commented Mar 18, 2023

What happens if one of the runs is aborted by Abort() function? Compiled.RunContext may call VM.Abort if the context is canceled.

@geseq
Copy link
Collaborator

geseq commented Mar 18, 2023

Any new run would still start with stack pointer 0 so it should be fine, but the stack itself wouldn’t start out empty. I don’t see any obvious way the items in the stack would be accessible but it’s not ideal.

@AprilFiv
Copy link
Author

Any new run would still start with stack pointer 0 so it should be fine, but the stack itself wouldn’t start out empty. I don’t see any obvious way the items in the stack would be accessible but it’s not ideal.

yeah... considered it already

@AprilFiv
Copy link
Author

AprilFiv commented May 11, 2023

Any new run would still start with stack pointer 0 so it should be fine, but the stack itself wouldn’t start out empty. I don’t see any obvious way the items in the stack would be accessible but it’s not ideal.

By adding such lines, we could clear the stack and frames array literally before recycling, and only cost few extra cpu time, though it makes no sense for VM.Run()

		*v.stack = [len(v.stack)]tengo.Object{}
		*v.frames = [len(v.frames)]frame{}

@geseq
Copy link
Collaborator

geseq commented May 15, 2023

But does that still provide any gains of the pool?

Do note that the go pool is not like a typical object pool and does get its objects garbage collected if not used frequent enough.

@AprilFiv
Copy link
Author

AprilFiv commented May 28, 2023

All the pool's target is to reduce the overhead of memory allocation.
These two lines would not make extra memory allocation if you doubt about that.

*v.stack = [len(v.stack)]tengo.Object{}
*v.frames = [len(v.frames)]frame{}

@geseq
Copy link
Collaborator

geseq commented May 28, 2023

Perhaps I'm not explaining this correctly. My concern is more that if you add up the fact that the pool objects can be GC'd non-deterministically, additional cycles takes for clearing the stack frames, indirect access to stack and frames during the VM run, I'm not sure the gains in memory allocation cost offset enough to make a difference.

Can you please add a benchmark that demonstrates the actual gains using the pool here provides?

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.

3 participants