Skip to content

Conversation

@drbassett
Copy link

I was recently tripped up by a performance problem with a custom implementation of getproperty. In summary, the problem is that using the dot operator within getproperty invokes getproperty recursively, which complicates the job of the type inference system in Julia, and causes type-instability. See the issue I posted to discourse for more details: https://discourse.julialang.org/t/type-inference-problem-with-getproperty/54585.

Using the dot operator is the obvious way to implement getproperty, especially considering the example in the official documentation does so. I think many other users will encounter this same problem, and many of them will be unaware of it. I am guessing it will be difficult to improve the optimizer to handle recursive getproperty calls more intelligently, so instead I propose adding a documentation blurb to warn users about this problem, and how to work around it, which this PR does.

@timholy
Copy link
Member

timholy commented Feb 7, 2021

The only reservation I have about merging this is whether the inference problem can instead be fixed. Has it been reported separately?

If this is hard to fix, I'd suggest we also change the example in the docstring to use getfield everywhere.

@kshyatt kshyatt added the docs This change adds or pertains to documentation label Feb 7, 2021
@drbassett
Copy link
Author

drbassett commented Feb 7, 2021

Agreed that it would be better to fix the problem than to just document it. As far as I can tell, there does not seem to be an existing issue for this problem: https://github.com/JuliaLang/julia/issues?q=is%3Aissue+is%3Aopen+getproperty+in%3Atitle. I will create one to see if I can get a discussion going.

EDIT Issue is here: #39563

`getproperty` recursively, which hampers type inference and can cause
[type-instability](@ref man-type-stability). To minimize the risk of type
instability, implement `getproperty` in terms of [`getfield`](@ref). For
example, replace `a.b` with `getfield(a, :b)`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
example, replace `a.b` with `getfield(a, :b)`.
example, replace `a.b` with `getfield(a, :b)` in the definition of `getproperty` for your type.

@vtjnash
Copy link
Member

vtjnash commented Feb 8, 2021

It could be fixed. Is it common enough though to warrant changing? The example already shows how you would typically call getfield in your definition, since the expectation is that if you are overloading getproperty, it is because you do not want it to just forward to getfield, so you need to use the "inner" API anyways.

@JeffBezanson
Copy link
Member

I'm not a big fan of enshrining "folk wisdom" about how the compiler works in documentation --- things can easily change and we'll never remember to update it. I think it's enough just to recommend getfield in general for implementing getproperty.

@vtjnash
Copy link
Member

vtjnash commented Apr 16, 2021

I think we now allow some recursion here (though infinite recursion may always be tricky). The issues posted in that discourse seem to be gone anyways.

@vtjnash vtjnash closed this Apr 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs This change adds or pertains to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants