Hi Martin,
sorry for the late response. Some thoughts:
1) I think the current API is not perfect. But aside from the implicit caching idea there is some places where caching happens explicitly. In those cases it would still be necessary to have an API which uses a given frame and does the setup without allocation. Those cases also fall into different cases, in some it is known that the same function will be called (f_map, f_filter, etc) and those where it could be a different function (tail recursion, etc). In the latter the current frame can only be reused when calling into pike code, which adds another complication. 2) I think the pike_frame struct should be smaller. My patch currently also added another pointer, which is not really needed. I did this mostly for clarity and was planning to clean it up later. It might also help to seperate the different frame types into several structs and place them into a union. This would also take care of places in the code where uninitialized values are used from the frames, mainly in the backtrace handling code. The different pointers to the stack could also be implemented using an offset from the save_sp.
Maybe a good idea would be to first add the caching using a new API. Also, I think interpreter struct might be a good place to do the caching. This avoids increasing the size of frames and also make the amount of cached frames a bit more predictable.
I would also be glad to have some feedback from people who are more familiar with the interpreter.
Arne
On 05/02/16 22:44, Martin Karlgren wrote:
Hi,
Great work!
A thought on the API – could frame_init perhaps be removed from the “public” API and have frame_setup_from_* perform that internally instead (and return the new frame)? That way a frame cache could be introduced transparently later on, by having frame_setup_from_* perform cache lookups based on the object/function offset etc. instead of creating a new frame.
Yes, caching multiple frame entries sounds desirable. If the cache is placed/referenced by the parent frame, even a multi-level cache should be possible:
void fie(int i) { } void bar(int i) { fie(i + 1); } void foo() { foreach(enumerate(1000000), int i) bar(i); }
Here, the F_FOREACH handler could set a PIKE_FRAME_DO_CACHE flag in the current frame, which would enable caching of the “bar” call. But the flag could also be propagated to the child frame, so that the “fie” call is cached in the frame handling the execution of “bar”. When execution leaves the scope that initiated the caching, the caches should be cleaned up recursively. I’m not sure whether this would work from a practical standpoint, but it might be worth considering. Thoughts?
/Marty
On 12 Apr 2016, at 18:48 , Arne Goedeke el@laramies.com wrote:
- Use the new API in more places like filter and automap.
- I also really like your idea of caching the last frame. I was
thinking about something similar, except that I did not think of using the frame for that, but instead cache it globally. Also, in many situations it is actually not enough to cache only one frame, instead we need 2 or more:
foreach (foo; mixed key; mixed val) bar();
During one iteration, this calls both next() in the iterator of foo and bar(). Its probably a good idea to experiment with that.