Skip to content
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

Exponential data-transfer can be changed to linear #229

Closed
rpriyadarshi opened this issue Mar 28, 2018 · 3 comments
Closed

Exponential data-transfer can be changed to linear #229

rpriyadarshi opened this issue Mar 28, 2018 · 3 comments

Comments

@rpriyadarshi
Copy link

Since we are accumulating data transferred in result string, the find becomes exponential as we always search the concatinated string from beginning.

Client file linuxtcpsocketclient.cpp:

void LinuxTcpSocketClient::SendRPCMessage(const std::string& message, std::string& result) throw (JsonRpcException) {
    ...
		{
			string tmp;
			tmp.append(buffer, nbytes);
			result.append(buffer,nbytes);
		}
	} while(result.find(DELIMITER_CHAR) == string::npos);
    ...
}

Server file linuxtcpsocketserver.cpp:

void* LinuxTcpSocketServer::GenerateResponse(void *p_data) {
...
		{
			request.append(buffer,nbytes);
		}
	} while(request.find(DELIMITER_CHAR) == string::npos);
...
}

Change the while loop to:
} while(memchr(buffer, DELIMITER_CHAR, nbytes) == NULL);
Delete the tmp string as well

			string tmp;
			tmp.append(buffer, nbytes);

With these changes, I did a round trip transfer of 10^7 characters in 0.8 seconds instead of 108 seconds.

@loreilei
Copy link

loreilei commented Mar 28, 2018

In which version did you spot this ?

This is indeed completely useless to have this

string tmp; tmp.append(buffer, nbytes);
It add useless copy.

I did not spot it in the master branch libjson-rpc-cpp/src/jsonrpccpp/client/connectors/linuxtcpsocketclient.cpp

but it is still present in the windows implementation ( libjson-rpc-cpp/src/jsonrpccpp/client/connectors/windowstcpsocketclient.cpp).

I see in the linux tcp socket connector and the unix domain socket connector that the stream reader is used, I guess it would be nice to also use it in the windows tcpsocket connector (unless it uses linux features, I haven't checked that).

PS: My bad it seems this useless copy was introduced by me since it is in tag v0.7.0 version of file...

@rpriyadarshi
Copy link
Author

There has been quite a bit of refactoring since I got the code last year. Looks like the exponential data issue is still there. The target will be a cumulative string that would be searched from the beginning each time: (common/streamreader.cpp)

bool StreamReader::Read(std::string &target, int fd, char delimiter) {
  ssize_t bytesRead;
  do {
    bytesRead = read(fd, this->buffer, buffersize);
    if (bytesRead < 0) {
      return false;
    } else {
      target.append(buffer, static_cast<size_t>(bytesRead));
    }
  } while (target.find(delimiter) == string::npos && bytesRead > 0);

  target.pop_back();
  return true;
}

@loreilei
Copy link

So a fix could be add a counter of how many bytes we read and use it as a starting position for the find call.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants