Initialize pointers at runtime #93

musm · 2017-06-04T15:09:01Z

@timholy
@Allardvm this fixes the windows build for me (btw I find it strange that the ENV variables have to loaded at runtime)

Unfortunately this makes loading ImageMagick slow

julia> @time using ImageMagick
  2.865298 seconds (1.69 M allocations: 100.748 MiB, 7.95% gc time)

Allardvm · 2017-06-04T15:46:43Z

Thanks for the help! I agree that the way in which ENV variables interact with precompilation is weird. My last commit worked on unix, but how things work for Windows remains unclear to me. I've tried all kinds of tricks to inspect Julia's ENV at the time of the failing ccalls to libwand, and MAGICK_CODER_MODULE_PATH always seems to be in place--but the call fails anyway as long as precompilation is enabled (but not on unix). Yet everytime I set the ENV variables before calling using ImageMagick, all works well.

I went as far as to build an internal precompiled _ImageMagick module within an uncompiled ImageMagick module that only sets the ENV variables and exports _ImageMagick's functions--but this was way too hacky to be an acceptable solution. Its loading time, btw, was the same as your version--which I like much more.

musm · 2017-06-04T16:36:32Z

Ok yeah it's probably an issue with precompilation the ENV vars.

Not sure if we can do anything about the load time ....

timholy · 2017-06-04T20:44:06Z

What about once again writing any "system" constants (paths etc.) to a file in deps/ and includeing it? It seems reasonable to explore paths when you're building a package, but when using a package it doesn't seem crazy to require fixed paths. If the user changes some aspect of her system, she can always rebuild.

musm · 2017-06-04T22:11:16Z

What about once again writing any "system" constants (paths etc.) to a file in deps/ and includeing it? It seems reasonable to explore paths when you're building a package, but when using a package it doesn't seem crazy to require fixed paths. If the user changes some aspect of her system, she can always rebuild.

yeah the paths are basically already determined and fixed. But the more time conusming part is loading all the symbols in particular MagickWandGenesis and some of the other symbols too.

