call convert in Base.setfield! #16226

stevengj · 2016-05-06T02:46:01Z

Fixes #16195, following the suggestion of @vtjnash.

vtjnash · 2016-05-06T04:36:58Z

this looks correct to me, although I think we also need to delete the corresponding lowering code that was inserting calls to convert. I also think we just need to check that it won't cause any performance regressions since now setfield is not handled by a special tfunc.

JeffBezanson · 2016-05-06T04:49:08Z

I think this will definitely be a problem, since the index argument to fieldtype would no longer be a constant.

stevengj · 2016-05-06T12:35:38Z

Don't we just have to make sure it uses Core.setfield! for foo.bar expressions, which are lowered to (top setfield!) right? inference.jl is using Core.setfield! for add_tfunc(setfield!, ...), not Base.setfield!, I think; isn't that enough?

JeffBezanson · 2016-05-06T13:39:30Z

Yes I think lowering will need to be changed to call (|.| Core setfield!).

stevengj · 2016-05-06T14:38:45Z

@JeffBezanson, when I replace (top setfield!) with that in julia-syntax.scm, I get ERROR: syntax: unhandled expr (. Core setfield!). What am I missing?

JeffBezanson · 2016-05-27T05:37:41Z

Good news --- in the interim I've added a core node and changed the lowering to (core setfield!), so this should work fine now.

stevengj · 2016-05-27T15:00:00Z

Is there a way restart Travis, or do I need a forced push?

tkelman · 2016-05-27T17:20:05Z

Log in to travis with github auth then you can restart. Travis overwrites logs on restarts, so please save failure logs to a gist before restarting.

Note that this succeeded 3 weeks ago, but I restarted it this morning to see if the merge commit into latest master would still pass. It didn't, so if you rebase this branch I suspect you'll see the same failure locally. Probably a newly added test that this would break.

stevengj · 2016-05-27T19:13:41Z

test/core.jl

@@ -2238,7 +2238,7 @@ f9534d(x) = (a=(1,2,4,6,7); a[x])
 @test try; f9534d() catch ex; (ex::BoundsError).a === (1,2,4,6,7) && ex.i == 7; end
 @test try; f9534d(-1) catch ex; (ex::BoundsError).a === (1,2,4,6,7) && ex.i == -1; end
 f9534e(x) = (a=IOBuffer(); setfield!(a, x, 3))
-@test try; f9534e(-2) catch ex; isa((ex::BoundsError).a,Base.IOBuffer) && ex.i == -2; end
+@test try; f9534e(-2) catch ex; (ex::BoundsError).a == Base.IOBuffer && ex.i == -2; end


I'm not sure why I had to make this change to get the test to pass. @JeffBezanson, has the behavior of Core.setfield! changed to store the type rather than the value in the a field?

No this should not have changed.

I think this means inlining may have changed? I'm not certain that jl_f_getfield and the codegen optimization for that getfield actually throw the same error.

~~This is probably #16517~~ Actually probably not... that one is only applicable when the one following the catch is a literal.

I think the issue is that Core.setfield!(x, i, convert(fieldtype(T, i), v)) is throwing a BoundsError from fieldtype, not from Core.setfield!.

I'm not sure if there is a clean way to fix this.

If we had an isfield(T, i) intrinsic, I could check it explicitly and then throw an error, but we don't have such an intrinsic. I can replace fieldtype(T, i) with try; fieldtype(T, i); catch; Any; end — that works, but is mildly horrifying.

Suggestions?

We could just call Core.setfield! in the test

We could, but do we care what kind of BoundsError data is thrown by Base.setfield!?

vtjnash · 2016-06-23T03:51:08Z

needs another rebase?

schmrlng · 2016-08-23T05:47:51Z

Bump - as the underlying issue (#16195) here is related to a 0.5 deprecation, should this be added to the 0.5.x milestone?

stevengj · 2016-08-23T12:16:03Z

Rebased.

tkelman · 2016-08-23T12:19:08Z

this may have to queue for a bit

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

nanosoldier · 2016-08-23T15:45:56Z

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

stevengj · 2016-08-23T16:05:04Z

I'm skeptical that the performance regression here is real; setfield! is barely used in Base, and isn't used at all in the sorting code.

KristofferC · 2016-08-23T16:12:39Z

The sort functions seem to generally be a bit noisy so nothing to worry about.

yuyichao · 2016-08-23T16:20:39Z

The BoundsError change sounds like a bug and should be fixed before merging.

StefanKarpinski · 2016-08-23T18:49:17Z

The sort functions seem to generally be a bit noisy so nothing to worry about.

I don't recall if we are or not, but we should be using a fixed random sequence since the randomization has a large impact on sort performance. Obviously, it evens out in the long term, but it's unnecessary noise that we're adding only to have to try to remove it with more runs.

KristofferC · 2016-08-23T18:50:41Z

Pretty sure everything in BaseBenchmarks fixes the seed for all "random" data.

jrevels · 2016-08-24T01:10:29Z

Pretty sure everything in BaseBenchmarks fixes the seed for all "random" data.

This is the case.

fix #16226 (close #16195) fix #16195

New functions `Base.getproperty` and `Base.setproperty!` can be overloaded to change the behavior of `x.f` and `x.f = v`, respectively. fix #16226 (close #16195) fix #1974

New functions `Base.getproperty` and `Base.setproperty!` can be overloaded to change the behavior of `x.p` and `x.p = v`, respectively. This forces inference constant propagation through get/setproperty, since it is very likely this method will yield better information after specializing on the field name (even if `convert` is too big to make us want to inline the generic version and trigger the heuristic normally). closes #16195 (and thus also closes #16226) fix #1974

stevengj force-pushed the setfield_convert branch from b1ff106 to 83b09e2 Compare May 27, 2016 19:11

stevengj reviewed May 27, 2016
View reviewed changes

stevengj and others added 2 commits August 23, 2016 08:10

call convert in Base.setfield! (fixes JuliaLang#16195)

b7f09a7

fix test for change(?) in BoundsError exception for setfield

73808cf

stevengj force-pushed the setfield_convert branch from 83b09e2 to 73808cf Compare August 23, 2016 12:15

vtjnash added a commit that referenced this pull request Dec 5, 2017

implement getfield overloading

7592748

fix #16226 (close #16195) fix #16195

vtjnash added a commit that referenced this pull request Dec 5, 2017

implement getfield overloading

015835f

fix #16226 (close #16195) fix #16195

vtjnash added a commit that referenced this pull request Dec 7, 2017

implement getfield overloading

74146c9

New functions `Base.getproperty` and `Base.setproperty!` can be overloaded to change the behavior of `x.f` and `x.f = v`, respectively. fix #16226 (close #16195) fix #1974

vtjnash mentioned this pull request Dec 7, 2017

implement getfield overloading #24960

Merged

2 tasks

vtjnash added a commit that referenced this pull request Dec 7, 2017

implement getfield overloading

2a9c500

New functions `Base.getproperty` and `Base.setproperty!` can be overloaded to change the behavior of `x.f` and `x.f = v`, respectively. fix #16226 (close #16195) fix #1974

vtjnash added a commit that referenced this pull request Dec 11, 2017

implement getfield overloading

4cf08bb

New functions `Base.getproperty` and `Base.setproperty!` can be overloaded to change the behavior of `x.f` and `x.f = v`, respectively. fix #16226 (close #16195) fix #1974

vtjnash added a commit that referenced this pull request Dec 15, 2017

implement getfield overloading

bdb5b96

New functions `Base.getproperty` and `Base.setproperty!` can be overloaded to change the behavior of `x.f` and `x.f = v`, respectively. fix #16226 (close #16195) fix #1974

vtjnash added a commit that referenced this pull request Dec 17, 2017

implement getfield overloading

8688acc

New functions `Base.getproperty` and `Base.setproperty!` can be overloaded to change the behavior of `x.f` and `x.f = v`, respectively. fix #16226 (close #16195) fix #1974

vtjnash closed this in #24960 Dec 18, 2017

stevengj deleted the setfield_convert branch December 18, 2017 13:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

call convert in Base.setfield! #16226

call convert in Base.setfield! #16226

stevengj commented May 6, 2016

vtjnash commented May 6, 2016

JeffBezanson commented May 6, 2016

stevengj commented May 6, 2016

JeffBezanson commented May 6, 2016

stevengj commented May 6, 2016

JeffBezanson commented May 27, 2016

stevengj commented May 27, 2016

tkelman commented May 27, 2016 •

edited

Loading

stevengj May 27, 2016 •

edited

Loading

JeffBezanson May 27, 2016

vtjnash May 28, 2016

yuyichao May 28, 2016 •

edited

Loading

stevengj Aug 23, 2016

stevengj Aug 23, 2016

vtjnash Aug 23, 2016

stevengj Aug 23, 2016

vtjnash commented Jun 23, 2016

schmrlng commented Aug 23, 2016

stevengj commented Aug 23, 2016

tkelman commented Aug 23, 2016

nanosoldier commented Aug 23, 2016

stevengj commented Aug 23, 2016

KristofferC commented Aug 23, 2016

yuyichao commented Aug 23, 2016

StefanKarpinski commented Aug 23, 2016

KristofferC commented Aug 23, 2016

jrevels commented Aug 24, 2016

call convert in Base.setfield! #16226

call convert in Base.setfield! #16226

Conversation

stevengj commented May 6, 2016

vtjnash commented May 6, 2016

JeffBezanson commented May 6, 2016

stevengj commented May 6, 2016

JeffBezanson commented May 6, 2016

stevengj commented May 6, 2016

JeffBezanson commented May 27, 2016

stevengj commented May 27, 2016

tkelman commented May 27, 2016 • edited Loading

stevengj May 27, 2016 • edited Loading

Choose a reason for hiding this comment

JeffBezanson May 27, 2016

Choose a reason for hiding this comment

vtjnash May 28, 2016

Choose a reason for hiding this comment

yuyichao May 28, 2016 • edited Loading

Choose a reason for hiding this comment

stevengj Aug 23, 2016

Choose a reason for hiding this comment

stevengj Aug 23, 2016

Choose a reason for hiding this comment

vtjnash Aug 23, 2016

Choose a reason for hiding this comment

stevengj Aug 23, 2016

Choose a reason for hiding this comment

vtjnash commented Jun 23, 2016

schmrlng commented Aug 23, 2016

stevengj commented Aug 23, 2016

tkelman commented Aug 23, 2016

nanosoldier commented Aug 23, 2016

stevengj commented Aug 23, 2016

KristofferC commented Aug 23, 2016

yuyichao commented Aug 23, 2016

StefanKarpinski commented Aug 23, 2016

KristofferC commented Aug 23, 2016

jrevels commented Aug 24, 2016

tkelman commented May 27, 2016 •

edited

Loading

stevengj May 27, 2016 •

edited

Loading

yuyichao May 28, 2016 •

edited

Loading