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.

Sign up for GitHub

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

Jump to bottom

Measure performance of TSPClient before and after JSON-RPC #999

Merged
merged 1 commit into from
Sep 5, 2023
Merged

Measure performance of TSPClient before and after JSON-RPC #999

merged 1 commit into from
Sep 5, 2023

Conversation

hoangphamEclipse
Copy link
Contributor

@hoangphamEclipse hoangphamEclipse commented Jul 19, 2023 •
edited
Loading

This commit adds a study on the impact of the JSON-RPC patch on the Theia Front End - Back End - Trace Server communication speed.

Relates to #990.

@marco-miller marco-miller self-requested a review July 26, 2023 18:48
marco-miller
marco-miller previously requested changes Aug 3, 2023
View reviewed changes
Copy link
Contributor

@marco-miller marco-miller left a comment

Choose a reason for hiding this comment

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

Thanks @hoangphamEclipse for this effort; well appreciated. I'd have these comments, some minor edit ones and the others more "fundamental". There could be more work ahead given these potentially opening aspects.

doc/performance/0002-http-vs-rcp-client-performance-analysis.md Outdated Show resolved Hide resolved
doc/performance/0002-http-vs-rcp-client-performance-analysis.md Outdated Show resolved Hide resolved
doc/performance/0002-http-vs-rcp-client-performance-analysis.md Outdated Show resolved Hide resolved
doc/performance/0002-http-vs-rcp-client-performance-analysis.md Outdated Show resolved Hide resolved
doc/performance/0002-http-vs-rcp-client-performance-analysis.md Outdated Show resolved Hide resolved
doc/performance/0002-http-vs-rcp-client-performance-analysis.md Outdated Show resolved Hide resolved
doc/performance/0002-http-vs-rcp-client-performance-analysis.md Outdated Show resolved Hide resolved
doc/performance/0002-http-vs-rcp-client-performance-analysis.md Outdated Show resolved Hide resolved
doc/performance/0002-http-vs-rcp-client-performance-analysis.md Outdated Show resolved Hide resolved
doc/performance/0002-http-vs-rcp-client-performance-analysis.md Outdated
3. For fetchTimeGraphStates(), the JSON-RCP update takes more time compared to the HTTP direct implementation.

One of the possible explanation is the difference in the amount of data that needs to be sent to the back end. This is true for the case of the `fetchXYTree()` and the `fetchTimeGraphStates()` tests. Since they both serialize a good amount of data, the performance either stays the same - in the case of `fetchXYTree()` where we have small amount of data - or becomes worse - in the case of `fetchTimeGraphStates()` where we have a decent amount of data.

Copy link
Contributor

Choose a reason for hiding this comment

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

A missing test case might be, to measure the impact on a trailing yet tiny payload request, preceded by a heavy one. Meaning, is the trailing (tiny) request starved in any way by the preceding elephant. We could compare the outcome of this test before versus after json-rpc. This was recently proposed by @MatthewKhouzam IIRC.

So I think he mentioned the tiny request being a trace annotations/markers one or so. -Preceded by some truckload data fetch act. The latter shouldn't starve or prevent the former from showing, IIUC. If this is an issue before changing to json-rpc, then measuring with the latter in turn might become irrelevant. The question could otherwise be, is the json-rpc wrapper (towards the back-ended client use) adding latency to the trailing/tiny request?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MatthewKhouzam since you are back, do you think this is a required test case?

@hoangphamEclipse hoangphamEclipse marked this pull request as draft August 7, 2023 18:21
@hoangphamEclipse hoangphamEclipse force-pushed the json-rcp branch 7 times, most recently from c7e12ce to b0e92f8 Compare August 23, 2023 15:34
@hoangphamEclipse hoangphamEclipse changed the title Measure performance of TSPClient before and after JSON-RCP. Measure performance of TSPClient before and after JSON-RPC Aug 23, 2023
@hoangphamEclipse hoangphamEclipse marked this pull request as ready for review August 23, 2023 15:34
marco-miller
marco-miller reviewed Aug 23, 2023
View reviewed changes
Copy link
Contributor

@marco-miller marco-miller left a comment

Choose a reason for hiding this comment

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

Leaving this review up to @MatthewKhouzam and others if need be, given my recent focus change; thanks.

bhufmann
bhufmann requested changes Aug 29, 2023
View reviewed changes
Copy link
Collaborator

@bhufmann bhufmann left a comment

Choose a reason for hiding this comment

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

Thanks for the report of your investigation. It looks good overall from my side, only some small update requests.

@bhufmann bhufmann requested review from bhufmann and marco-miller and removed request for marco-miller August 30, 2023 18:10
@hoangphamEclipse
Measure performance of TSPClient before and after JSON-RPC.
27b1429 
This commit adds a study on the impact of the JSON-RPC patch on the
Theia Front End - Back End - Trace Server communication speed.

Relates to eclipse-cdt-cloud#990.

Signed-off-by: Hoang Thuan Pham <hoang.pham@calian.ca>
bhufmann
bhufmann approved these changes Sep 5, 2023
View reviewed changes
Copy link
Collaborator

@bhufmann bhufmann 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 to me. Thanks!

@MatthewKhouzam could you please do another round of review since you gave comments initially?

@bhufmann bhufmann dismissed marco-miller’s stale review September 5, 2023 15:23

Marco changed work focus

MatthewKhouzam
MatthewKhouzam approved these changes Sep 5, 2023
View reviewed changes
Copy link
Contributor

@MatthewKhouzam MatthewKhouzam left a comment

Choose a reason for hiding this comment

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

Good!

@MatthewKhouzam MatthewKhouzam merged commit a50cb95 into eclipse-cdt-cloud:master Sep 5, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marco-miller marco-miller marco-miller left review comments

@MatthewKhouzam MatthewKhouzam MatthewKhouzam approved these changes

@bhufmann bhufmann bhufmann approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hoangphamEclipse @bhufmann @marco-miller @MatthewKhouzam