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

GUACAMOLE-2036: Implement the GuacamoleParser within the ReaderGuacamoleReader. #1057

Merged
merged 1 commit into from
Feb 25, 2025

Conversation

aleitner
Copy link
Contributor

@aleitner aleitner commented Feb 7, 2025

Implement the GuacamoleParser within the ReaderGuacamoleReader to support multi byte characters when reading instructions.

@aleitner aleitner force-pushed the GUACAMOLE-615-reader branch from b263fa7 to 41c7018 Compare February 10, 2025 19:37
@aleitner aleitner marked this pull request as ready for review February 10, 2025 19:41
@aleitner
Copy link
Contributor Author

We need to benchmark this to make sure there's no degradation of performance

@mike-jumper
Copy link
Contributor

mike-jumper commented Feb 24, 2025

Well, I've thrown together a quick benchmark, and the new version leveraging GuacamoleParser actually seems significantly faster:

[mjumper@mdesktop ~]$ java -cp ./guacamole-common-old-reader.jar:. ReaderBenchmark desktop-vnc.guac 
desktop-vnc.guac ... 18.79 ms / run
[mjumper@mdesktop ~]$ java -cp ./guacamole-common-new-reader.jar:. ReaderBenchmark desktop-vnc.guac 
desktop-vnc.guac ... 8.54 ms / run
[mjumper@mdesktop ~]$

This is just with calls to readInstruction(), which would be expected to have slightly more overhead than read(). The bad news is that things fail when using read() instead:

[mjumper@mdesktop ~]$ java -cp ./guacamole-common-old-reader.jar:. ReaderBenchmark --buffers desktop-vnc.guac 
desktop-vnc.guac ... 13.9 ms / run
[mjumper@mdesktop ~]$ java -cp ./guacamole-common-new-reader.jar:. ReaderBenchmark --buffers desktop-vnc.guac 
desktop-vnc.guac ... FAIL
org.apache.guacamole.GuacamoleException: No instruction available to read.
        at org.apache.guacamole.io.ReaderGuacamoleReader.read(ReaderGuacamoleReader.java:93)
        at ReaderBenchmark.readFile(ReaderBenchmark.java:25)
        at ReaderBenchmark.main(ReaderBenchmark.java:57)
[mjumper@mdesktop ~]$ 

The test file that I'm using is a recording of a VNC session that's about 19M in size. The benchmark just times how long it takes to fully read through the recording, averaged over 100 trials.

Here's the hacky, quick-and-dirty benchmark: https://gist.github.com/mike-jumper/d0939f0c1a4688e9217062788a3ebfba

Copy link
Contributor

@mike-jumper mike-jumper left a comment

Choose a reason for hiding this comment

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

A few things:

  • Good news: it's faster. Bad news: read() seems to fail (see the comment regarding benchmark results)
  • This should be against patch.
  • The GUACAMOLE-615 issue was part of a past release (1.5.2), so it's now strictly historical. Any new changes will need to be associated with a new JIRA issue.

@aleitner aleitner changed the base branch from main to patch February 25, 2025 01:18
@aleitner aleitner force-pushed the GUACAMOLE-615-reader branch from 41c7018 to 1997435 Compare February 25, 2025 01:24
@aleitner aleitner changed the title GUACAMOLE-615: Implement the GuacamoleParser within the ReaderGuacamoleReader. GUACAMOLE-2036: Implement the GuacamoleParser within the ReaderGuacamoleReader. Feb 25, 2025
…oleReader to support multibyte characters when reading instructions.
@aleitner aleitner force-pushed the GUACAMOLE-615-reader branch from 1997435 to c9f2e45 Compare February 25, 2025 01:45
@aleitner
Copy link
Contributor Author

A few things:

* **Good news:** it's faster. **Bad news:** `read()` seems to fail (see [the comment regarding benchmark results](https://github.com/apache/guacamole-client/pull/1057#issuecomment-2679632871))

* This should be against `patch`.

* The [GUACAMOLE-615](https://issues.apache.org/jira/browse/GUACAMOLE-615) issue was part of a past release (1.5.2), so it's now strictly historical. Any new changes will need to be associated with a new JIRA issue.

Big relief that it's faster! I just added tests for reader.read and rebased to patch

Copy link
Contributor

@mike-jumper mike-jumper left a comment

Choose a reason for hiding this comment

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

These changes do fix the issue with read(), though read() is now taking a bit of a performance hit:

ReaderGuacamoleReader throughput for the old, new, and proposed implementations.

I think the performance hit is due to the need to regenerate the protocol representation of the instruction from scratch. The above graph shows:

  • Old: The ReaderGuacamoleReader from before this PR.
  • New: The ReaderGuacamoleReader from this PR.
  • Revised: The ReaderGuacamoleReader from this PR, with additional modifications to avoid regenerating what we've parsed from scratch (a672229).

I'm good with the changes as they stand and think it makes sense to consider further revisions like a672229 in a separate PR.

Unfortunately, it seems impossible to maintain the significant performance improvement of the readInstruction() implementation here without sacrificing the ability to efficiently pass received instructions on to the client ... but we can get close!

@mike-jumper mike-jumper merged commit b08e0be into apache:patch Feb 25, 2025
1 check passed
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

Successfully merging this pull request may close these issues.

2 participants