Skip to content

Commit

Permalink
RFC: Also change lowering of getproperty
Browse files Browse the repository at this point in the history
So after thinking about #37268, I was wondering what would happen
if one applied the same trick to the lowering of `getproperty`
as well. This PR starts at that, but is mostly a placeholder for
discussion of whether this is something we want to do. In particular,
this PR lowers `a.b` to:

```
getproperty(a)(a, :b)
```

with a default implementation of

```
getproperty(a) = getfield
```

The inference benefit is as expected. Timing end-to-end inference&optimize
time of:

```
struct Foo; end
f(a::Foo) = a.x
```

shows that time to infer `f` drops by about a third (because it doesn't need to
infer getproperty twice). Overall build time of the system image drops
by 6% for me.

If we do decide to go this way, things that would still need to be fixed:

- [] Do the same for `setproperty!`
- [] `ccall` doesn't like this lowering. Didn't look into it too closely
- [] Needs a fallback to backwards compatibility. I'm thinking something
along the lines of

```
getproperty(x) = hasmethod(getproperty, Tuple{typeof(x), Symbol}) ? getproperty : getfield
```

but obviously it would need inference integration to be fast.
  • Loading branch information
Keno committed Aug 30, 2020
1 parent 554f57e commit dd49715
Show file tree
Hide file tree
Showing 9 changed files with 24 additions and 12 deletions.
6 changes: 4 additions & 2 deletions base/Base.jl
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ include(path::String) = include(Base, path)
const is_primary_base_module = ccall(:jl_module_parent, Ref{Module}, (Any,), Base) === Core.Main
ccall(:jl_set_istopmod, Cvoid, (Any, Bool), Base, is_primary_base_module)

getproperty(x) = getfield

# Try to help prevent users from shooting them-selves in the foot
# with ambiguities by defining a few common and critical operations
# (and these don't need the extra convert code)
Expand Down Expand Up @@ -317,8 +319,8 @@ include("hashing2.jl")

# irrational mathematical constants
include("irrationals.jl")
include("mathconstants.jl")
using .MathConstants: ℯ, π, pi
#include("mathconstants.jl")
#using .MathConstants: ℯ, π, pi

# metaprogramming
include("meta.jl")
Expand Down
3 changes: 2 additions & 1 deletion base/boot.jl
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,6 @@ export
# constants
nothing, Main

const getproperty = getfield
const setproperty! = setfield!

abstract type Number end
Expand Down Expand Up @@ -226,6 +225,8 @@ ccall(:jl_toplevel_eval_in, Any, (Any, Any),
(f::typeof(Typeof))(x) = ($(_expr(:meta,:nospecialize,:x)); isa(x,Type) ? Type{x} : typeof(x))
end)

getproperty(x) = getfield

# let the compiler assume that calling Union{} as a constructor does not need
# to be considered ever (which comes up often as Type{<:T})
Union{}(a...) = throw(MethodError(Union{}, a))
Expand Down
2 changes: 1 addition & 1 deletion base/compiler/compiler.jl
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import Core: print, println, show, write, unsafe_write, stdout, stderr,
_apply, _apply_iterate, svec, apply_type, Builtin, IntrinsicFunction,
MethodInstance, CodeInstance, MethodMatch

const getproperty = Core.getfield
getproperty(x) = Core.getfield
const setproperty! = Core.setfield!

ccall(:jl_set_istopmod, Cvoid, (Any, Bool), Compiler, false)
Expand Down
2 changes: 1 addition & 1 deletion base/io.jl
Original file line number Diff line number Diff line change
Expand Up @@ -346,7 +346,7 @@ If `pipe isa AbstractPipe`, it must obey the following interface:
"""
abstract type AbstractPipe <: IO end

function getproperty(pipe::AbstractPipe, name::Symbol)
getproperty(pipe::AbstractPipe) = (pipe::AbstractPipe, name::Symbol)->begin
if name === :in || name === :in_stream || name === :out || name === :out_stream ||
name === :err || name === :err_stream
return getfield(pipe, name)::IO
Expand Down
4 changes: 2 additions & 2 deletions base/stream.jl
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ If `server isa LibuvServer`, it must obey the following interface:
"""
abstract type LibuvServer <: IOServer end

function getproperty(server::LibuvServer, name::Symbol)
getproperty(server::LibuvServer) = (server::LibuvServer, name::Symbol)->begin
if name === :handle
return getfield(server, :handle)::Ptr{Cvoid}
elseif name === :status
Expand Down Expand Up @@ -55,7 +55,7 @@ If`stream isa LibuvStream`, it must obey the following interface:
"""
abstract type LibuvStream <: IO end

function getproperty(stream::LibuvStream, name::Symbol)
getproperty(server::LibuvStream) = (stream::LibuvStream, name::Symbol)->begin
if name === :handle
return getfield(stream, :handle)::Ptr{Cvoid}
elseif name === :status
Expand Down
3 changes: 2 additions & 1 deletion base/task.jl
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,8 @@ const task_state_runnable = UInt8(0)
const task_state_done = UInt8(1)
const task_state_failed = UInt8(2)

@inline function getproperty(t::Task, field::Symbol)
getproperty(t::Task) = (t::Task, field::Symbol)->begin
@_inline_meta
if field === :state
# TODO: this field name should be deprecated in 2.0
st = getfield(t, :_state)
Expand Down
7 changes: 6 additions & 1 deletion src/julia-syntax.scm
Original file line number Diff line number Diff line change
Expand Up @@ -1765,7 +1765,12 @@
(if (and (pair? e) (eq? (car e) '|.|))
(let ((f (cadr e)) (x (caddr e)))
(cond ((or (atom? x) (eq? (car x) 'quote) (eq? (car x) 'inert) (eq? (car x) '$))
`(call (top getproperty) ,f ,x))
(if (symbol-like? f)
`(call (call (top getproperty) ,f) ,f ,x)
(let ((ff (make-ssavalue)))
`(block
(= ,ff ,f)
(call (call (top getproperty) ,ff) ,ff ,x)))))
((eq? (car x) 'tuple)
(if (and (eq? (identifier-name f) '^) (length= x 3) (integer? (caddr x)))
(make-fuse '(top literal_pow)
Expand Down
6 changes: 4 additions & 2 deletions stdlib/Random/src/generation.jl
Original file line number Diff line number Diff line change
Expand Up @@ -76,12 +76,14 @@ function _rand(rng::AbstractRNG, sp::SamplerBigFloat, ::CloseOpen12{BigFloat})
z
end

const RoundingMode = Base.MPFR.MPFRRoundingMode

function _rand(rng::AbstractRNG, sp::SamplerBigFloat, ::CloseOpen01{BigFloat})
z, randbool = _rand(rng, sp)
z.exp = 0
randbool &&
ccall((:mpfr_sub_d, :libmpfr), Int32,
(Ref{BigFloat}, Ref{BigFloat}, Cdouble, Base.MPFR.MPFRRoundingMode),
(Ref{BigFloat}, Ref{BigFloat}, Cdouble, RoundingMode),
z, z, 0.5, Base.MPFR.ROUNDING_MODE[])
z
end
Expand All @@ -90,7 +92,7 @@ end
# TODO: make an API for requesting full or not-full precision
function _rand(rng::AbstractRNG, sp::SamplerBigFloat, ::CloseOpen01{BigFloat}, ::Nothing)
z = _rand(rng, sp, CloseOpen12(BigFloat))
ccall((:mpfr_sub_ui, :libmpfr), Int32, (Ref{BigFloat}, Ref{BigFloat}, Culong, Base.MPFR.MPFRRoundingMode),
ccall((:mpfr_sub_ui, :libmpfr), Int32, (Ref{BigFloat}, Ref{BigFloat}, Culong, RoundingMode),
z, z, 1, Base.MPFR.ROUNDING_MODE[])
z
end
Expand Down
3 changes: 2 additions & 1 deletion stdlib/Random/src/normal.jl
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,8 @@ end

### complex randn

Base.@irrational SQRT_HALF 0.7071067811865475244008 sqrt(big(0.5))
#Base.@irrational SQRT_HALF 0.7071067811865475244008 sqrt(big(0.5))
const SQRT_HALF = 0.7071067811865475244008

randn(rng::AbstractRNG, ::Type{Complex{T}}) where {T<:AbstractFloat} =
Complex{T}(SQRT_HALF * randn(rng, T), SQRT_HALF * randn(rng, T))
Expand Down

0 comments on commit dd49715

Please sign in to comment.