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

Builders need to have a builder in them causing many copies #84

Open
noambergIL opened this issue Sep 5, 2018 · 3 comments
Open

Builders need to have a builder in them causing many copies #84

noambergIL opened this issue Sep 5, 2018 · 3 comments
Assignees

Comments

@noambergIL
Copy link
Contributor

Code from Public-API
func prepareResponse(transactionOutput *services.AddNewTransactionOutput) *services.SendTransactionOutput {
var receiptForClient *protocol.TransactionReceiptBuilder = nil

if receipt := transactionOutput.TransactionReceipt; receipt != nil {
	receiptForClient = &protocol.TransactionReceiptBuilder{
		Txhash:              receipt.Txhash(),
		ExecutionResult:     receipt.ExecutionResult(),
		OutputArgumentArray: receipt.OutputArgumentArray(),
	}
}

response := &client.SendTransactionResponseBuilder{
	TransactionReceipt: receiptForClient,
	TransactionStatus:  transactionOutput.TransactionStatus,
	BlockHeight:        transactionOutput.BlockHeight,
	BlockTimestamp:     transactionOutput.BlockTimestamp,
}

return &services.SendTransactionOutput{ClientResponse: response.Build()}

}
i need to open and close the transaction receipt as the builder of SendTransaction won't let me use the receipt and forces a creation and copy of another builder.

opened during code review by shai ...
@erankirsh @electricmonk

@talkol
Copy link
Member

talkol commented Sep 7, 2018

This is on purpose

Sending the response to the client requires re-encoding of the data in the client SDK format. Accidentally, we use here the same format as our internal one, but we will probably support other formats as well.

We are aware of the performance hit of the copy but decided it's ok. It's a very small copy (not entire blocks, just a single receipt). If our format was accidentally JSON, paying this encoding and copy cost would have been very natural.

From API perspective, this copy is cumbersome on Membuffers on purpose, so every time you have a copy you'll feel it's a smell (which it is) and open an issue exactly like this one so we can examine if the copy is necessary or not :)

@talkol
Copy link
Member

talkol commented Sep 7, 2018

btw - if we decided this copy is too much to handle and we would have wanted to eliminate it at all cost, this is possible but complicates the client response format a little

this is what we do in gossip (where the copies are expensive since it's entire blocks)

The solution in this case would be to provide the publicAPI the response in multiple chunks (a separate membuffer for the receipt for example). And then the publicAPI would write it on socket to the client in multiple write operations.

This would be truly zero copies (like the rest of the system), but I think it's overkill for now from a complexity perspective.

@electricmonk
Copy link
Contributor

My problem here is that there are too many wrapper types. Why do we even need ClientResponse?

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

3 participants