Successfully precompile _parse_colorant

[timholy]

This is an expensive method to infer (>50ms on my machine) and it
has never precompiled successfully. It turns out that
calling it during module definition makes it non-precompilable.
I am guessing that this is because there end up being backedges
to something in the temporary process that does the precompilation
that is no longer available (perhaps gensyms in macro expansion?).
In any case, avoiding the call works around the problem.

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (master@86d8e36). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master     #445   +/-   ##
=========================================
  Coverage          ?   92.54%           
=========================================
  Files             ?       10           
  Lines             ?      952           
  Branches          ?        0           
=========================================
  Hits              ?      881           
  Misses            ?       71           
  Partials          ?        0           

Impacted Files	Coverage Δ
src/Colors.jl	100.00% <ø> (ø)
src/parse.jl	94.80% <ø> (ø)

---

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 86d8e36...c53bfec. Read the comment docs.

[kimikage]

BTW, is there a documented way somewhere to determine if the precompilation was successful or not? I will learn it for future uses.

If the calling parse_colorant is the trigger, then the modified definition of JULIA_LOGO_COLORS could be before _precompile_(). (It's a little odd that JULIA_LOGO_COLORS exists in "parse.jl", though.)

Also, does the order of execution of precompile() affect the inference results?

[timholy]

BTW, is there a documented way somewhere to determine if the precompilation was successful or not?

Just check the list returned from @snoopi; if it shows up despite you having put an @assert precompile(...) directive in place, then it didn't "work." Example (on this branch):

julia> using SnoopCompileCore

julia> tinf = @snoopi tmin=0.01 (using Colors; Colors._parse_colorant("rgb(255,0,0)"))
Tuple{Float64, Core.MethodInstance}[]

But if you leave the new precompile in place but don't make the red = colorant"#cb3c33" change, then _parse_colorant appears in the list.

If the calling parse_colorant is the trigger, then the modified definition of JULIA_LOGO_COLORS could be before precompile(). (It's a little odd that JULIA_LOGO_COLORS exists in "parse.jl", though.)

It doesn't matter. Would you prefer the other order? I don't care which we pick.

Also, does the order of execution of precompile() affect the inference results?

It shouldn't...but then again, I was surprised that colorant"#cb3c33" proved to be a problem, so 🤷 ? And I guess there is one way it would: if you precompile at an intermediate stage sort of like in Julia's bootstrap with reduce.jl being followed by reducedim.jl, then precompilation at an intermediate stage will result in invalidation when the improved/more specialzed methods get defined. (This is very rare, but it is the one example I can think of.)

[kimikage]

I don't have a strong preference for the place or order of the definitions themselves (i.e., I don't think this PR needs to be modified), but I would like to clarify what is needed for this measure and what is not.

I was just curious because I thought that _parse_colorant(desc::String) had no room for specialization.

[timholy]

I was just curious because I thought that _parse_colorant(desc::String) had no room for specialization.

Not sure I understand what you mean by this.

[kimikage]

I was just curious because I thought that _parse_colorant(desc::String) had no room for specialization.

Not sure I understand what you mean by this.

I don't understand the intent of the @assert precompile(Tuple{typeof(_parse_colorant),String}) after@assert precompile(Tuple{typeof(parse), .... As you mentioned above, @assert is not helpful in this case.

[timholy]

Ah, I get it! Actually it's not necessary, it was leftover from earlier attempts to fix this. It's actually the deletion of the colorant"#cb3c33" statements that fixes the precompilation.

Updated.

[timholy]

The @assert just checks that precompile returns true:

julia> foo(x) = 1
foo (generic function with 1 method)

julia> precompile(foo, (Int, String))
false

julia> foo(1, "weird")
ERROR: MethodError: no method matching foo(::Int64, ::String)
Closest candidates are:
  foo(::Any) at REPL[2]:1
Stacktrace:
 [1] top-level scope
   @ REPL[4]:1

It's essentially a guard against the precompile silently going stale as we change things.