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

Use kotlinx-coroutines for UI launcher #712

Merged
merged 7 commits into from Jun 3, 2022
Merged

Conversation

vlsi
Copy link
Collaborator

@vlsi vlsi commented May 9, 2022

Description

This PR moves startGui from class JMeter to another class.

Motivation and Context

a) It is nice to factor UI from non-UI code
b) Current code fails to update progress during "GUI startup". The reason is that all the contents of startGui is running in a single Runnable, so Swing has no way to break in-between and render progress updates.
See https://bz.apache.org/bugzilla/show_bug.cgi?id=66044#c5

Coroutines enable an easy-to-maintain approach for splitting long UI methods into chunks.
In this case, I used yield() right after setProgress, so we give room for the Swing event dispatcher thread to update the UI.

See also https://github.com/Kotlin/kotlinx.coroutines/blob/master/ui/coroutines-guide-ui.md

Checklist:

  • My code follows the code style of this project.
  • I have updated the documentation accordingly.

@vlsi vlsi marked this pull request as draft May 9, 2022 10:41
private suspend fun setProgress(splash: SplashScreen, progress: Int) {
splash.setProgress(progress)
// Allow UI updates
yield()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@FSchumacher , this enables to make a brief pause in the execution of the current coroutine, and it immediately schedules the continuation. That allows Swing EDT to update the UI.
The code in startGuiPartTwo looks sequential, however, in practice, it is split into a sequence of chunks.

Comment on lines 119 to 124
withContext(Dispatchers.Default) {
f = File(testFile)
log.info("Loading file: {}", f)
FileServer.getFileServer().setBaseForScript(f)
tree = SaveService.loadTree(f)
}
Copy link
Collaborator Author

@vlsi vlsi May 9, 2022

Choose a reason for hiding this comment

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

This is what switching between "regular thread pool" and "Swing event dispatcher" would look like. The code within withContext(Dispatchers.Default) is executed outside of the EDT, and then the execution moves back to the EDT.

@codecov-commenter
Copy link

codecov-commenter commented May 9, 2022

Codecov Report

Merging #712 (52b8c3d) into master (cbacd08) will increase coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #712   +/-   ##
=========================================
  Coverage     55.23%   55.23%           
- Complexity    10382    10383    +1     
=========================================
  Files          1061     1062    +1     
  Lines         65763    65760    -3     
  Branches       7530     7533    +3     
=========================================
+ Hits          36322    36324    +2     
+ Misses        26841    26837    -4     
+ Partials       2600     2599    -1     
Impacted Files Coverage Δ
...c/core/src/main/java/org/apache/jmeter/JMeter.java 42.79% <0.00%> (+4.42%) ⬆️
...c/main/java/org/apache/jmeter/JMeterGuiLauncher.kt 0.00% <0.00%> (ø)
...isualizers/backend/influxdb/HttpMetricsSender.java 80.20% <0.00%> (+2.08%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cbacd08...52b8c3d. Read the comment docs.

@vlsi vlsi marked this pull request as ready for review May 11, 2022 13:52
@vlsi
Copy link
Collaborator Author

vlsi commented May 11, 2022

@pmouawad , @FSchumacher , team. do you have any thoughts/comments? I think this is pretty much mergeable, except I would like to make JMeterGuiLauncher private or something like that.
I really like how coroutine-swing enables writing sequential code that is executed in chunks and moved across thread pools via withContext as needed (UI thread vs computation thread).

I am fine if this is merged after 5.5 release, however, including the change in 5.5 might work as well.

Copy link
Contributor

@pmouawad pmouawad left a comment

Choose a reason for hiding this comment

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

Thanks

@vlsi vlsi merged commit 52b8c3d into apache:master Jun 3, 2022
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.

None yet

3 participants