GRIP icon indicating copy to clipboard operation
GRIP copied to clipboard

Generated Python code is inefficient

Open SamCarlberg opened this issue 8 years ago • 3 comments

Hopefully someone better at Python than me can open a PR to clean it up

(Originally reported by @virtuald, hopefully he can help with this)

SamCarlberg avatar Dec 08 '16 05:12 SamCarlberg

Can you provide examples??

JLLeitschuh avatar Dec 08 '16 14:12 JLLeitschuh

My complaints apply mostly to a low-performance platform such as the RoboRIO -- on a higher performance platform you're less likely to run into problems, but it's very noticeable on a low performance platform. Let's consider just a single example of generated code (https://github.com/WPIRoboticsProjects/GRIP/blob/master/ui/src/main/resources/edu/wpi/grip/ui/codegeneration/python/operations/CV_cvtColor.vm):

OpenCV docs -- 2.4, but it gets the point across

You'll note that cvtColor takes (src, code, [dst, [dstCn]]) -> dst. You only provide the src and code parameters, which means that every time the function is called then a brand new (large) memory allocation is made so that it can return a dst image. When you have a lot of steps in your pipeline, this becomes a big problem because you're allocating all of these large chunks of memory for no apparent reason (this one has two large memory allocations).

Instead (when possible), you should try to utilize the optional dst parameter, which tells the function to copy the results to that array instead of allocating a new array. You usually cannot just give it the src array (though, there may be exceptions), but instead you should at the beginning of the program (or pipeline) figure out what arrays you'll need, allocate them up front, and then reuse them each time. This helps a lot in all languages, but particularly because python has higher overhead than Java or C++ every optimization you can do helps. It can be the difference between 50% CPU and 5% CPU.

An example of this strategy can be found in my team's 2016 code.

I suspect this advice can be applied to other areas of GRIP as well (I've heard that it's slow on a RoboRIO, this may be why).

virtuald avatar Dec 08 '16 15:12 virtuald

Wait... Yea, this should really be changed so that it uses the dst param. We do that in GRIP all the time because we were running out of memory as well. I didn't even think to check for this while doing my code review on the code gen stuff. @SamCarlberg Is it possible that the code gen stuff could be fixed to do this?

JLLeitschuh avatar Jan 16 '17 23:01 JLLeitschuh