Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow constant-propagation to be disabled #42125

Merged
merged 3 commits into from
Sep 10, 2021
Merged

Allow constant-propagation to be disabled #42125

merged 3 commits into from
Sep 10, 2021

Conversation

timholy
Copy link
Member

@timholy timholy commented Sep 5, 2021

Our heuristics for constant propagation are imperfect (and probably
never will be perfect), and I've now seen many examples of
methods that no developer would ask to have const-propped get
that treatment. In some cases the cost for latency/precompilation
is very large. This renames @aggressive_constprop to @constprop
and allows two settings, :aggressive and :none.

Closes #38983 (presuming this will propagate to kw body methods, which were a huge deal in that DataFrames analysis)

Since we haven't released Julia 1.7 yet, I decided it was safe to do the rename. But if folks don't like that, we can have both @aggressive_constprop and @noconstprop. JuliaHub informs me that @aggressive_constprop is being used in half a dozen packages, but I can make PRs against them. And since it doesn't appear in our docs, it's not part of our stable interface anyway.

Our heuristics for constant propagation are imperfect (and probably
never will be perfect), and I've now seen many examples of
methods that no developer would ask to have const-propped get
that treatment. In some cases the cost for latency/precompilation
is very large. This renames `@aggressive_constprop` to `@constprop`
and allows two settings, `:aggressive` and `:none`.

Closes #38983 (presuming this will propagate to kw body methods)
src/ircode.c Outdated Show resolved Hide resolved
@aviatesk
Copy link
Member

aviatesk commented Sep 6, 2021

presuming this will propagate to kw body methods, which were a huge deal in that DataFrames analysis

Actually we need to fix this as well ?

julia> Base.@aggressive_constprop foo(a; kws...) = a + length(kws)
foo (generic function with 1 method)

julia> (@which foo(10)).aggressive_constprop
true

julia> (@which foo(10; kw1 = nothing)).aggressive_constprop
false

The fix can be left for another PR though.

Co-authored-by: Shuhei Kadowaki <40514306+aviatesk@users.noreply.github.com>
Co-authored-by: Martin Holters <martin.holters@hsu-hh.de>
@timholy
Copy link
Member Author

timholy commented Sep 8, 2021

The kwfunc is missing, but the body function is covered:

julia> f = Base.bodyfunction(@which foo(10; kw1 = nothing))
#foo#11 (generic function with 1 method)

julia> m = only(methods(f))
var"#foo#11"(kws::Base.Pairs{Symbol, V, Tuple{Vararg{Symbol, N}}, NamedTuple{names, T}} where {V, N, names, T<:Tuple{Vararg{Any, N}}}, ::typeof(foo), a) in Main at REPL[1]:1

julia> m.aggressive_constprop
true

One consolation is that the kwfunc should be pretty quick to infer for all methods, but there's no obvious limit on the body function inference time. Still, we definitely want to fix it.

@timholy
Copy link
Member Author

timholy commented Sep 9, 2021

One question: do I need to bump the serialization version number?

const ser_version = 15 # do not make changes without bumping the version #!

aggressive_constprop used to be serialized as a Bool, now constprop is serialized as a UInt8. But it's self-describing, so the version number issue would only affect different Julia versions (which presumably have a hard time communicating via the serializer anyway since internal structs, etc, change).

@codecov
Copy link

codecov bot commented Sep 9, 2021

Codecov Report

Merging #42125 (1326e4b) into master (8812c5c) will decrease coverage by 0.14%.
The diff coverage is 85.85%.

❗ Current head 1326e4b differs from pull request most recent head 5f02e1d. Consider uploading reports for the commit 5f02e1d to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master   #42125      +/-   ##
==========================================
- Coverage   79.66%   79.51%   -0.15%     
==========================================
  Files         351      351              
  Lines       77567    77683     +116     
