Understanding and reducing invalidation caused by CxxWrap #278
Comments
Closed
|
If you're interested in fixing these, one thing I've noted is that many people seem to think the fix is in the "offending" package. Occasionally that's true (#265 was a good example and an excellent change), but most often the fix lies in the target. Basically, anything that has poor inferrability is a vulnerable target, and improving inferrability makes it less vulnerable. |
|
CxxWrap still seems to cause (??) lots of invalidations, e.g. in If I load CxxWrap it goes up to 3.3 seconds: Looking at invalidations, in my report above, we had 8226 from loading CxxWrap. Now with Julia 1.8.3, it is 20750 (!), with 1.10 it is down to "just" 14284: These main invalidations go away with this patch (down to 1957): diff --git a/src/CxxWrap.jl b/src/CxxWrap.jl
index 5847ba8..4eb8ed6 100644
--- a/src/CxxWrap.jl
+++ b/src/CxxWrap.jl
@@ -272,13 +272,13 @@ Base.setindex!(x::CxxBaseRef, val, i::Int) = Base.setindex!(x[], val, i)
Base.convert(::Type{RT}, p::SmartPointer{T}) where {T, RT <: CxxBaseRef{T}} = p[]
Base.cconvert(::Type{RT}, p::SmartPointer{T}) where {T, RT <: CxxBaseRef{T}} = p[]
-function Base.convert(::Type{T1}, p::SmartPointer) where {T1}
- return cxxupcast(T1, p[])[]
-end
+#function Base.convert(::Type{T1}, p::SmartPointer) where {T1}
+# return cxxupcast(T1, p[])[]
+#end
function Base.convert(to_type::Type{<:Ref{T1}}, p::SmartPointer) where {T1}
return to_type(convert(T1,p))
end
-Base.convert(::Type{Any}, x::SmartPointer) = x
+#Base.convert(::Type{Any}, x::SmartPointer) = x
Base.unsafe_convert(to_type::Type{<:CxxBaseRef}, x) = to_type(x.cpp_object)
Now of course this can't just be applied as-is, as it likely breaks some existing code... but I can't help to wonder if it is really a good idea to have all these automagical conversion if we pay the price for them in invalidations? |
This was referenced Dec 28, 2022
|
Just as a minor update, the number of invalidations reported by running the code in my initial comment on this issue is as follows right now:
I've made all three PRs based on tests done on 1.10. Now that I "benchmarked" them also against 1.8, it is obvious that PR #335 has a negative impact there. I have not yet tried to find out what causes this. But at least #337 and #338 have a positive impact both on their own and together, and also in both Julia 1.8.4 and Julia master. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
So I run into https://discourse.julialang.org/t/new-tools-for-reducing-compiler-latency/52882 today and started reading https://julialang.org/blog/2021/01/precompile_tutorial/ as well as https://timholy.github.io/SnoopCompile.jl/stable/snoopr/#invalidations
Still got a lot to learn, but some basic tests already seem to indicate that CxxWrap has a lot room for improvement there -- note that this is not improvements that affect that load time of CxxWrap, but rather of packages using it. While that already got a lot better in Julia 1.6/1.7, there is still room to improvement.
Here's a sample run, listing 8226 invalidations:
Turns out the top offender (which is at the end, and shown above) is a
convertmethod for converting toAny, which was already removed by my PR #265 in September -- but apparently no CxxWrap release was made since then, meaning this performance improvement has not yet been released to the general public.Again, fixing this may not affect CxxWrap's startup speed, but it does impact other code (see e.g. the issue linked in PR #265).
Here's another bad one:
This is caused by the following method:
If I am not mistaken, one possible choice for
BaseTisAny, so the above code is effectively equivalent to this:And so in particular, it also covers this special case, which seem highly problematic for the same reasons as discussed in issue #258, and beyond:
The text was updated successfully, but these errors were encountered: