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

empty! the GLMakie screen for reuse instead of closeing and reopening #3881

Merged
merged 5 commits into from
Dec 16, 2024

Conversation

jmert
Copy link
Contributor

@jmert jmert commented May 22, 2024

Description

Fixes #2453

This fixes a problem observed on Linux (across at least a couple of different desktop environments) with windows flashing away and coming back in a different place (can be a different monitor!) when reusing the shared singleton screen.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

@SimonDanisch
Copy link
Member

This is likely not correct and can't be merged like this, but good to know that this fixes it...

@jmert
Copy link
Contributor Author

jmert commented May 23, 2024

The second commit eliminates (locally for me) the test errors that occurred on the last CI run. I tracked down the issue to being caused by the process of saving to a file stopping the render loop, but the reused singleton window would auto-close when its render loop was stopped (after switching to the empty! process instead of close) during the saving process.

Just encouraging the save-to-file code path to use screens from the pool (which still get closed before being re-added to the pool for reuse, IIUC) rather than the singleton solved the particular error cases on my machine, but like you said, this may be a bit fragile and not the ideal solution.

@jmert
Copy link
Contributor Author

jmert commented May 23, 2024

The second commit caused an accumulation of screens and therefore a ballooning memory use. The latest commit simplifies by just copying what seems to be the only line in the close implementation which doesn't relate to controlling the visibility/closed state of the window, and this also seems to resolve the test errors locally for me.

@jmert jmert force-pushed the jw/linux_reuse_empty branch from bc071b3 to 8948efc Compare May 23, 2024 04:23
@jmert
Copy link
Contributor Author

jmert commented May 23, 2024

The debug output to compare to #2453 (comment) is

julia> using GLMakie

julia> ENV["JULIA_DEBUG"] = "GLMakie"
"GLMakie"

julia> scatter(rand(10))
┌ Debug: new singleton screen
└ @ GLMakie ~/.julia/dev/Makie/GLMakie/src/screen.jl:328
┌ Debug: reopening screen
└ @ GLMakie ~/.julia/dev/Makie/GLMakie/src/screen.jl:295
┌ Debug: Applying screen config! to existing screen
└ @ GLMakie ~/.julia/dev/Makie/GLMakie/src/screen.jl:341
┌ Debug: display scene on screen
└ @ GLMakie ~/.julia/dev/Makie/GLMakie/src/screen.jl:407

julia> scatter(rand(10))
┌ Debug: reusing singleton screen
└ @ GLMakie ~/.julia/dev/Makie/GLMakie/src/screen.jl:323
┌ Debug: Leaving renderloop, cause: stopped renderloop
└ @ GLMakie ~/.julia/dev/Makie/GLMakie/src/screen.jl:953
┌ Debug: empty screen!
└ @ GLMakie ~/.julia/dev/Makie/GLMakie/src/screen.jl:570
┌ Debug: reopening screen
└ @ GLMakie ~/.julia/dev/Makie/GLMakie/src/screen.jl:295
┌ Debug: Applying screen config! to existing screen
└ @ GLMakie ~/.julia/dev/Makie/GLMakie/src/screen.jl:341
┌ Debug: display scene on screen
└ @ GLMakie ~/.julia/dev/Makie/GLMakie/src/screen.jl:407

@jmert jmert force-pushed the jw/linux_reuse_empty branch from 8948efc to 9bed591 Compare June 8, 2024 16:53
@jmert jmert force-pushed the jw/linux_reuse_empty branch from 9bed591 to df687a8 Compare July 17, 2024 14:58
@jmert jmert force-pushed the jw/linux_reuse_empty branch 2 times, most recently from 98dc4d6 to 3330ebf Compare August 8, 2024 21:25
@jmert jmert force-pushed the jw/linux_reuse_empty branch from 3330ebf to ec33d87 Compare September 1, 2024 01:57
@jmert jmert force-pushed the jw/linux_reuse_empty branch from ec33d87 to e33b8d2 Compare September 12, 2024 22:03
@jmert jmert force-pushed the jw/linux_reuse_empty branch from e33b8d2 to a5f246e Compare October 7, 2024 14:08
@jmert jmert force-pushed the jw/linux_reuse_empty branch from a5f246e to 549a1c8 Compare November 6, 2024 15:35
…ning

This fixes a problem observed on Linux (across at least a couple of
different desktop environments) with windows flashing away and coming
back in a different place (can be a different monitor!) when reusing the
shared singleton screen.

Also, wait for renderloop task to stop before restoring
`close_after_renderloop` value. Without this, one can observe that the
singleton window is fully closed and reopened by applying the following
patch:

```diff
diff --git a/GLMakie/src/screen.jl b/GLMakie/src/screen.jl
index ff8b69fdc..e18b8ba6b 100644
--- a/GLMakie/src/screen.jl
+++ b/GLMakie/src/screen.jl
@@ -839,6 +839,7 @@ function pause_renderloop!(screen::Screen)
 end

 function stop_renderloop!(screen::Screen; close_after_renderloop=screen.close_after_renderloop)
+    yield()
     # don't double close when stopping renderloop
     c = screen.close_after_renderloop
     screen.close_after_renderloop = close_after_renderloop
@@ -974,7 +975,7 @@ function renderloop(screen)
     end
     if screen.close_after_renderloop
         try
-            @debug("Closing screen after quiting renderloop!")
+            @info("Closing screen after quiting renderloop!")
             close(screen)
         catch e
             @warn "error closing screen" exception=(e, Base.catch_backtrace())
```
which aims to force the renderloop's task to run via the call to
`yield()` so that the task is sleeping during the rest of the function
call. (The logging change just makes the particular action easier to
find than enabling debug-level logging.)

Opening a plot and replotting at the REPL, I observe both the window
quickly close and reappear and the log message being printed by the end
of the renderloop:
```julia-repl
julia> using GLMakie
Precompiling GLMakie
        Info Given GLMakie was explicitly requested, output will be shown live
[ Info: Closing screen after quiting renderloop!
[ Info: Closing screen after quiting renderloop!
[ Info: Closing screen after quiting renderloop!
[ Info: Closing screen after quiting renderloop!
[ Info: Closing screen after quiting renderloop!
[ Info: Closing screen after quiting renderloop!
[ Info: Closing screen after quiting renderloop!
[ Info: Closing screen after quiting renderloop!
[ Info: Closing screen after quiting renderloop!
[ Info: Closing screen after quiting renderloop!
[ Info: Closing screen after quiting renderloop!
[ Info: Closing screen after quiting renderloop!
[ Info: Closing screen after quiting renderloop!
[ Info: Closing screen after quiting renderloop!
[ Info: Closing screen after quiting renderloop!
[ Info: Closing screen after quiting renderloop!
[ Info: Closing screen after quiting renderloop!
[ Info: Closing screen after quiting renderloop!
[ Info: Closing screen after quiting renderloop!
[ Info: Closing screen after quiting renderloop!
[ Info: Closing screen after quiting renderloop!
[ Info: Closing screen after quiting renderloop!
[ Info: Closing screen after quiting renderloop!
[ Info: Closing screen after quiting renderloop!
[ Info: Closing screen after quiting renderloop!
  1 dependency successfully precompiled in 21 seconds. 303 already precompiled.
  1 dependency had output during precompilation:
┌ GLMakie
│  [Output was shown above]
└

julia> scatter([1, 6])

julia> scatter([1, 6])
[ Info: Closing screen after quiting renderloop!

```

With the changes in this commit, all of the log message disappear,
including from the precompile process.
@jmert jmert force-pushed the jw/linux_reuse_empty branch from 549a1c8 to 176d7de Compare December 16, 2024 03:53
Copy link
Member

@SimonDanisch SimonDanisch left a comment

Choose a reason for hiding this comment

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

Thanks, this seems to be working! :)

@ffreyer ffreyer merged commit a1e9fdf into MakieOrg:master Dec 16, 2024
19 of 20 checks passed
@jmert jmert deleted the jw/linux_reuse_empty branch January 3, 2025 23:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Merged
Development

Successfully merging this pull request may close these issues.

GLMakie window is closed and re-opened upon rendering of a new plot
3 participants