==========================================
- Hits        61790    61769      -21     
- Misses      15777    15914     +137     
Impacted Files Coverage Δ
base/binaryplatforms.jl 81.90% <ø> (+50.60%) ⬆️
base/compiler/types.jl 64.28% <ø> (ø)
base/shell.jl 68.03% <ø> (ø)
base/sysinfo.jl 58.16% <0.00%> (ø)
base/version.jl 70.50% <0.00%> (+0.21%) ⬆️
stdlib/REPL/src/REPL.jl 34.29% <ø> (ø)
base/compiler/abstractinterpretation.jl 69.54% <50.00%> (-1.06%) ⬇️
base/initdefs.jl 42.65% <50.00%> (ø)
base/floatfuncs.jl 88.00% <80.00%> (-2.22%) ⬇️
base/strings/util.jl 86.03% <82.60%> (-0.96%) ⬇️
... and 84 more

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 8812c5c...5f02e1d. Read the comment docs.

src/ircode.c Outdated
@@ -788,7 +788,7 @@ JL_DLLEXPORT jl_code_info_t *jl_uncompress_ir(jl_method_t *m, jl_code_instance_t

jl_code_info_t *code = jl_new_code_info_uninit();
uint8_t flags = read_uint8(s.s);
code->aggressive_constprop = !!(flags & (1 << 4));
code->constprop = (flags >> 4) & 3;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may miss some basic understanding, but why do we need & 3 here (isn't flags >> 4 enough) ?

Another consideration here is, will we need to change this code when we add new flags (say when we add new boolean flag code->noinfer << 5 ? (that's why I suggested #42125 (comment))

Copy link
Member Author

@timholy timholy Sep 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This grabs both bits, without grabbing anything higher. So position 6 and higher are available and won't "contaminate" noinfer if they are nonzero. This is essentially the same as your suggestion but briefer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One stylistic side note, instead of using the integer values directly, creating an enum and using named variables makes refactoring and understanding a tad easier.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe writing it 0b11 instead of 3, just so it's obviously "two bits"? If you created enums, I think you'd need them for both the shifts and the masks, and that would be almost a half-dozen enums for this one flag. Do you think that really would be worth it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I get it. Thanks for your explanation.

I think I will like the idea of enum (we can do that in another PR).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not create a jl_code_info_flag_t or something with bit fields? We use it many other places like

julia/src/julia.h

Lines 168 to 175 in 2f00c5f

uint16_t how:2;
uint16_t ndims:9;
uint16_t pooled:1;
uint16_t ptrarray:1; // representation is pointer array
uint16_t hasptr:1; // representation has embedded pointers
uint16_t isshared:1; // data is shared by multiple Arrays
uint16_t isaligned:1; // data allocated with memalign
} jl_array_flags_t;

Do we need to keep the ABI compat for Serialization.jl?

Copy link
Member Author

@timholy timholy Sep 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that idea but this worries me:

C gives no guarantee of the ordering of fields within machine words, so if you do use them for the latter reason, you program will not only be non-portable, it will be compiler-dependent too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yay, it looks much cleaner

Copy link
Member

@tkf tkf Sep 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... doesn't our GC already rely on the first field of the bitfield to be the lowerest bits?

julia/src/julia.h

Lines 97 to 109 in 50ab3a9

struct _jl_taggedvalue_bits {
uintptr_t gc:2;
};
JL_EXTENSION struct _jl_taggedvalue_t {
union {
uintptr_t header;
jl_taggedvalue_t *next;
jl_value_t *type; // 16-byte aligned
struct _jl_taggedvalue_bits bits;
};
// jl_value_t value;
};

i.e.., tag->bits.gc is used for extracting the lowest two bits of tag->type. (But yes, it'd be nice to be standard-compliant whenever possible.)

@aviatesk
Copy link
Member

aviatesk commented Sep 9, 2021

One question: do I need to bump the serialization version number?

Since v1.7 isn't released yet, I'd like to say we don't need to. Maybe @JeffBezanson has an idea on this ?

src/ircode.c Outdated
@@ -788,7 +788,7 @@ JL_DLLEXPORT jl_code_info_t *jl_uncompress_ir(jl_method_t *m, jl_code_instance_t

jl_code_info_t *code = jl_new_code_info_uninit();
uint8_t flags = read_uint8(s.s);
code->aggressive_constprop = !!(flags & (1 << 4));
code->constprop = (flags >> 4) & 3;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I get it. Thanks for your explanation.

I think I will like the idea of enum (we can do that in another PR).

@aviatesk aviatesk added the merge me PR is reviewed. Merge when all tests are passing label Sep 10, 2021
@timholy
Copy link
Member Author

timholy commented Sep 10, 2021

I bumped the serialization number just as a precaution. If this passes I think we should merge.

@timholy timholy merged commit 77572bc into master Sep 10, 2021
@timholy timholy deleted the teh/no_constprop branch September 10, 2021 11:00
@timholy
Copy link
Member Author

timholy commented Sep 10, 2021

xref JuliaLang/Compat.jl#752

simeonschaub added a commit to JuliaDiff/Diffractor.jl that referenced this pull request Sep 10, 2021
simeonschaub added a commit to simeonschaub/Static.jl that referenced this pull request Sep 10, 2021
simeonschaub added a commit to JuliaDiff/Diffractor.jl that referenced this pull request Sep 10, 2021
chriselrod added a commit to SciML/Static.jl that referenced this pull request Sep 10, 2021
simeonschaub added a commit to JuliaDiff/Diffractor.jl that referenced this pull request Sep 10, 2021
simeonschaub added a commit to simeonschaub/ArrayInterface.jl that referenced this pull request Sep 10, 2021
chriselrod added a commit to JuliaArrays/ArrayInterface.jl that referenced this pull request Sep 10, 2021
mcabbott added a commit to JuliaDiff/Diffractor.jl that referenced this pull request Sep 10, 2021
@DilumAluthge DilumAluthge removed the merge me PR is reviewed. Merge when all tests are passing label Sep 11, 2021
aviatesk added a commit to aviatesk/JET.jl that referenced this pull request Sep 11, 2021
aviatesk added a commit to JuliaDebug/Cthulhu.jl that referenced this pull request Sep 11, 2021
simeonschaub pushed a commit that referenced this pull request Sep 16, 2021
Our heuristics for constant propagation are imperfect (and probably
never will be perfect), and I've now seen many examples of
methods that no developer would ask to have const-propped get
that treatment. In some cases the cost for latency/precompilation
is very large. This renames `@aggressive_constprop` to `@constprop`
and allows two settings, `:aggressive` and `:none`.

Closes #38983

Co-authored-by: Shuhei Kadowaki <40514306+aviatesk@users.noreply.github.com>
Co-authored-by: Martin Holters <martin.holters@hsu-hh.de>
simeonschaub added a commit that referenced this pull request Sep 17, 2021
This also adds a depwarn for `@aggressive_constprop`, which seemed
sensible to me, given that it will be removed in 1.8.

replaces #42125
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
Our heuristics for constant propagation are imperfect (and probably
never will be perfect), and I've now seen many examples of
methods that no developer would ask to have const-propped get
that treatment. In some cases the cost for latency/precompilation
is very large. This renames `@aggressive_constprop` to `@constprop`
and allows two settings, `:aggressive` and `:none`.

Closes JuliaLang#38983

Co-authored-by: Shuhei Kadowaki <40514306+aviatesk@users.noreply.github.com>
Co-authored-by: Martin Holters <martin.holters@hsu-hh.de>
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
Our heuristics for constant propagation are imperfect (and probably
never will be perfect), and I've now seen many examples of
methods that no developer would ask to have const-propped get
that treatment. In some cases the cost for latency/precompilation
is very large. This renames `@aggressive_constprop` to `@constprop`
and allows two settings, `:aggressive` and `:none`.

Closes JuliaLang#38983

Co-authored-by: Shuhei Kadowaki <40514306+aviatesk@users.noreply.github.com>
Co-authored-by: Martin Holters <martin.holters@hsu-hh.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:latency Compiler latency
Projects
None yet
Development

Successfully merging this pull request may close these issues.

precompile interacts badly with Const-specialization
7 participants