Improve precompilation with concrete typing #436

timholy · 2020-08-19T20:56:04Z

MSC was being called during precompilation with non-inferrable argument types and consequently was an invalidation tigger (and risk). This also specifies concrete types for some of our bigger functions, which makes them more likely to compile only once, and then uses small normalization wrappers to handle argument diversity.

This cuts out more than a dozen invalidations.

kimikage · 2020-08-20T02:08:32Z

src/parse.jl

@@ -150,6 +150,7 @@ function _parse_colorant(desc::AbstractString)

    error("Unknown color: ", desc)
 end
+_parse_colorant(desc::AbstractString) = _parse_colorant(String(desc))


The _parse_colorant is an internal function, so I don't think we need to provide this method. Alternatively, we can force the conversion at higher levels (i.e. Base.parse).

Base.parse(::Type{C}, desc::AbstractString) where {C<:Colorant} = _parse_colorant(C, supertype(C), String(desc))

Also, I think we can rewrite the workaround as follows in Julia v1:

function Base.parse(::Type{C}, desc::AbstractString) where {C<:Colorant} c = _parse_colorant(String(desc))::Colorant # BTW, we know that `c` is a `Colorant` supertype(C) <: Colorant ? convert(C, c)::C : c end

Indeed I don't think we need the supertype stuff at all now.

That's right. However, I am concerned that convert will result in a slight degradation in speed performance. (ColorTypes.jl defines the method to pass through and I'm not sure why it affects the speed. 😕)

Even so, it does improve the following strange behavior.:smile:

julia> parse(Colorant{Float32}, "red") RGB{N0f8}(1.0,0.0,0.0) # current RGB{Float32}(1.0f0,0.0f0,0.0f0) # force conversion

This is just for your reference and I don't think the slowdown is fatal.

The optimizer elides the call to convert when it's a no-op:

julia> @code_typed parse(Colorant, "red") CodeInfo( 1 ─ %1 = invoke Colors._parse_colorant(_3::String)::Any │ Core.typeassert(%1, Colors.Colorant)::Colorant │ %3 = π (%1, Colorant) └── return %3 ) => Colorant julia> @code_typed parse(RGB{N0f8}, "red") CodeInfo( 1 ─ %1 = $(Expr(:static_parameter, 1))::Core.Const(RGB{N0f8}, false) │ %2 = invoke Colors._parse_colorant(_3::String)::Any │ Core.typeassert(%2, Colors.Colorant)::Colorant │ %4 = π (%2, Colorant) │ %5 = Colors.convert(%1, %4)::Any │ Core.typeassert(%5, $(Expr(:static_parameter, 1)))::RGB{N0f8} │ %7 = π (%5, RGB{N0f8}) └── return %7 ) => RGB{N0f8}

The type-assert is necessary because _parse_colorant has a sufficiently-complicated Union that inference justifiably chooses to declare it Any.

Hmm, that is surprising. It doesn't seem to be fixed by the supertype check. I am a bit puzzled. I'd stilll say this is an improvement, and that we can look into the optimizer issues later.

I added a ::Colorant assertion, thinking it would rather help the optimizer, but it seems to be the cause.:sob:

You probably saw this, but for the benefit of all: xref JuliaLang/julia#37135.

Check out this implementation:

function Base.parse(::Type{C}, desc::AbstractString) where {C<:Colorant} c = _parse_colorant(String(desc)) return isconcretetype(C) ? C(c)::C : c end julia> @btime parse(Colorant, "red"); 404.800 ns (5 allocations: 144 bytes) julia> @btime parse(RGB{N0f8}, "red"); 501.647 ns (5 allocations: 144 bytes)

Yet of course the second will be better if the return type is used in other code, since it will assure the compiler that it can do compile-time lookup of all the calls. Conversely, just knowing it's a colorant doesn't limit the space of future calls very much (e.g., think of convert(C2, c) where C2 is some other colorspace--there are far too many choices for union/world-splitting).

I vote we go with this implementation?

Hmm, that one was wrong...how about this:

function Base.parse(::Type{C}, desc::AbstractString) where {C<:Colorant} c = _parse_colorant(String(desc)) return C === Colorant ? c : isconcretetype(C) ? convert(C, c)::C : convert(C, c) end julia> @btime parse(Colorant, "red"); 407.970 ns (5 allocations: 144 bytes) julia> @btime parse(RGB{N0f8}, "red"); 539.243 ns (5 allocations: 144 bytes) julia> @btime parse(RGB, "red"); 521.741 ns (5 allocations: 144 bytes)

src/colormaps.jl

kimikage · 2020-08-20T04:35:06Z

This cuts out more than a dozen invalidations.

How was that number measured? I think the value is highly dependent on the packages you load.

The reason I have a concern with this is because of its relevance to PR #427. If necessary, I can split the PR #427 into fixes related to the MSC and others.

As mentioned in JuliaGraphics/ColorTypes.jl#213 (comment), I'm beware of breaking changes in Colors.jl.

codecov · 2020-08-20T10:30:03Z

Codecov Report

Merging #436 into master will decrease coverage by 0.70%.
The diff coverage is 88.88%.

@@            Coverage Diff             @@
##           master     #436      +/-   ##
==========================================
- Coverage   82.01%   81.31%   -0.71%     
==========================================
  Files          10       10              
  Lines         862      867       +5     
==========================================
- Hits          707      705       -2     
- Misses        155      162       +7

Impacted Files	Coverage Δ
src/colormaps.jl	`90.72% <80.00%> (-5.03%)`	⬇️
src/parse.jl	`95.77% <100.00%> (+0.06%)`	⬆️
src/display.jl	`98.82% <0.00%> (-1.18%)`	⬇️
src/algorithms.jl	`58.87% <0.00%> (-0.94%)`	⬇️

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 0e37ed4...113e84b. Read the comment docs.

MSC was being called with non-inferrable argument types and consequently was an invalidation tigger (and risk). This also specifies concrete types for some of our bigger functions, which makes them more likely to compile only once, and then uses small normalization wrappers to handle argument diversity. This cuts out more than a dozen invalidations.

timholy · 2020-08-20T10:42:42Z

How was that number measured? I think the value is highly dependent on the packages you load.

Just using Colors in a fresh Julia session (except for having loaded SnoopCompileCore). The number of invalidations can be counted using length(uninvalidated(invs)) from SnoopCompile. And yes, the number depends very much on what else you load, at least until the community starts tackling these issues more broadly.

timholy · 2020-08-20T10:45:28Z

If necessary, I can split the PR #427 into fixes related to the MSC and others.

I don't think that's necessary, this is quite orthogonal to that work. Which, by the way, is very welcome in its own right.

kimikage · 2020-08-20T14:20:36Z

I didn't see a clear difference on the nightly build (Commit bd65e85ca8) for Windows.

However, the revised changes in this PR seem to make things better.

timholy · 2020-08-20T16:20:57Z

OK to merge?

kimikage · 2020-08-20T22:12:58Z

It might be a good practice to check the performance with SubString input, just to be sure. I'm writing from my phone right now and haven't tested it.

Edit:

after: #436 (comment)

julia> julia> versioninfo()
Julia Version 1.5.0
Commit 96786e22cc (2020-08-01 23:44 UTC)
Platform Info:
  OS: Windows (x86_64-w64-mingw32)
  CPU: Intel(R) Core(TM) i7-8565U CPU @ 1.80GHz
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-9.0.1 (ORCJIT, skylake)

julia> s = "lightgoldenrodyellow"; ss = SubString(s, 1)
"lightgoldenrodyellow"

julia> @btime parse(Colorant, $s);
  737.390 ns (3 allocations: 128 bytes) # before
  732.300 ns (3 allocations: 128 bytes) # after

julia> @btime parse(Colorant, $ss);
  752.174 ns (3 allocations: 128 bytes) # before
  753.381 ns (4 allocations: 176 bytes) # after

I think it is OK to ignore the allocation within String().

timholy · 2020-08-25T14:13:45Z

FYI JuliaLang/julia#37192 gave me this:

julia> @btime parse(Colorant, "red");
  341.559 ns (1 allocation: 16 bytes)

compared to

julia> @btime parse(Colorant, "red");
  407.970 ns (5 allocations: 144 bytes)

above.

kimikage reviewed Aug 20, 2020

View reviewed changes

src/colormaps.jl Outdated Show resolved Hide resolved

timholy force-pushed the teh/pc_invalidation branch from ba20e29 to be8085a Compare August 20, 2020 10:26

timholy force-pushed the teh/pc_invalidation branch from be8085a to 80f1641 Compare August 20, 2020 10:41

Improve the parse implementation

113e84b

timholy merged commit aef107a into master Aug 21, 2020

timholy deleted the teh/pc_invalidation branch August 21, 2020 10:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve precompilation with concrete typing #436

Improve precompilation with concrete typing #436

timholy commented Aug 19, 2020

kimikage Aug 20, 2020

kimikage Aug 20, 2020

timholy Aug 20, 2020

kimikage Aug 20, 2020 •

edited

Loading

timholy Aug 20, 2020

timholy Aug 20, 2020

kimikage Aug 20, 2020

timholy Aug 20, 2020

timholy Aug 20, 2020 •

edited

Loading

timholy Aug 20, 2020

kimikage commented Aug 20, 2020

codecov bot commented Aug 20, 2020 •

edited

Loading

timholy commented Aug 20, 2020

timholy commented Aug 20, 2020

kimikage commented Aug 20, 2020 •

edited

Loading

timholy commented Aug 20, 2020

kimikage commented Aug 20, 2020 •

edited

Loading

timholy commented Aug 25, 2020

Improve precompilation with concrete typing #436

Improve precompilation with concrete typing #436

Conversation

timholy commented Aug 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kimikage Aug 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timholy Aug 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kimikage commented Aug 20, 2020

codecov bot commented Aug 20, 2020 • edited Loading

Codecov Report

timholy commented Aug 20, 2020

timholy commented Aug 20, 2020

kimikage commented Aug 20, 2020 • edited Loading

timholy commented Aug 20, 2020

kimikage commented Aug 20, 2020 • edited Loading

timholy commented Aug 25, 2020

kimikage Aug 20, 2020 •

edited

Loading

timholy Aug 20, 2020 •

edited

Loading

codecov bot commented Aug 20, 2020 •

edited

Loading

kimikage commented Aug 20, 2020 •

edited

Loading

kimikage commented Aug 20, 2020 •

edited

Loading