Fix type instability of closures capturing types (2) #40985

simeonschaub · 2021-05-28T12:09:34Z

Instead of closures lowering to typeof for the types of captured fields, this introduces a new function _typeof_captured_variable that returns Type{T} if T is a type (w/o free typevars). Also adds special inference support for that function to make it a no-op in most cases.

replaces/closes #35970
fixes #23618

base/compiler/compiler.jl

base/Base.jl

tkf · 2021-05-28T20:26:00Z

I just run ]test ChainRules. Just eyeballing the test result, I don't see a major regression. ~~Though maybe rulesets/LinearAlgebra/symmetric.jl is slower? (307 in this PR to 251 on master)~~ (Edit: never mind, the compilation time is < 1%)

This PR

Testing ChainRules.jl
Testing rulesets/Base/base.jl:
...
113.387431 seconds (194.65 M allocations: 11.406 GiB, 4.15% gc time, 0.18% compilation time)
Testing rulesets/Base/fastmath_able.jl:
135.469393 seconds (201.52 M allocations: 11.869 GiB, 3.89% gc time, 0.03% compilation time)
Testing rulesets/Base/evalpoly.jl:
 35.726451 seconds (64.08 M allocations: 3.702 GiB, 3.45% gc time, 76.07% compilation time)
Testing rulesets/Base/array.jl:
 10.367831 seconds (17.88 M allocations: 1.074 GiB, 3.64% gc time, 21.27% compilation time)
Testing rulesets/Base/arraymath.jl:
...
188.849576 seconds (341.39 M allocations: 19.978 GiB, 3.86% gc time, 92.38% compilation time)
Testing rulesets/Base/indexing.jl:
 17.777968 seconds (26.13 M allocations: 1.505 GiB, 3.51% gc time, 98.34% compilation time)
Testing rulesets/Base/mapreduce.jl:
113.682090 seconds (221.61 M allocations: 12.675 GiB, 5.68% gc time, 0.00% compilation time)
Testing rulesets/Base/sort.jl:
  9.309037 seconds (14.99 M allocations: 894.253 MiB, 4.40% gc time, 10.71% compilation time)

Testing rulesets/Statistics/statistics.jl:
  3.154018 seconds (5.54 M allocations: 337.662 MiB, 6.72% gc time, 18.29% compilation time)

Testing rulesets/LinearAlgebra/dense.jl:
 90.476702 seconds (144.34 M allocations: 8.792 GiB, 4.83% gc time, 20.87% compilation time)
Testing rulesets/LinearAlgebra/norm.jl:
153.307127 seconds (241.14 M allocations: 14.354 GiB, 4.56% gc time, 0.00% compilation time)
Testing rulesets/LinearAlgebra/matfun.jl:
 16.050503 seconds (27.39 M allocations: 2.706 GiB, 8.82% gc time, 0.37% compilation time)
Testing rulesets/LinearAlgebra/structured.jl:
 81.032083 seconds (112.20 M allocations: 6.698 GiB, 3.87% gc time, 21.03% compilation time)
Testing rulesets/LinearAlgebra/symmetric.jl:
307.083321 seconds (385.80 M allocations: 45.909 GiB, 6.67% gc time, 0.46% compilation time)
...
Testing rulesets/LinearAlgebra/blas.jl:
 57.792782 seconds (93.93 M allocations: 7.993 GiB, 5.22% gc time, 0.00% compilation time)
Testing rulesets/LinearAlgebra/lapack.jl:
 18.828087 seconds (29.57 M allocations: 1.831 GiB, 4.03% gc time, 17.00% compilation time)

Testing rulesets/Random/random.jl:
  0.783869 seconds (821.83 k allocations: 50.719 MiB, 90.65% compilation time)

Testing rulesets/packages/NaNMath.jl:
  0.000138 seconds (56 allocations: 4.266 KiB)

...

julia> Base.GIT_VERSION_INFO
Base.GitVersionInfo("75086967ef7e5385b54369cb0f99b8eb9cc7480b", "75086967ef", "sds/tkf/type-capturing", 1189, "2021-05-28 18:48 UTC", false, 3, 1.622195605e9)

master

Testing ChainRules.jl
Testing rulesets/Base/base.jl:
...
110.912486 seconds (191.40 M allocations: 11.192 GiB, 3.72% gc time, 0.18% compilation time)
Testing rulesets/Base/fastmath_able.jl:
133.547750 seconds (199.73 M allocations: 11.757 GiB, 3.86% gc time, 0.03% compilation time)
Testing rulesets/Base/evalpoly.jl:
 34.510045 seconds (63.45 M allocations: 3.660 GiB, 3.54% gc time, 75.96% compilation time)
Testing rulesets/Base/array.jl:
 10.106647 seconds (17.67 M allocations: 1.061 GiB, 3.55% gc time, 20.78% compilation time)
Testing rulesets/Base/arraymath.jl:
...
185.997513 seconds (339.63 M allocations: 19.862 GiB, 4.00% gc time, 92.22% compilation time)
Testing rulesets/Base/indexing.jl:
 17.365063 seconds (25.89 M allocations: 1.489 GiB, 3.21% gc time, 98.30% compilation time)
Testing rulesets/Base/mapreduce.jl:
110.458438 seconds (220.21 M allocations: 12.584 GiB, 4.98% gc time, 0.00% compilation time)
Testing rulesets/Base/sort.jl:
  9.054407 seconds (14.83 M allocations: 882.215 MiB, 4.29% gc time, 11.54% compilation time)

Testing rulesets/Statistics/statistics.jl:
  3.022534 seconds (5.51 M allocations: 335.324 MiB, 4.24% gc time, 16.47% compilation time)

Testing rulesets/LinearAlgebra/dense.jl:
 88.285892 seconds (143.68 M allocations: 8.751 GiB, 4.53% gc time, 20.88% compilation time)
Testing rulesets/LinearAlgebra/norm.jl:
150.491677 seconds (239.33 M allocations: 14.238 GiB, 4.41% gc time, 0.00% compilation time)
Testing rulesets/LinearAlgebra/matfun.jl:
 16.358479 seconds (27.57 M allocations: 2.718 GiB, 9.26% gc time, 0.36% compilation time)
Testing rulesets/LinearAlgebra/structured.jl:
 76.466074 seconds (108.54 M allocations: 6.501 GiB, 4.32% gc time, 22.90% compilation time)
Testing rulesets/LinearAlgebra/symmetric.jl:
251.823712 seconds (348.90 M allocations: 44.452 GiB, 7.87% gc time, 0.61% compilation time)
...
Testing rulesets/LinearAlgebra/blas.jl:
 56.097692 seconds (93.35 M allocations: 7.957 GiB, 5.91% gc time, 0.00% compilation time)
Testing rulesets/LinearAlgebra/lapack.jl:
 18.680355 seconds (29.96 M allocations: 1.859 GiB, 4.90% gc time, 17.17% compilation time)

Testing rulesets/Random/random.jl:
  0.771533 seconds (817.91 k allocations: 50.452 MiB, 90.70% compilation time)

Testing rulesets/packages/NaNMath.jl:
  0.000109 seconds (56 allocations: 4.266 KiB)

...

julia> Base.GIT_VERSION_INFO
Base.GitVersionInfo("61701d7a84be62beaf129b2178ede42882a7a46a", "61701d7a84", "master", 1186, "2021-05-28 09:53 UTC", false, 0, 1.622195605e9)

KristofferC · 2021-05-31T15:20:58Z

@oscardssmith Why is this added to the 1.7 milestone?

vtjnash · 2021-05-31T21:24:15Z

This is not going into v1.7, but is something to review and consider for a next release

simeonschaub · 2021-07-01T19:38:32Z

Triage echoed worries about significantly increased latency in some cases. We should at least run all Base benchmarks on this and we also talked about potentially doing a PkgEval run and analyzing whether there are any outliers in terms of run times.

simeonschaub · 2021-07-01T23:02:04Z

@nanosoldier runbenchmarks(ALL, vs=":master")

@nanosoldier runtests(ALL, vs = ":master")

nanosoldier · 2021-07-02T05:04:33Z

Your package evaluation job has completed - possible new issues were detected. A full report can be found here. cc @maleadt

nanosoldier · 2021-07-02T05:22:55Z

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @christopher-dG

simeonschaub · 2021-10-18T01:50:25Z

Sorry it took me so long to get back to this! Overall, I couldn't measure any significant mean increase in run time for the PkgEval jobs. Just to be safe, I'll rerun PkgEval for all packages whose tests took more than 20% longer.

@nanosoldier runtests(["HealthBase", "P4est", "Pitaya", "Geophysics", "Synthesizer", "LatinHypercubeSampling", "SequentialMonteCarlo", "Batsrus", "InterProcessCommunication", "MixedModels", "RandomizedNMF", "EasyConfig", "Crayons", "CooperativeGames", "DashBootstrapComponents", "Word2Vec", "PatModules", "IntervalTrees", "IncrementalInference", "VT100", "TraitWrappers", "URIParser", "KeywordCalls", "ProgrammableAPI", "LikelihoodProfiler", "DataFitting", "FeatureDescriptors", "StatsDiscretizations", "LittleEndianBase128", "ConventionalApp", "RealContinuedFractions", "PredictMD", "Avalon", "FromDigits", "H3", "SLEEFInline", "DashTable", "MultivariateOrthogonalPolynomials", "Vinyl", "CIAOAlgorithms", "Zabbix", "RenoiseOSC", "UnitTestDesign", "Remark", "Scalpels", "MixedModelsExtras", "RegularizationTools", "ChainLadder", "Morton", "CPLEX", "LCMGL", "LibDeflate", "SuperLU", "QuadraticToBinary", "AbstractIndices", "Gloria", "GLNS", "ArgParse", "TriangleMesh", "BlockBootstrap", "ForecastEval", "SalesForceBulkApi", "MLDataUtils", "SuiteSparseGraphBLAS", "ARFFFiles", "FeatherLib", "StippleLatex", "GtkMarkdownTextView", "ProgressMeter", "LinearFractionalTransformations", "SimpleLife", "Plots", "Glob", "BDF", "CurrenciesBase", "Skyler", "TrackingTimers", "BioServices", "BoltzmannMachines", "ClassicalOrthogonalPolynomials", "Taro", "TensorKit", "ReusePatterns", "SymArrays", "RiskAdjustedLinearizations", "YahooFinance", "JuliaPetra", "PooledArrays", "StaticNumbers", "ExponentialUtilities", "LogCompose", "GitCommand", "Pilot", "FastRounding", "TSAnalysis", "ConstantArrays", "Intervals", "NeuroCore", "Parameters", "AMRVW", "HighFrequencyCovariance", "ComputabilityTheory", "GenFlux", "Shoco", "KiteConnect", "EditionBuilders", "FastFloat16s", "ImGuiOpenGLBackend", "Yao", "ConstructiveGeometry", "CircoCore", "Vortice", "AbaqusReader", "RelocatableFolders", "BayesianNonparametrics", "NearestNeighbors", "ElementaryChemistry", "NLPModelsJuMP", "IPNets", "Tectonic", "JacobiDavidson", "Multisets", "PropertyUtils", "BitInformation", "TensArrays", "ValidatedNumerics", "EffectiveWaves", "MitosisStochasticDiffEq", "Chron", "ApproxFunFourier", "Quadrature", "StateSpaceReconstruction", "HOODESolver", "EcoSISTEM", "MPIMapReduce"], vs = ":master")

nanosoldier · 2021-10-18T03:15:01Z

Your package evaluation job has completed - no new issues were detected. A full report can be found here.

simeonschaub · 2021-10-18T03:46:46Z

@nanosoldier runtests(["JacobiDavidson"], vs = ":master")

nanosoldier · 2021-10-18T04:11:18Z

Your package evaluation job has completed - possible new issues were detected. A full report can be found here.

simeonschaub · 2021-10-18T04:40:40Z

Yeah, doesn't seem like there are any significant outliers, so I will mark this for triage.

JeffBezanson · 2021-10-21T21:06:46Z

base/Base.jl

+# for closures
+function _typeof_captured_variable(Core.@nospecialize x)
+    if x isa DataType
+        if x.layout === C_NULL


Are we able to optimize this out? And what is the reason for widening only this specific kind of abstract type?

Apparently not. This is from my #35970 but I don't remember why I checked it. It could be just a silly artifact of my blind try-and-error back then.

For inference, it looks like

_typeof_captured_variable(Core.@nospecialize x) = isconcretetype(x) ? Core.Typeof(x) : DataType

works. But I wonder if we can just do

const _typeof_captured_variable = Core.Typeof

?

simeonschaub · 2024-09-07T15:02:17Z

@nanosoldier runbenchmarks(ALL, vs=":master")

nanosoldier · 2024-09-07T23:36:22Z

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

simeonschaub · 2024-09-08T06:56:18Z

Benchmarks look good as far as I can tell. Tagging for triage to give the final green light considering some of the concerns voiced above. I do believe we've sufficiently demonstrated that this would be a net win and otherwise this is ready to merge.

oscardssmith · 2024-09-12T01:47:21Z

triage assigned a build time and sysimage size check to Jeff, but merge after that.

base/reflection.jl

JeffBezanson · 2024-09-17T03:31:51Z

I don't see any meaningful changes in the numbers there, so probably good.

vtjnash · 2024-09-17T11:29:02Z

base/compiler/abstractinterpretation.jl

+            rt = Const(has_free_typevars(tv) ? typeof(tv) : Core.Typeof(tv))
+        elseif isType(t)
+            tv = t.parameters[1]
+            rt = Const(has_free_typevars(tv) ? typeof(tv) : Core.Typeof(tv))


This seems mostly likely incorrect changing an uncertain value to a guaranteed one

You mean just the isType branch? Or is there an issue with this optimization as a whole?

Bump. @vtjnash What would a correct version of this look like?

Can you just delete it?

Let me try that again. I remember there being cases where the call wouldn't be eliminated otherwise, but that was also a while ago so things might have changed

simeonschaub · 2024-09-26T14:09:09Z

Ok, this seemed to work, all tests still pass and I've confirmed locally that the call is still eliminated in all cases I care about. Good to merge?

base/compiler/abstractinterpretation.jl

test/core.jl

base/compiler/ssair/inlining.jl

Co-authored-by: Simeon Schaub <[email protected]>

simeonschaub added performance Must go faster compiler:lowering Syntax lowering (compiler front end, 2nd stage) labels May 28, 2021

oscardssmith added this to the 1.7 milestone May 28, 2021

tkf reviewed May 28, 2021

View reviewed changes

base/compiler/compiler.jl Outdated Show resolved Hide resolved

tkf reviewed May 28, 2021

View reviewed changes

base/Base.jl Outdated Show resolved Hide resolved

simeonschaub marked this pull request as ready for review May 29, 2021 22:24

vtjnash removed this from the 1.7 milestone May 31, 2021

vtjnash added the feature Indicates new feature / enhancement requests label May 31, 2021

vtjnash mentioned this pull request May 31, 2021

Fix type instability of closures capturing types #35970

Closed

oxinabox mentioned this pull request Jun 29, 2021

WIP: use preproject implementation JuliaDiff/ChainRules.jl#457

Closed

simeonschaub added needs nanosoldier run This PR should have benchmarks run on it needs pkgeval Tests for all registered packages should be run with this change labels Jul 1, 2021

bvdmitri mentioned this pull request Oct 8, 2021

Lambda function internal fields types aren't always inferred correctly #42559

Open

simeonschaub added this to the 1.8 milestone Oct 18, 2021

simeonschaub added triage This should be discussed on a triage call and removed needs nanosoldier run This PR should have benchmarks run on it needs pkgeval Tests for all registered packages should be run with this change labels Oct 18, 2021

JeffBezanson reviewed Oct 21, 2021

View reviewed changes

simeonschaub force-pushed the sds/tkf/type-capturing branch from 5d0db22 to 593677b Compare September 7, 2024 12:46

simeonschaub added the triage This should be discussed on a triage call label Sep 8, 2024

oscardssmith assigned oscardssmith and JeffBezanson and unassigned oscardssmith Sep 12, 2024

oscardssmith removed triage This should be discussed on a triage call feature Indicates new feature / enhancement requests labels Sep 12, 2024

JeffBezanson reviewed Sep 17, 2024

View reviewed changes

base/reflection.jl Show resolved Hide resolved

simeonschaub force-pushed the sds/tkf/type-capturing branch from 593677b to 0b84d4b Compare September 17, 2024 08:42

simeonschaub added the merge me PR is reviewed. Merge when all tests are passing label Sep 17, 2024

vtjnash reviewed Sep 17, 2024

View reviewed changes

vtjnash removed the merge me PR is reviewed. Merge when all tests are passing label Sep 17, 2024

simeonschaub force-pushed the sds/tkf/type-capturing branch from e0006ec to fe224ab Compare September 21, 2024 18:02

simeonschaub requested a review from vtjnash September 26, 2024 14:09

aviatesk approved these changes Sep 30, 2024

View reviewed changes

base/compiler/abstractinterpretation.jl Outdated Show resolved Hide resolved

test/core.jl Show resolved Hide resolved

aviatesk force-pushed the sds/tkf/type-capturing branch 2 times, most recently from e6eef3c to 1b749b9 Compare October 10, 2024 16:04

aviatesk reviewed Oct 10, 2024

View reviewed changes

base/compiler/ssair/inlining.jl Outdated Show resolved Hide resolved

tkf and others added 5 commits October 11, 2024 12:55

Fix type instability of closures capturing types

34336c1

Co-authored-by: Simeon Schaub <[email protected]>

remove special-casing in inlining

d5d4b9b

fixup

09171a9

partially revert b64b1b3

5d786e4

remove unnecessary special cases for _typeof_captured_variable

7230e78

aviatesk force-pushed the sds/tkf/type-capturing branch from 1b749b9 to 7230e78 Compare October 11, 2024 03:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix type instability of closures capturing types (2) #40985

Fix type instability of closures capturing types (2) #40985

simeonschaub commented May 28, 2021 •

edited

Loading

tkf commented May 28, 2021 •

edited

Loading

KristofferC commented May 31, 2021

vtjnash commented May 31, 2021

simeonschaub commented Jul 1, 2021

simeonschaub commented Jul 1, 2021

nanosoldier commented Jul 2, 2021

nanosoldier commented Jul 2, 2021

simeonschaub commented Oct 18, 2021

nanosoldier commented Oct 18, 2021

simeonschaub commented Oct 18, 2021

nanosoldier commented Oct 18, 2021

simeonschaub commented Oct 18, 2021

JeffBezanson Oct 21, 2021

tkf Oct 21, 2021

simeonschaub commented Sep 7, 2024

nanosoldier commented Sep 7, 2024

simeonschaub commented Sep 8, 2024 •

edited

Loading

oscardssmith commented Sep 12, 2024

JeffBezanson commented Sep 17, 2024

vtjnash Sep 17, 2024

simeonschaub Sep 17, 2024

simeonschaub Sep 19, 2024

vtjnash Sep 19, 2024

simeonschaub Sep 19, 2024

simeonschaub commented Sep 26, 2024

Fix type instability of closures capturing types (2) #40985

Are you sure you want to change the base?

Fix type instability of closures capturing types (2) #40985

Conversation

simeonschaub commented May 28, 2021 • edited Loading

tkf commented May 28, 2021 • edited Loading

This PR

master

KristofferC commented May 31, 2021

vtjnash commented May 31, 2021

simeonschaub commented Jul 1, 2021

simeonschaub commented Jul 1, 2021

nanosoldier commented Jul 2, 2021

nanosoldier commented Jul 2, 2021

simeonschaub commented Oct 18, 2021

nanosoldier commented Oct 18, 2021

simeonschaub commented Oct 18, 2021

nanosoldier commented Oct 18, 2021

simeonschaub commented Oct 18, 2021

JeffBezanson Oct 21, 2021

Choose a reason for hiding this comment

tkf Oct 21, 2021

Choose a reason for hiding this comment

simeonschaub commented Sep 7, 2024

nanosoldier commented Sep 7, 2024

simeonschaub commented Sep 8, 2024 • edited Loading

oscardssmith commented Sep 12, 2024

JeffBezanson commented Sep 17, 2024

vtjnash Sep 17, 2024

Choose a reason for hiding this comment

simeonschaub Sep 17, 2024

Choose a reason for hiding this comment

simeonschaub Sep 19, 2024

Choose a reason for hiding this comment

vtjnash Sep 19, 2024

Choose a reason for hiding this comment

simeonschaub Sep 19, 2024

Choose a reason for hiding this comment

simeonschaub commented Sep 26, 2024

simeonschaub commented May 28, 2021 •

edited

Loading

tkf commented May 28, 2021 •

edited

Loading

simeonschaub commented Sep 8, 2024 •

edited

Loading