improve `effect_free` to fix regressions from #20726 #20843

JeffBezanson · 2017-02-28T21:24:55Z

No description provided.

JeffBezanson · 2017-02-28T21:24:59Z

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

ararslan · 2017-02-28T21:33:49Z

base/inference.jl

+                    # fall-through
+                else
+                    return false
+                end


Couldn't you write this as

if !isa(ft, Const) || !contains_is(_pure_builtins, ft.val) return false end

or am I missing something?

vtjnash · 2017-02-28T21:44:32Z

base/inference.jl

@@ -3405,13 +3408,20 @@ function effect_free(e::ANY, src::CodeInfo, mod::Module, allow_volatile::Bool)
                            # first argument must be immutable to ensure e is affect_free
                            a = ea[2]
                            typ = widenconst(exprtype(a, src, mod))
-                            if !isa(typ, DataType) || typ.mutable || typ.abstract
+                            if !isa(typ, DataType) || typ.mutable || (typ.abstract && !isconstType(typ))


some of the fields of DataType aren't affect_free (uid isn't constant)

Is it possible for code to actually observe that change though? I have trouble believing this is a real issue.

Precompile could. But I have trouble believing we should add that bug. Can you actually observe the benefit of this change for well-written code?

Yes, this is needed here; the code in question does something like Tuple{...}.types. Would it be sufficient just to exclude anything that might access uid?

vtjnash · 2017-02-28T21:45:28Z

base/inference.jl

+            elseif is_known_call(e, _apply, src, mod) && length(ea) > 1
+                ft = exprtype(ea[2], src, mod)
+                if isa(ft, Const) && contains_is(_pure_builtins, ft.val)
+                    # fall-through


need to duplicate the rest of the logic from :call here (well, DRY it in a helper function probably)

That's why I only allowed _pure_builtins; looking into apply's arguments is difficult.

That's why I only allowed _pure_builtins

ah, right. So this won't handle return_type either, but is still needed for this PR?

Yes. Nothing here was splatting into return_type itself (the code is _return_type(f, eltypestuple(A, Bs...))), but in theory that could need to be added.

It's marked @pure though; that should already express that it can be considered effect-free. Couldn't we just ccall arg_type_tuple (or write a proper O(n) Julia function) instead of this crazy O(n^2 * log(n)) implementation though?

I think there are cases of splatting args to a @pure function where we do not end up calling pure_eval_call. That could be fixed as well.

vtjnash

Line 3400 has an isa test. That's from an old bug where we were passing in the wrong sv object. I think that's fixed now that we're passing in the right src object. Is that not true somewhere?
(https://github.com/JuliaLang/julia/pull/20843/files#diff-c0d983631c937ee5acfd0137cd7332deR3400)

vtjnash · 2017-02-28T21:55:51Z

base/inference.jl

@@ -3405,13 +3408,20 @@ function effect_free(e::ANY, src::CodeInfo, mod::Module, allow_volatile::Bool)
                            # first argument must be immutable to ensure e is affect_free
                            a = ea[2]
                            typ = widenconst(exprtype(a, src, mod))
-                            if !isa(typ, DataType) || typ.mutable || typ.abstract
+                            if !isa(typ, DataType) || typ.mutable || (typ.abstract && !isconstType(typ))


Precompile could. But I have trouble believing we should add that bug. Can you actually observe the benefit of this change for well-written code?

vtjnash · 2017-02-28T22:02:25Z

base/inference.jl

+            elseif is_known_call(e, _apply, src, mod) && length(ea) > 1
+                ft = exprtype(ea[2], src, mod)
+                if isa(ft, Const) && contains_is(_pure_builtins, ft.val)
+                    # fall-through


That's why I only allowed _pure_builtins

ah, right. So this won't handle return_type either, but is still needed for this PR?

JeffBezanson · 2017-02-28T22:17:44Z

Yes, I believe all effect_free calls now pass the right argument.

nanosoldier · 2017-03-01T00:53:20Z

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

vchuravy · 2017-03-01T01:07:42Z

Let's do this again for the most recent changes @nanosoldier runbenchmarks(ALL, vs = ":master")

nanosoldier · 2017-03-01T04:37:00Z

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

JeffBezanson · 2017-03-01T20:34:07Z

So, what do we make of those results? I'm not sure what to conclude.

Keno · 2017-03-01T20:36:24Z

I'd rebase the branch and run it again.

JeffBezanson · 2017-03-01T22:58:39Z

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

nanosoldier · 2017-03-02T02:28:10Z

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

tkelman · 2017-03-02T04:09:24Z

where's travis here? attempting to re trigger it

JeffBezanson · 2017-03-02T20:24:44Z

Should we merge this? It does seem to fix the vector arithmetic issue. The code for Complex{Int64} looks unchanged vs. master.

vtjnash · 2017-03-02T21:05:32Z

base/inference.jl

                        et = exprtype(e, src, mod)
                        if !isa(et,Const) && !(isType(et) && isleaftype(et))
                            # first argument must be immutable to ensure e is affect_free
                            a = ea[2]
                            typ = widenconst(exprtype(a, src, mod))
-                            if !isa(typ, DataType) || typ.mutable || typ.abstract
+                            if isconstType(typ)
+                                if Const(:uid) ⊑ exprtype(ea[3], src, mod)


that's quite optimistic :P

as in incorrect? can't tell if this is an objection to the change or not

yes, it's blacklist of one bad case but doesn't actually handle the general case

although it's possible we should just be more nuanced here. the uid field is probably the only observable value that can change, and it only can change when loading pre-compiled code – so we really only need to be concerned about constant folding it during then. for all other purposes, it's effect-free.

For constant folding, getfield_tfunc uses a whitelist of DataType fields (name, parameters, types, super), so I don't think there's any risk of constant folding the value of type.uid.

so I guess this effect-free test is basically equivalent to tbaa_const, for all current usages? (which all fields of DataType should be)

Yes that sounds right.

ararslan reviewed Feb 28, 2017

View reviewed changes

vtjnash reviewed Feb 28, 2017

View reviewed changes

JeffBezanson force-pushed the jb/fix20726 branch from 741ec6a to 3475b6c Compare February 28, 2017 23:08

JeffBezanson force-pushed the jb/fix20726 branch from 3475b6c to 33dac60 Compare March 1, 2017 22:58

tkelman closed this Mar 2, 2017

tkelman reopened this Mar 2, 2017

vtjnash reviewed Mar 2, 2017

View reviewed changes

improve effect_free to fix regressions from #20726

2f800a5

JeffBezanson force-pushed the jb/fix20726 branch from 33dac60 to 2f800a5 Compare March 6, 2017 18:25

JeffBezanson merged commit 2e12d10 into master Mar 7, 2017

ararslan deleted the jb/fix20726 branch March 7, 2017 03:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve `effect_free` to fix regressions from #20726 #20843

improve `effect_free` to fix regressions from #20726 #20843

JeffBezanson commented Feb 28, 2017

JeffBezanson commented Feb 28, 2017

ararslan Feb 28, 2017

vtjnash Feb 28, 2017

JeffBezanson Feb 28, 2017

vtjnash Feb 28, 2017

JeffBezanson Feb 28, 2017

vtjnash Feb 28, 2017

JeffBezanson Feb 28, 2017

vtjnash Feb 28, 2017

JeffBezanson Feb 28, 2017

vtjnash Feb 28, 2017

JeffBezanson Feb 28, 2017

vtjnash left a comment

vtjnash Feb 28, 2017

vtjnash Feb 28, 2017

JeffBezanson commented Feb 28, 2017

nanosoldier commented Mar 1, 2017

vchuravy commented Mar 1, 2017

nanosoldier commented Mar 1, 2017

JeffBezanson commented Mar 1, 2017

Keno commented Mar 1, 2017

JeffBezanson commented Mar 1, 2017

nanosoldier commented Mar 2, 2017

tkelman commented Mar 2, 2017

JeffBezanson commented Mar 2, 2017

vtjnash Mar 2, 2017

StefanKarpinski Mar 2, 2017

vtjnash Mar 2, 2017

JeffBezanson Mar 3, 2017

vtjnash Mar 3, 2017

JeffBezanson Mar 3, 2017

improve effect_free to fix regressions from #20726 #20843

improve effect_free to fix regressions from #20726 #20843

Conversation

JeffBezanson commented Feb 28, 2017

JeffBezanson commented Feb 28, 2017

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

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

Choose a reason for hiding this comment

vtjnash left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeffBezanson commented Feb 28, 2017

nanosoldier commented Mar 1, 2017

vchuravy commented Mar 1, 2017

nanosoldier commented Mar 1, 2017

JeffBezanson commented Mar 1, 2017

Keno commented Mar 1, 2017

JeffBezanson commented Mar 1, 2017

nanosoldier commented Mar 2, 2017

tkelman commented Mar 2, 2017

JeffBezanson commented Mar 2, 2017

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

Choose a reason for hiding this comment

improve `effect_free` to fix regressions from #20726 #20843

improve `effect_free` to fix regressions from #20726 #20843