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
StructCPID optimizations #157
Comments
I think I have tracked it down to this: https://github.com/gridcoin/Gridcoin-Research/blob/master/src/main.cpp#L6304 Also, what is that function intended to do with the incoming map? It adds a new object to the map but since it's only a copy that insert isn't propagated through the rest of the program. Is the intention that a new object should be inserted into the caller's map, not the copy? I will submit a PR for this as a fix for #140, not for this one as the fix that covers this issue is much, much larger in terms of SLOC and files. |
Hmmm, this looks pretty major and ingenious, thanks. So before I try to answer, what kind of performance gain did you experience by changing the struct initializer to directly pull the data from the map without copying it? Rob |
Well, in comparison by copying the entire map it's minor. The big benefit is the change in #158 whereas the rest is just a bonus. By passing all StructCPID objects by reference instead of value you gain a lot of small increases but not really more than "it's common practice, there will be less copying" (at least the way they're called now). By passing the maps by reference instead of value as in #158 you drop the CPU usage by a massive amount. |
So the reason I added the function initially was about a year ago, we were experiencing some illegal memory overwrite exceptions inside TallyNetworkAverages, and it was right around the line after we initialize the structcpid, then we start setting values, and someone running valgrind on linux picked up situations where the code wrote into protected memory and core dumped. So I made that function that sets the basic remnant of the object before using it - but to this day never realized it was copying every time it accessed it because it was not being passed by reference, so this is indeed a great find, great work, thanks! I want to pull 158 in and test it... Im trying to get to it, the issue is I have an issue Im working on right now, but shouldnt be much longer. I hope this fixes our performance issue people are complaining about. I was always confused by it because on windows, my box runs at 1% utilization, but hey it could be a thread-lock or contention issue on other platforms or something. |
Oh btw Den, can you do me a favor? Can you leave your code running for a couple days and verify the wallet does not crash on linux with the new code? Ill test Windows over the next few days so we can have a safe main branch commit. |
I'll let it run also (with the new merge) on a linux64, no crash since 1h |
I have it running on two boxes since yesterday. Which compiler is used on Windows? It could be that MSVC is smarter and can see that the map is a local copy and simply optimize it out. |
so, after ca. 7h:
|
The RAM performance on Windows Server 2008 R2 for your info is steady between 425Mb - 450Mb of RAM. |
The rest of the optimisations, including the one in the first post, are negligible so I'm closing this one. |
I did a quick run with profiling enabled to see where all the CPU cycles go and discovered this interesting bit:
A majority of the time spent goes to constructing and destructing StructCPID objects. I took a peek at how these are used and saw a lot of these usages:
Disregarding the extra copies of std::string objects for now, this will:
I think all this can be replaced with:
const StructCPID& structMag = mvMagnitudes[(*ii).first];
No extra copies, no destructors.
I'll give it a go in my wallet and see what happens.
The text was updated successfully, but these errors were encountered: