Do a single allocation per Simulation for INST Flooding

[SebastianMestre]

This is a straightforward patch that fixes #645.

I do have two questions though:

1. Is the variable name appropiate?
2. Should the allocation be done in the constructor?

[jacob1]

imo the malloc wasn't really a big deal. But if we are changing it, perhaps it might be a good idea to rewrite FloodINST to use CoordStack.h ? Haven't checked the feasibility of that, but it's used in most other flood fill functions.

I like the allocation where it is, no need to allocate memory on startup that may not be used.

Also could change malloc / free to new[] / delete[].

[SebastianMestre]

imo the malloc wasn't really a big deal. But if we are changing it, perhaps it might be a good idea to rewrite FloodINST to use CoordStack.h ? Haven't checked the feasibility of that, but it's used in most other flood fill functions.

I like the allocation where it is, no need to allocate memory on startup that may not be used.

Also could change malloc / free to new[] / delete[].

Oh, I didn't know about CoordStack. I'll use that instead.

About new and delete, since I'm going to be using CoordStack, should I change it?

[jacob1]

I think even CoordStack still needs an allocated buffer, if it still does then I do recommend changing it to new / delete.

[jacob1]

Oh, is the allocation not even needed? In that case, don't put CoordStack onto Simulation.h (or anything). Since there is no allocation, we don't need to save it anymore.

https://github.com/The-Powder-Toy/The-Powder-Toy/blob/master/src/simulation/Simulation.cpp#L599 <- example of CoordStack on the stack.

Pretty much all other uses of CoordStack have that bitmap be allocated. I thought that would be needed here too. If it is, then put just the bitmap onto Simulation.h, otherwise, put nothing there, everything can be local to the FloodINST function. I think it's not needed because INST doesn't need to keep track of which locations it's visited before. It can tell because it will be SPRK instead of INST now.

[SebastianMestre]

I'll do that, but I'll point out that CoordStack allocates in its constructor. If we want to allocate only once, we need to instanciate it only once, and in order to do that, it must be saved.

[jacob1]

hmm, it does ...

I guess I will just go back to what I originally said with, "that isn't a problem"

Sorry for the conflicting instructions >_>, I should have closed that issue because it isn't an issue.

I do like CoordStack better, which is why I asked you to switch to it :).

If you want, you could leave the PR as is and I can discuss it with the other devs. Not sure what they think about the allocation on every INST spark.

[SebastianMestre]

Oh, I see, it was my mistake then haha

Not a problem! I'll just put the CoordStack on the stack and let it allocate, then.

[jacob1]

perfect :). Going to merge this now.

I gave some bad advice earlier because I thought CoordStack kept the list of locations it still needed to visit on the stack, not in allocated memory. I'm still not worried about the allocations, will maybe talk to moony (person who opened the #645) later. Probably there's something we can do without having to keep a bunch of CoordStack objects around in .h files everywhere.

[SebastianMestre]

You could put a single CoordStack in the Simulation class, and have all the different methods use the same one, clearing it before or after they use it.

That way, there would be a single allocation, and it wouldn't make a mess out of the code.

[jacob1]

Maybe. But if we ever add threading that would be a mess. And CoordStack is used in at least one other place besides Simulation.cpp.

[SebastianMestre]

Ah, yes, fair point. I didn't think of that.

[moonheart08]

tl;dr allocations in simcode bad.
CoordStack is large enough that when allocated, it will fall back to the mmap system call, which has high latency, and can result in TPT being rescheduled early, reducing the amount of time we have to run simcode.
My proposal: Bite the bullet and preallocate a CoordStack instance for use in Simulation, or preallocate a thread_local coordstack.