Optimize getfilecontents() to reduce chance of race for client_state.xml

[cyrossignol]

The getfilecontents() function loads a file into memory, and the previous implementation read the file line-by-line into a buffer. We use this function to load BOINC's client_state.xml file. A user on Slack reported that the wallet took 7 seconds to read this file, and @jamescowens confirmed that it took 10 seconds with his version.

As also confirmed on Slack, BOINC aborts on Windows when trying to update client_state.xml if the wallet holds the file open during start-up. This change improves the performance of the function to greatly reduce the chance of a race between Gridcoin and BOINC.

[jamescowens]

Tested it out. Even more compact than my own solution. Reduced the client_state.xml load time on my windows testnet wallet to < 0.5 seconds from about 10 seconds.

[jamescowens]

I can do a special build of 4.0.2.0 release with just this change to give to KarVi71 and see if it fixes his problem. If so, we can hold off on doing more substantial plumbing work, such as implementing RPC communication between the BOINC client and the wallet.

[tomasbrod]

Can the file be opened in such way, that does not block boinc from writing to it?

[jamescowens]

The file is already being opened in a non-blocking way. The issue is that BOINC tries to rename the file. Windows does not allow a file to be renamed if another file handle already exists, even if in non-exclusive mode.

…

Sent from my iPhone

On Apr 4, 2019, at 8:32 AM, Tomáš ***@***.***> wrote: Can the file be opened in such way, that does not block boinc from writing to it? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[tomasbrod]

That is unfortunate and breaks the copy-and-replace pattern.

[jamescowens]

It looks like minimizing the amount of time we have the file open has practically solved the issue, but I agree it is an ugly issue underneath....

[cyrossignol]

Fortunately, BOINC does retry the write/rename operations on client_state.xml, so as long as we read the file quickly, the wallet probably won't ever kill BOINC anymore.

I studied this issue for a bit because I was completely blown away by the evidence which showed that it took such a huge amount of time to read a file that's only a few MB large. I wanted to understand why this was happening so that I won't make the same mistake later. At first, I thought that the way that the function opened and closed the file twice introduced some delay because of something in the OS, but removing that nonsense didn't change the result.

The culprit appears to be in the way that the old function appended each line to the buffer:

while (getline(myfile, line))
{
    //       vvvvvv--- copy
    buffer = buffer + line + "\n";
    //     ^--- swap
}

Namely, the function really didn't append anything to the existing buffer. Instead, it copied the whole buffer and replaced it for every line. For larger client_state.xml files, we were churning through over a GB of data just to read the file.

The issue disappeared when I changed that line of code to:

buffer += line + "\n";

...for which std::string overloads operator+=() to append the input to the internal buffer. If the function was written like this to begin with, we may never have seen the race condition with BOINC.

[jamescowens]

Yep. And the irony is that we didn’t need to do any of that at all!

…

Sent from my iPhone

On Apr 5, 2019, at 5:16 PM, Cy Rossignol ***@***.***> wrote: Fortunately, BOINC does retry the write/rename operations on client_state.xml, so as long as we read the file quickly, the wallet probably won't ever kill BOINC anymore. I studied this issue for a bit because I was completely blown away by the evidence which showed that it took such a huge amount of time to read a file that's only a few MB large. I wanted to understand why this was happening so that I wouldn't make the same mistake later. At first, I thought that the way that the function opened and closed the file twice introduced some delay because of something in the OS, but removing that nonsense didn't change the result. The culprit appears to be in the way that the old function appended each line to the buffer: while (getline(myfile, line)) { // vvvvvv--- copy buffer = buffer + line + "\n"; // ^--- swap } Namely, the function really didn't append anything to the existing buffer, but, instead, it copied the whole buffer and replaced it for every line, which--for larger client_state.xml files--means that we were churning through over a GB of data just to read the file. The issue disappeared when I changed that line of code to: buffer += line + "\n"; ...for which std::string overloads operator+=() to append the input to the internal buffer. If the function was written like this to begin with, we may never have seen the race condition with BOINC. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.