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

RPC: Decouple message creation from message encoding/decoding #6027

Merged

Conversation

nb-ohad
Copy link
Contributor

@nb-ohad nb-ohad commented May 19, 2020

Explain the changes

  1. Create a new RpcMessage class to represent a generic non-encoded RPC message that can describe any op (in contrast to RpcReqeust that can be used solely for requests and responses).
  2. Move encoding/decoding responsibilities (from and to transit representation) into rpc_connection where the base class just delegate to the old encode/decode code inside RpcRequest.
  3. Decode on "message" and emit a "decoded_message" event with the decoded RpcMessage.
  4. Override the encoding and decoding for fcall connections to passthrough the RpcMessage without encoding or decoding.

Issues: Fixed #xxx / Gap #xxx

Testing Instructions:

@nb-ohad nb-ohad force-pushed the ohad-move-encode-decode-to-rpc-connetion branch from 5994f92 to bce027e Compare May 19, 2020 21:51
@nb-ohad nb-ohad requested a review from guymguym May 19, 2020 21:51
@nb-ohad nb-ohad force-pushed the ohad-move-encode-decode-to-rpc-connetion branch 2 times, most recently from 2207382 to f318061 Compare May 21, 2020 12:13
src/rpc/rpc_message.js Outdated Show resolved Hide resolved
src/rpc/rpc_message.js Show resolved Hide resolved
src/rpc/rpc_message.js Show resolved Hide resolved
src/rpc/rpc_request.js Outdated Show resolved Hide resolved
src/rpc/rpc.js Outdated Show resolved Hide resolved
src/rpc/rpc_message.js Show resolved Hide resolved
src/rpc/rpc_request.js Outdated Show resolved Hide resolved
src/rpc/rpc_base_conn.js Outdated Show resolved Hide resolved
src/rpc/rpc_base_conn.js Outdated Show resolved Hide resolved
src/rpc/rpc_base_conn.js Outdated Show resolved Hide resolved
@nb-ohad nb-ohad force-pushed the ohad-move-encode-decode-to-rpc-connetion branch 2 times, most recently from efe7f9f to 0b5eaa3 Compare June 10, 2020 17:11
Copy link
Member

@guymguym guymguym left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, did you check the CI failures?

@guymguym
Copy link
Member

@nimrod-becker I'm not sure what happened to TravisCI but I cant find how to restart thee build.
@nb-ohad perhaps you can simply rebase and push to rebuild?

@nb-ohad
Copy link
Contributor Author

nb-ohad commented Jun 11, 2020

@guymguym I restarted the build

@guymguym
Copy link
Member

@nb-ohad It failed. Not clear if related so I restarted the tests now.

@nb-ohad
Copy link
Contributor Author

nb-ohad commented Jul 2, 2020

It seems that not serializing the payload (body) into a buffer (and then deserializing) creates a difference with regard to encoding.

I am working on trying to pinpoint the problem.
I will update here

@guymguym
Copy link
Member

any news on that?

@guymguym guymguym added Performance Type:Enhancement New suggestions for behaviours labels Aug 17, 2020
@guymguym guymguym changed the title Decouple RPC message creation from message encoding/decoding RPC: Decouple message creation from message encoding/decoding Aug 17, 2020
@nb-ohad nb-ohad force-pushed the ohad-move-encode-decode-to-rpc-connetion branch from 0b5eaa3 to c51c44e Compare August 20, 2020 10:38
@nb-ohad nb-ohad requested a review from guymguym August 20, 2020 10:51
@liranmauda liranmauda changed the title RPC: Decouple message creation from message encoding/decoding RPC: Decouple message creation from message encoding/decoding dbg Aug 20, 2020
@nb-ohad nb-ohad force-pushed the ohad-move-encode-decode-to-rpc-connetion branch from c51c44e to b840d29 Compare August 20, 2020 13:37
@nb-ohad nb-ohad changed the title RPC: Decouple message creation from message encoding/decoding dbg RPC: Decouple message creation from message encoding/decoding Aug 21, 2020
@nb-ohad
Copy link
Contributor Author

nb-ohad commented Sep 1, 2020

@guymguym ping on review for the second commit

Copy link
Member

@guymguym guymguym left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nb-ohad One last comment about making the omit conditional to avoid copying the params/reply objects, the copy is shallow but still this is very much unneeded.
Other than that it looks good to me.

src/util/js_utils.js Outdated Show resolved Hide resolved
src/rpc/rpc.js Show resolved Hide resolved
src/rpc/rpc_base_conn.js Show resolved Hide resolved
src/rpc/rpc_base_conn.js Show resolved Hide resolved
@nb-ohad nb-ohad force-pushed the ohad-move-encode-decode-to-rpc-connetion branch from b840d29 to bbac13a Compare September 14, 2020 15:38
Copy link
Member

@guymguym guymguym left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nb-ohad To summarize this PR - it tries to improve how rpc_fcall (which is an optimized rpc path for calls which are executed in the same process) by getting rid of the intermediate large memory allocations created by JSON.parse(JSON.stringify(..)) and instead use cloneDeep. It does introduce some subtleties of cloning in rpc_fcall, but the rest of the system is not affected.

However, how do you measure the effect of this change? Did it help? is it possible that it is somehow slower/heavier than previous solution? Of course - I don't really think so. But we need to work with some numbers here.

I would suggest to enhance rpc_benckmark.js for that. You can add a new --fcall flag that will do the benchmark inside one process (both client and server). And then our main metric is operations/second and the higher the better, or something more complicated is needed to see the effect? We can run it with node --trace-gc for GC info. And we can add anything else we need to that benchmark to simulate the effect on a real process...

@nb-ohad
Copy link
Contributor Author

nb-ohad commented Sep 15, 2020

@guymguym This optimization is not about getting rid of JSON.parse/JSON.stringify because deepClone will also create a new object tree. It is about removing the need in the allocation of buffers for interprocess calls (header and body buffers)

regarding measurements, This is a thing that is very hard to measure and the normal rpc_banchmark will not yield any meaningful results. We initiated this task as a result of the measurement that we did with a high volume, small object tests
The results indicated that a lot of memory pressure was produced in this kind of scenario coming from 3-4 prime locations in the code, one of them was the allocation of buffers and cloning the message body object Tree.

We decided to tackle the points according to their impact and ease of fixing. So we choose to improve memory allocations in the instantiation of our API clients (#6022) and minimizing allocations for RPC calls via 'fcall' connections

@nb-ohad nb-ohad force-pushed the ohad-move-encode-decode-to-rpc-connetion branch 2 times, most recently from eb2ed8c to 64a5f89 Compare September 17, 2020 08:28
@nb-ohad
Copy link
Contributor Author

nb-ohad commented Sep 17, 2020

@guymguym as of your request for enhancements to rpc_benckmark:
#6173

@nb-ohad nb-ohad force-pushed the ohad-move-encode-decode-to-rpc-connetion branch from 64a5f89 to 31bba1d Compare September 17, 2020 13:47
allowing rpc_fcall to override defualt encoding/decoding decoding

Ensure cloning of rpc messages procude a clone that is  more like a message decoded form buffers by:
1. Removing rpc_buffers from message body (and n2n_proxy messages)
2. serializing object ids into strings
3. ensuring empty buffers array on every message

Signed-off-by: nb-ohad <[email protected]>
@nb-ohad nb-ohad force-pushed the ohad-move-encode-decode-to-rpc-connetion branch from 31bba1d to a9c0f44 Compare September 22, 2020 11:14
@nb-ohad nb-ohad merged commit ed1c45e into noobaa:master Sep 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Performance Type:Enhancement New suggestions for behaviours
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants