From adc5c910349548832c6d07b82f69991a867b85b6 Mon Sep 17 00:00:00 2001 From: Simon Date: Wed, 20 Dec 2023 01:33:38 +0100 Subject: [PATCH] Fix & test relocatability (#3490) * fix relocatability * refactor shader source handling * add relocatability test workflow * rename workflow * fix path * add Test library * fix for linux --- .github/workflows/relocatability.yml | 45 ++++++++++ GLMakie/src/GLAbstraction/GLAbstraction.jl | 1 + GLMakie/src/GLAbstraction/GLShader.jl | 95 +++++++--------------- GLMakie/src/GLAbstraction/GLTypes.jl | 2 +- GLMakie/src/GLMakie.jl | 17 +++- GLMakie/test/unit_tests.jl | 42 +++++++++- relocatability.jl | 50 ++++++++++++ 7 files changed, 181 insertions(+), 71 deletions(-) create mode 100644 .github/workflows/relocatability.yml create mode 100644 relocatability.jl diff --git a/.github/workflows/relocatability.yml b/.github/workflows/relocatability.yml new file mode 100644 index 00000000000..5612a9e08e8 --- /dev/null +++ b/.github/workflows/relocatability.yml @@ -0,0 +1,45 @@ +name: Relocatability +on: + pull_request: + paths-ignore: + - 'docs/**' + - '*.md' + branches: + - master + push: + tags: + - '*' + branches: + - master + +concurrency: + group: ${{ github.workflow }}-${{ github.head_ref || github.run_id }} + cancel-in-progress: true + +jobs: + glmakie: + name: GLMakie relocatability + env: + MODERNGL_DEBUGGING: "true" # turn on errors when running OpenGL tests + runs-on: ${{ matrix.os }} + strategy: + fail-fast: false + matrix: + version: + - '1' # automatically expands to the latest stable 1.x release of Julia + os: + - ubuntu-20.04 + arch: + - x64 + steps: + - name: Checkout + uses: actions/checkout@v4 + - uses: julia-actions/setup-julia@v1 + with: + version: ${{ matrix.version }} + arch: ${{ matrix.arch }} + - uses: julia-actions/cache@v1 + - run: sudo apt-get update && sudo apt-get install -y xorg-dev mesa-utils xvfb libgl1 freeglut3-dev libxrandr-dev libxinerama-dev libxcursor-dev libxi-dev libxext-dev xsettingsd x11-xserver-utils + - name: Relocatability test + run: > + DISPLAY=:0 xvfb-run -s '-screen 0 1024x768x24' julia ./relocatability.jl diff --git a/GLMakie/src/GLAbstraction/GLAbstraction.jl b/GLMakie/src/GLAbstraction/GLAbstraction.jl index 67657c15191..e8c36de91c7 100644 --- a/GLMakie/src/GLAbstraction/GLAbstraction.jl +++ b/GLMakie/src/GLAbstraction/GLAbstraction.jl @@ -6,6 +6,7 @@ using Makie using FixedPointNumbers using ColorTypes using ..GLMakie.GLFW +using ..GLMakie: ShaderSource using Printf using LinearAlgebra using Observables diff --git a/GLMakie/src/GLAbstraction/GLShader.jl b/GLMakie/src/GLAbstraction/GLShader.jl index bddc6e61302..0313256dea7 100644 --- a/GLMakie/src/GLAbstraction/GLShader.jl +++ b/GLMakie/src/GLAbstraction/GLShader.jl @@ -103,73 +103,57 @@ function ShaderCache(context) ) end - abstract type AbstractLazyShader end + struct LazyShader <: AbstractLazyShader shader_cache::ShaderCache - paths::Tuple + paths::Vector{ShaderSource} kw_args::Dict{Symbol, Any} - function LazyShader(cache::ShaderCache, paths...; kw_args...) + function LazyShader(cache::ShaderCache, paths::ShaderSource...; kw_args...) args = Dict{Symbol, Any}(kw_args) get!(args, :view, Dict{String, String}()) - new(cache, paths, args) + new(cache, [paths...], args) end end gl_convert(shader::GLProgram, data) = shader - -# TODO remove this silly constructor -function compile_shader(source::Vector{UInt8}, typ, name) - shaderid = createshader(typ) - glShaderSource(shaderid, source) +function compile_shader(source::ShaderSource, template_src::String) + name = source.name + shaderid = createshader(source.typ) + glShaderSource(shaderid, template_src) glCompileShader(shaderid) if !GLAbstraction.iscompiled(shaderid) GLAbstraction.print_with_lines(String(source)) @warn("shader $(name) didn't compile. \n$(GLAbstraction.getinfolog(shaderid))") end - return Shader(name, source, typ, shaderid) + return Shader(name, Vector{UInt8}(template_src), source.typ, shaderid) end -function compile_shader(path, source_str::AbstractString) - typ = shadertype(splitext(path)[2]) - source = Vector{UInt8}(source_str) - name = Symbol(path) - return compile_shader(source, typ, name) -end -function get_shader!(cache::ShaderCache, path, template_replacement) +function get_shader!(cache::ShaderCache, src::ShaderSource, template_replacement) # this should always be in here, since we already have the template keys - shader_dict = cache.shader_cache[path] + shader_dict = cache.shader_cache[src.name] return get!(shader_dict, template_replacement) do - template_source = read(path, String) - source = mustache_replace(template_replacement, template_source) + templated_source = mustache_replace(template_replacement, src.source) ShaderAbstractions.switch_context!(cache.context) - return compile_shader(path, source) + return compile_shader(src, templated_source) end::Shader end -function get_template!(cache::ShaderCache, path, view, attributes) - return get!(cache.template_cache, path) do - _, ext = splitext(path) - - typ = shadertype(ext) - template_source = read(path, String) - source, replacements = template2source( - template_source, view, attributes - ) - s = compile_shader(path, source) +function get_template!(cache::ShaderCache, src::ShaderSource, view, attributes) + return get!(cache.template_cache, src.name) do + templated_source, replacements = template2source(src.source, view, attributes) + shader = compile_shader(src, templated_source) template_keys = collect(keys(replacements)) template_replacements = collect(values(replacements)) # can't yet be in here, since we didn't even have template keys - cache.shader_cache[path] = Dict(template_replacements => s) - + cache.shader_cache[src.name] = Dict(template_replacements => shader) return template_keys end end - -function compile_program(shaders, fragdatalocation) +function compile_program(shaders::Vector{Shader}, fragdatalocation) # Remove old shaders program = createprogram() #attach new ones @@ -210,55 +194,33 @@ gl_convert(lazyshader::AbstractLazyShader, data) = error("gl_convert shader") function gl_convert(lazyshader::LazyShader, data) gl_convert(lazyshader.shader_cache, lazyshader, data) end + function gl_convert(cache::ShaderCache, lazyshader::AbstractLazyShader, data) kw_dict = lazyshader.kw_args paths = lazyshader.paths - if all(x-> isa(x, Shader), paths) - fragdatalocation = get(kw_dict, :fragdatalocation, Tuple{Int, String}[]) - ShaderAbstractions.switch_context!(cache.context) - return compile_program([paths...], fragdatalocation) - end + v = get_view(kw_dict) fragdatalocation = get(kw_dict, :fragdatalocation, Tuple{Int, String}[]) - # Tuple(Source, ShaderType) - if all(paths) do x - isa(x, Tuple) && length(x) == 2 && - isa(first(x), AbstractString) && - isa(last(x), GLenum) - end - # we don't cache view & templates for shader strings! - shaders = map(paths) do source_typ - source, typ = source_typ - src, _ = template2source(source, v, data) - ShaderAbstractions.switch_context!(cache.context) - compile_shader(Vector{UInt8}(src), typ, :from_string) - end - ShaderAbstractions.switch_context!(cache.context) - return compile_program([shaders...], fragdatalocation) - end - if !all(x -> isa(x, AbstractString), paths) - error("Please supply only paths or tuples of (source, typ) for Lazy Shader - Found: $paths" - ) - end template_keys = Vector{Vector{String}}(undef, length(paths)) replacements = Vector{Vector{String}}(undef, length(paths)) - for (i, path) in enumerate(paths) - template = get_template!(cache, path, v, data) + + for (i, shader_source) in enumerate(paths) + template = get_template!(cache, shader_source, v, data) template_keys[i] = template replacements[i] = String[mustache2replacement(t, v, data) for t in template] end + return get!(cache.program_cache, (paths, replacements)) do # when we're here, this means there were uncached shaders, meaning we definitely have # to compile a new program shaders = Vector{Shader}(undef, length(paths)) - for (i, path) in enumerate(paths) + for (i, shader_source) in enumerate(paths) tr = Dict(zip(template_keys[i], replacements[i])) - shaders[i] = get_shader!(cache, path, tr) + shaders[i] = get_shader!(cache, shader_source, tr) end ShaderAbstractions.switch_context!(cache.context) - compile_program(shaders, fragdatalocation) + return compile_program(shaders, fragdatalocation) end end @@ -346,7 +308,6 @@ function mustache2replacement(mustache_key, view, attributes) end # Takes a shader template and renders the template and returns shader source -template2source(source::Vector{UInt8}, view, attributes::Dict{Symbol, Any}) = template2source(String(source), attributes, view) function template2source(source::AbstractString, view, attributes::Dict{Symbol, Any}) replacements = Dict{String, String}() source = mustache_replace(source) do mustache_key diff --git a/GLMakie/src/GLAbstraction/GLTypes.jl b/GLMakie/src/GLAbstraction/GLTypes.jl index 19d7123e4fa..206a8dda42a 100644 --- a/GLMakie/src/GLAbstraction/GLTypes.jl +++ b/GLMakie/src/GLAbstraction/GLTypes.jl @@ -28,7 +28,7 @@ struct Shader id::GLuint context::GLContext function Shader(name, source, typ, id) - new(name, source, typ, id, current_context()) + new(Symbol(name), source, typ, id, current_context()) end end diff --git a/GLMakie/src/GLMakie.jl b/GLMakie/src/GLMakie.jl index 5af488594da..47361006a81 100644 --- a/GLMakie/src/GLMakie.jl +++ b/GLMakie/src/GLMakie.jl @@ -41,16 +41,29 @@ end import ShaderAbstractions: Sampler, Buffer export Sampler, Buffer +struct ShaderSource + typ::GLenum + source::String + name::String +end + +function ShaderSource(path) + typ = GLAbstraction.shadertype(splitext(path)[2]) + source = read(path, String) + name = String(path) + return ShaderSource(typ, source, name) +end + const GL_ASSET_DIR = RelocatableFolders.@path joinpath(@__DIR__, "..", "assets") const SHADER_DIR = RelocatableFolders.@path joinpath(GL_ASSET_DIR, "shader") -const LOADED_SHADERS = Dict{String, String}() +const LOADED_SHADERS = Dict{String, ShaderSource}() function loadshader(name) # Turns out, joinpath is so slow, that it actually makes sense # To memoize it :-O # when creating 1000 plots with the PlotSpec API, timing drop from 1.5s to 1s just from this change: return get!(LOADED_SHADERS, name) do - return joinpath(SHADER_DIR, name) + return ShaderSource(joinpath(SHADER_DIR, name)) end end diff --git a/GLMakie/test/unit_tests.jl b/GLMakie/test/unit_tests.jl index 9ae5d6ca161..409247bc075 100644 --- a/GLMakie/test/unit_tests.jl +++ b/GLMakie/test/unit_tests.jl @@ -6,6 +6,46 @@ function project_sp(scene, point) return point_px .+ offset end +@testset "shader cache" begin + GLMakie.closeall() + screen = display(Figure()) + cache = screen.shader_cache + # Postprocessing shaders + @test length(cache.shader_cache) == 5 + @test length(cache.template_cache) == 5 + @test length(cache.program_cache) == 4 + + # Shaders for scatter + linesegments + poly etc (axis) + display(screen, scatter(1:4)) + @test length(cache.shader_cache) == 16 + @test length(cache.template_cache) == 16 + @test length(cache.program_cache) == 10 + + # No new shaders should be added: + display(screen, scatter(1:4)) + @test length(cache.shader_cache) == 16 + @test length(cache.template_cache) == 16 + @test length(cache.program_cache) == 10 + + # Same for linesegments + display(screen, linesegments(1:4)) + @test length(cache.shader_cache) == 16 + @test length(cache.template_cache) == 16 + @test length(cache.program_cache) == 10 + + # Lines hasn't been compiled so one new program should be added + display(screen, lines(1:4)) + @test length(cache.shader_cache) == 18 + @test length(cache.template_cache) == 18 + @test length(cache.program_cache) == 11 + + # For second time no new shaders should be added + display(screen, lines(1:4)) + @test length(cache.shader_cache) == 18 + @test length(cache.template_cache) == 18 + @test length(cache.program_cache) == 11 +end + @testset "unit tests" begin GLMakie.closeall() @testset "Window handling" begin @@ -250,7 +290,7 @@ end @test screen.root_scene === nothing @test screen.rendertask === nothing - @test (Base.summarysize(screen) / 10^6) < 1.22 + @test (Base.summarysize(screen) / 10^6) < 1.4 end # All should go to pool after close @test all(x-> x in GLMakie.SCREEN_REUSE_POOL, screens) diff --git a/relocatability.jl b/relocatability.jl new file mode 100644 index 00000000000..835aec2fc3a --- /dev/null +++ b/relocatability.jl @@ -0,0 +1,50 @@ + +module_src = """ +module MakieApp + +using GLMakie + +function julia_main()::Cint + screen = display(scatter(1:4)) + # wait(screen) commented out to test if this blocks anything, but didn't change anything + return 0 # if things finished successfully +end + +end # module MakieApp +""" + +using Pkg, Test + +makie_dir = pwd() +tmpdir = mktempdir() +# create a temporary project +cd(tmpdir) +Pkg.generate("MakieApp") +Pkg.activate("MakieApp") + +paths = [makie_dir, joinpath(makie_dir, "MakieCore"), joinpath(makie_dir, "GLMakie")] + +Pkg.develop(map(x-> (;path=x), paths)) + +open("MakieApp/src/MakieApp.jl", "w") do io + print(io, module_src) +end + +Pkg.activate(".") +Pkg.add("PackageCompiler") + +using PackageCompiler + +create_app(joinpath(pwd(), "MakieApp"), "executable"; force=true, incremental=true, include_transitive_dependencies=false) +exe = joinpath(pwd(), "executable", "bin", "MakieApp") +@test success(`$(exe)`) +julia_pkg_dir = joinpath(Base.DEPOT_PATH[1], "packages") +@test isdir(julia_pkg_dir) +mvd_julia_pkg_dir = julia_pkg_dir * ".old" +# Move package dir so that we can test relocatability (hardcoded paths to package dir being invalid now) +try + mv(julia_pkg_dir, mvd_julia_pkg_dir) + @test success(`$(exe)`) +catch e + mv(mvd_julia_pkg_dir, julia_pkg_dir) +end