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
8313697: [XWayland][Screencast] consequent getPixelColor calls are slow #15250
Conversation
… from the one used by the current session.
👋 Welcome back azvegint! A progress list of the required criteria for merging this PR into |
fp_pw_stream_disconnect(screenProps->data->stream); | ||
fp_pw_thread_loop_lock(pw.loop); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving pw_stream_disconnect
call out of this lock fixes JDK-8310334
Webrevs
|
@@ -28,6 +28,7 @@ | |||
import sun.awt.UNIXToolkit; | |||
import sun.security.action.GetPropertyAction; | |||
|
|||
import javax.swing.Timer; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason you can't use java.util.Timer ?
Ideally [*] AWT classes should not depend on Swing classes since already Swing classes depend on AWT classes ..
[*] yes, I know we already have such dependencies but only for very good reasons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course we can use java.util.Timer. Swing timer is just more convenient to use. Updated.
timerTask = new TimerTask() { | ||
@Override | ||
public void run() { | ||
closeSession(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happen if the "closeSession" is executed why the new Robot#getRGBPixels is called? In the robot code we call timerCloseSessionRestart to restart/cancel timer, but I think it could be possible that closeSession() is already executed and may close session right before it will be used by the robot but after we "open" it.
Or this case is not possible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both ScreencastHelper#getRGBPixels
(Robot#getRGBPixels
calls it internally) and ScreencastHelper#closeSession
are synchronized.
So ScreencastHelper#closeSession
will wait until the execution of ScreencastHelper#getRGBPixels
is finished and vice versa.
Many hours of testing with random delays close to DELAY_BEFORE_SESSION_CLOSE worked fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Java part looks good to me. Why 2s and not more or less ?
The delay could be set through a system property ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why 2s and not more or less ?
This is the average value chosen so as not to slow down tests execution, and not to annoy the user with the "Screen is being shared" icon while the session is still open.
The delay could be set through a system property ?
This functionality could be added, but I don't see much reason to do so.
If a screenshot is taken from time to time, the benefit of holding the session for a long time will be quite small compared to frequent requests.
Plus it requires some CSR work.
/issue add JDK-8310334 |
@azvegint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is good as is, an additional option could be added if this functionality will work well and no other solution will not be found.
But we also can try to close the session if the robot object is collected by GC.
@azvegint This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 135 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
/integrate |
Going to push as commit 2f04bc5.
Your commit was automatically rebased without conflicts. |
Right now, each call to getPixelColor/createScreenCapture opens and closes a ScreenCast session.
When there are many such calls in a row, it becomes a rather time-consuming operation.
For example,
50x50 took 375219ms
This fix offers optimization by keeping the session open and closing it only 2 seconds after the last activity.
50x50 took 28113ms
which is ~ 13 times faster.Testing looks good.
Progress
Issues
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/15250/head:pull/15250
$ git checkout pull/15250
Update a local copy of the PR:
$ git checkout pull/15250
$ git pull https://git.openjdk.org/jdk.git pull/15250/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 15250
View PR using the GUI difftool:
$ git pr show -t 15250
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/15250.diff
Webrev
Link to Webrev Comment