BTW I think there is a problem in the finalizers in this package (I don't know how to debug finalizers and all the reference counts)

one thing we are not doing is calling GenesisWandTerminus after we are done with the package which we should. If you try to add it in a atexit(..... call the package crashes because it doesn't seem like all the other references have been finalized before the atexit call (which I believe is not guaranteed to run after all the previous finalizers have completed.) I'm not sure we will need some global reference count or similar.

It's kinda hard for me to figure out all these things since I can't find a good clear API reference, the imagemagick website doesn't make it clear... (Do you have any good references on the API )

musm · 2017-06-05T03:19:41Z

I just realized that this is no slower than master MagickWandGenesis is the dominant cost and both here and on master they are loaded in the init files, loading the pointers of the other symbols in a negligible cost.

Allardvm

Perhaps consider the Ref version of libversion, but other than that, LGTM.

Allardvm · 2017-06-05T06:21:19Z

src/libmagickwand.jl

+    ccall((:MagickWandGenesis, libwand), Void, ())
+
+    libversion_string = queryoption("LIB_VERSION_NUMBER")
+    global libversion = VersionNumber(join(split(libversion_string, ',')[1:3], '.'))


Would it be better to use theRef{VersionNumber} version here? __init__() isn't called during compilation, but libversion is used in a precompiled function (https://github.com/musm/ImageMagick.jl/blob/b1178deb283ee2e5ec1e449ddb4b80a7c67e728e/src/ImageMagick.jl#L155).

Allardvm · 2017-06-05T07:06:43Z

Yes, we should call GenesisWandTerminus at exit, but let's figure that out in a separate PR, so the fixed builds are already available.

I haven't found a better reference than the regular C API docs (https://www.imagemagick.org/script/magick-wand.php), but even there it's clear that the exit procedure should destroy all the wands and then call GenesisWandTerminus. atexit() is run before object finalizers, so we'll have to do this in a different way. We could use a global ref count similar to how libgit2 solved this issue: https://github.com/JuliaLang/julia/pull/20124/files, where we increment and decrement when we create and finalize a wand. To prevent terminating the instance because there just happens to be no wand around for a short while, we can add const instance_finished = Ref{Bool}(false) that we flip with an atexit() function. Only when the ref count reaches 0, and instance_finished[] is true, we call terminus.

timholy

Merge whenever you two are satisfied. Thanks for tackling a long-standing problem!

timholy · 2017-06-05T09:27:25Z

src/libmagickwand.jl

+end
+
+const libmagic = Ref{Ptr{Void}}()
+func(fun::Symbol) = Libdl.dlsym(libmagic[], fun)


Can this get a more descriptive name?

timholy · 2017-06-05T09:30:07Z

src/libmagickwand.jl

+const libmagic = Ref{Ptr{Void}}()
+func(fun::Symbol) = Libdl.dlsym(libmagic[], fun)
+
+const MagickWandGenesis                = Ref{Ptr{Void}}()


Given the julia convention of using uppercase for types & modules, is it worth the extra verbosity of calling these libwand_MagickWandGenesis? I don't feel strongly about this, but thought I'd at least bring it up.

timholy · 2017-06-05T09:40:57Z

With regards to cleanup: what about having MagickWand() add obj to a WeakKeyDict; atexit could then iterate over the dict and finalize all the objects. In case you're unfamiliar with how WeakKeyDict works, here's a demo:

julia> A = rand(5,5)
5×5 Array{Float64,2}:
 0.687806  0.218271   0.718677  0.50439   0.974442 
 0.51159   0.0894912  0.528783  0.468124  0.93002  
 0.704555  0.0490828  0.43582   0.46748   0.583288 
 0.739887  0.561882   0.966313  0.188641  0.246377 
 0.16764   0.665912   0.758272  0.697644  0.0569193

julia> B = rand(5, 5)
5×5 Array{Float64,2}:
 0.196685  0.608029  0.168657  0.051015  0.733482  
 0.186336  0.385804  0.590266  0.48538   0.11401   
 0.774902  0.113197  0.418957  0.159507  0.887277  
 0.427635  0.757624  0.623393  0.207146  0.833457  
 0.126983  0.210338  0.345119  0.894254  0.00865314

julia> w = WeakKeyDict()
WeakKeyDict{Any,Any} with 0 entries

julia> w[A] = nothing

julia> w[B] = nothing

julia> w
WeakKeyDict{Any,Any} with 2 entries:
  [0.687806 0.218271 … 0.50439 0.974442; 0.51159 0.0894912 … 0.468124 0.93002; … ; 0.739887 0.561882 … 0.188641 0.246377; 0.16764 0.665912 … 0.697644 0.0569193]   => nothing
  [0.196685 0.608029 … 0.051015 0.733482; 0.186336 0.385804 … 0.48538 0.11401; … ; 0.427635 0.757624 … 0.207146 0.833457; 0.126983 0.210338 … 0.894254 0.00865314] => nothing

julia> A = []
0-element Array{Any,1}

julia> gc(); gc()

julia> w
WeakKeyDict{Any,Any} with 1 entry:
  [0.196685 0.608029 … 0.051015 0.733482; 0.186336 0.385804 … 0.48538 0.11401; … ; 0.427635 0.757624 … 0.207146 0.833457; 0.126983 0.210338 … 0.894254 0.00865314] => nothing

julia> for items in w
           @show items
       end
items = Pair{Any,Any}([0.196685 0.608029 0.168657 0.051015 0.733482; 0.186336 0.385804 0.590266 0.48538 0.11401; 0.774902 0.113197 0.418957 0.159507 0.887277; 0.427635 0.757624 0.623393 0.207146 0.833457; 0.126983 0.210338 0.345119 0.894254 0.00865314], nothing)

musm · 2017-06-05T16:42:53Z

There is something very odd going on in Windows where ImageMagick is picking up a strange version in the /usr/local/etc/ folder on Windows..... I don't understand where that could be coming from
notice even though we download the correct version and set the correct paths in the build file the version it picks up is 7.0.0.

Allardvm · 2017-06-05T20:38:45Z

@musm I've pushed two commits to your branch that fix the Windows build, including the issues with the reported version. The commit descriptions provide some more info on what I've done.

musm · 2017-06-05T20:50:34Z

@Allardvm Thanks!

BTW are you sure we need to add them to the start of the path?
For example, everything works with me if I remove them (does it also work on OSX?)
I'm not a fan of having to mess with path. I think if we properly set the Imagemagick variables everything should be ok...

It's really confusing why
ccall((:MagickWandGenesis, libwand), Void, ()) errors with
ERROR: TypeError: anonymous: in ccall: first argument not a pointer or valid constant expression, expected Ptr, got Tuple{Symbol,String}

Also I'd like to find a cleaner way of initializing the init_envs since it's not needed on the linux builds.
Doing:

const init_envs = Dict{String,String}()

# Find the library
const depsfile = joinpath("..", "deps", "deps.jl")
if isfile(depsfile)
    include(depsfile)
...

and then

    for (key, value) in init_envs
        ENV[key] = value
    end

is pretty ugly since it's not needed when init_envs is empty.

Can we confirm that the function need to be initialized at runtime?
The current approach initializes them all upon load-time

Another strategy is the following:
https://docs.julialang.org/en/stable/manual/calling-c-and-fortran-code/#indirect-calls

I think the advantage of the current approach is that corresponding function calls are fast. Where as with the strategy in the docs the first call with initialize it and then subsequent calls will be cached. So perhaps the initial package load time will be slightly lower (however I think the dominant cost is initializing MagicWandGenesis which will need to be done regardless

Allardvm · 2017-06-05T21:36:13Z

include doesn't need to path since it searches relative to the current file so depsfile = joinpath("..", "deps", "deps.jl") will work.

On my Windows machine, I get the ImageMagick not properly installed. Please run Pkg.build(\"ImageMagick\") then restart Julia.-error if I don't add dirname(@__FILE__). I didn't run into this issue on OSX.

BTW are you sure we need to add them to the start of the path?
For example, everything works with me if I remove them (does it also work on OSX?)
I'm not a fan of having to mess with path. I think if we properly set the Imagemagick variables everything should be ok...

Everything also works fine on OSX, but the Windows build fails if the binaries aren't in PATH. The error is very obscure (suggesting that the PNG coder doesn't exist in a particular location, even though it does), but if the coder is really missing (after removing it) then it pops a different error. I looked for previously reported issues and poked around in the source and found that it needs "convert.exe" on Windows. The library looks for it in the general path, but not in any of the ENV variable paths as far as I can see. I'll push another commit that omits messing with PATH on OSX, since everything works fine without it.

It's really confusing why
ccall((:MagickWandGenesis, libwand), Void, ()) errors with
ERROR: TypeError: anonymous: in ccall: first argument not a pointer or valid constant expression, expected Ptr, got Tuple{Symbol,String}

Yes, that's indeed confusing, and I'm not getting this error on OSX. What do you run to get this error?

Also I'd like to find a cleaner way of initializing the init_envs since it's not needed on the linux builds.

I'm 100% with you on this. Too bad it's a bit of a mess to prototype alternatives since Unix and Windows keep tripping over different things. Maybe things will be a bit more consistent now that we load the Windows library the proper way and actually manage to call Genesis during init etc.

Can we confirm that the function need to be initialized at runtime?
The current approach initializes them all upon load-time
Another strategy is the following:
https://docs.julialang.org/en/stable/manual/calling-c-and-fortran-code/#indirect-calls
I think the advantage of the current approach is that corresponding function calls are fast. Where as with the strategy in the docs the first call with initialize it and then subsequent calls will be cached. So perhaps the initial package load time will be slightly lower (however I think the dominant cost is initializing MagicWandGenesis which will need to be done regardless

I have not yet seen a Windows build pass the tests without this. I'm fine with the current load-time of the library. It's a pretty sizable library and loading is still quite fast. The approach in the docs add further complexity that I'm not sure is worth it for us.

musm · 2017-06-05T22:30:40Z

@Allardvm I pushed a few more commits, primarily changing how we get the library version and also a couple more tests regarding the relative paths and adding to path in windows environments to double check things.

musm · 2017-06-06T00:32:52Z

@Allardvm do you mind squashing some of your commits?

BTW I can confirm that appveyor fails if you don't include the ENV overloading on windows

Another idea is to perhaps call the symbols within :

withenv( ....apropriate env vars...)
....
end

not sure if that is cleaner or not
in any case we should try to find a nicer way of initializing the env vars if possible

More consistent fix to support libraries in the system path Readme now uses Homebrew.jl interface to fix borked installations Made fix compatible with precompilation + update libversion on init Avoid global const in __init__

- MagickWandGenesis needs to be included as a Ref for the Windows fix to work - `depsfile` needs a relative path to work regardless of the user’s pwd() - The binaries in the ImageMagick library need to be available in the user’s PATH. The strange “could not find coder X” errors when using the ImageMagick installation with the directories in place aren’t because the coders couldn’t be found, but because they couldn’t be loaded with “convert.exe”—since it wasn’t in the PATH. By binding against ImageMagick in its proper file structure, Windows also loads the correct library (as evidenced by the version number it reports).

Allardvm · 2017-06-06T06:48:19Z

I've squashed every single consecutive commit by myself so the log now interleaves between your and my commits :)

Allardvm · 2017-06-06T08:06:08Z

I'm satisfied with the current solution. If this passes the tests, then we can merge this as far as I'm concerned.

We should keep a close eye on any issues caused by removing the OSX PATH. It was added to fix JuliaImages/Images.jl#240 when ImageMagick.jl was still part of Images.jl. I'm fine with merging without the PATH, since the tests pass the calls to the equivalent functions in ImageMagick, and calling the equivalent functions in Images also works fine.

musm · 2017-06-06T15:08:26Z

@Allardvm that makes sense. I think there is perhaps some bad interaction between paths in windows (probably with the required packages or pre installed packages on appveyor though either bad bindeps or winrpm configurations that cause the windows builds to fail without the path set)

Now that I think about it more carefully perhaps we should just add back the path overloading on osx as well. Do you mind taking care of this? (I am not familiar with the exact path that needs to be added)

After this change we can merge this pr

Allardvm · 2017-06-06T15:41:22Z

Good spot on terminate instead of terminus!

ImageMagick bin is now in the OSX path, and I've made the ENV's a bit more defensive by pointing it to an @6 version. Shall I squash the PR into one commit before the merge?

musm · 2017-06-06T15:42:51Z

thank you very much! I think the commits are fine the way they now look! That sounds regarding pointing explicitly to the @6 version

Allardvm · 2017-06-06T15:50:32Z

Alright, let's wait for the tests and then merge. As far as I can see, this shouldn't reintroduce #91, but could you also give this a quick look?

@timholy, want to tag a release after the merge?

timholy · 2017-06-06T17:04:25Z

Absolutely to the release. In case you don't know, I think you can do this too (not that I mind doing it): we're using attobot, so it's as easy as going to the "releases" tab on the main page and creating a new one.

timholy · 2017-06-06T17:47:01Z

Woot! 🎆

musm · 2017-06-06T18:36:11Z

@timholy thanks for tagging. Are you sure attobot is installed? I don't see corresponding METADATA entry? It might have to be enabled a per package basis.

timholy · 2017-06-06T19:58:45Z

I'm pretty sure we did, at one point. But I just checked, and it was only MsgPack. I kinda suspect it might have gotten reset. I'm going to turn it on for all repos in JuliaIO (and you can complain here if you don't want that).

tkelman · 2017-06-07T10:11:47Z

Another idea is to perhaps call the symbols within : withenv

That would be much better. Or use an absolute path for the executable, or imagemagick-specific environment variables to tell it where to load everything. Messing with PATH should be done very, very sparingly, especially on windows due to dll conflicts.

musm force-pushed the fx branch from 72399c6 to bc421e5 Compare June 4, 2017 15:53

Allardvm reviewed Jun 5, 2017

View reviewed changes

Allardvm mentioned this pull request Jun 5, 2017

General build and init fixes and improvements #92

Closed

timholy approved these changes Jun 5, 2017

View reviewed changes

musm force-pushed the fx branch from 1cea878 to f9af5c8 Compare June 5, 2017 16:16

musm force-pushed the fx branch 4 times, most recently from 6d2acbd to bac6498 Compare June 6, 2017 00:30

Allardvm and others added 4 commits June 6, 2017 08:39

More resilient precompiled builds

509c064

More consistent fix to support libraries in the system path Readme now uses Homebrew.jl interface to fix borked installations Made fix compatible with precompilation + update libversion on init Avoid global const in __init__

initialize pointers at runtime

e8e7870

Modify libversion logicand add functions for initalization

d4823c2

Allardvm force-pushed the fx branch from bac6498 to d4823c2 Compare June 6, 2017 06:45

Use function instead of Dict to init ENV

94bca64

musm force-pushed the fx branch from f5d408d to 94bca64 Compare June 6, 2017 15:08

Added ImageMagick bins to OSX path

784e8ae

Allardvm merged commit b0bc710 into JuliaIO:master Jun 6, 2017

musm deleted the fx branch June 6, 2017 17:08

This was referenced Jun 6, 2017

could not spawn `'C:\Program Files\ImageMagick-6.9.8-Q16-HDRI\convert.exe C:\Windows\System32\convert.exe' #88

Closed

ImageMagick silently doesn't install on Windows #52

Closed

timholy mentioned this pull request Jul 5, 2018

build with binaryprovider #121

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initialize pointers at runtime #93

Initialize pointers at runtime #93

musm commented Jun 4, 2017 •

edited

Loading

Allardvm commented Jun 4, 2017

musm commented Jun 4, 2017 •

edited

Loading

timholy commented Jun 4, 2017

musm commented Jun 4, 2017 •

edited

Loading

musm commented Jun 5, 2017 •

edited

Loading

Allardvm left a comment

Allardvm Jun 5, 2017

Allardvm commented Jun 5, 2017

timholy left a comment

timholy Jun 5, 2017

timholy Jun 5, 2017

timholy commented Jun 5, 2017

musm commented Jun 5, 2017 •

edited

Loading

Allardvm commented Jun 5, 2017

musm commented Jun 5, 2017 •

edited

Loading

Allardvm commented Jun 5, 2017 •

edited

Loading

musm commented Jun 5, 2017

musm commented Jun 6, 2017 •

edited

Loading

Allardvm commented Jun 6, 2017

Allardvm commented Jun 6, 2017

musm commented Jun 6, 2017

Allardvm commented Jun 6, 2017 •

edited

Loading

musm commented Jun 6, 2017 •

edited

Loading

Allardvm commented Jun 6, 2017

timholy commented Jun 6, 2017

timholy commented Jun 6, 2017

musm commented Jun 6, 2017 •

edited

Loading

timholy commented Jun 6, 2017

tkelman commented Jun 7, 2017

Initialize pointers at runtime #93

Initialize pointers at runtime #93

Conversation

musm commented Jun 4, 2017 • edited Loading

Allardvm commented Jun 4, 2017

musm commented Jun 4, 2017 • edited Loading

timholy commented Jun 4, 2017

musm commented Jun 4, 2017 • edited Loading

musm commented Jun 5, 2017 • edited Loading

Allardvm left a comment

Choose a reason for hiding this comment

Allardvm Jun 5, 2017

Choose a reason for hiding this comment

Allardvm commented Jun 5, 2017

timholy left a comment

Choose a reason for hiding this comment

timholy Jun 5, 2017

Choose a reason for hiding this comment

timholy Jun 5, 2017

Choose a reason for hiding this comment

timholy commented Jun 5, 2017

musm commented Jun 5, 2017 • edited Loading

Allardvm commented Jun 5, 2017

musm commented Jun 5, 2017 • edited Loading

Allardvm commented Jun 5, 2017 • edited Loading

musm commented Jun 5, 2017

musm commented Jun 6, 2017 • edited Loading

Allardvm commented Jun 6, 2017

Allardvm commented Jun 6, 2017

musm commented Jun 6, 2017

Allardvm commented Jun 6, 2017 • edited Loading

musm commented Jun 6, 2017 • edited Loading

Allardvm commented Jun 6, 2017

timholy commented Jun 6, 2017

timholy commented Jun 6, 2017

musm commented Jun 6, 2017 • edited Loading

timholy commented Jun 6, 2017

tkelman commented Jun 7, 2017

musm commented Jun 4, 2017 •

edited

Loading

musm commented Jun 4, 2017 •

edited

Loading

musm commented Jun 4, 2017 •

edited

Loading

musm commented Jun 5, 2017 •

edited

Loading

musm commented Jun 5, 2017 •

edited

Loading

musm commented Jun 5, 2017 •

edited

Loading

Allardvm commented Jun 5, 2017 •

edited

Loading

musm commented Jun 6, 2017 •

edited

Loading

Allardvm commented Jun 6, 2017 •

edited

Loading

musm commented Jun 6, 2017 •

edited

Loading

musm commented Jun 6, 2017 •

edited

Loading