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

got dot access working \o/ #3

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

got dot access working \o/ #3

wants to merge 11 commits into from

Conversation

amanica
Copy link
Collaborator

@amanica amanica commented May 21, 2019

Hi,
I've not tested this much but it looks like I got the dot method access working eg.:
@test s1.fun1(1, 2, z=3) == 6

let me know what you think :)

@ipod825
Copy link
Owner

ipod825 commented May 22, 2019

This is nice! But I wonder how much overhead that costs. Because we might add a lot of branching in an instance function where you might keep referring to self.something. It would be nice to check its overhead in (1) field reference (2) lightweight functions and (3) heavy duty functions compared with the running time without this getproperty overloading implementation.

If there's an obvious overhead, then we should turn off this behaviour by default and have a mechanism to enable it (either by adding a function enableObjFnCall in the module or add an optional argument to the @class macro).

@amanica
Copy link
Collaborator Author

amanica commented May 22, 2019

This is nice! But I wonder how much overhead that costs. Because we might add a lot of branching in an instance function where you might keep referring to self.something.

From what I read it should be fine:
https://discourse.julialang.org/t/how-to-use-getproperty-setproperty/16033/3

It would be nice to check its overhead in

I agree, I'll write some checks and let you know. (I made it optional so-long)

@ipod825
Copy link
Owner

ipod825 commented May 24, 2019

Try catch should reduce the overhead

                  try
                    getfield(self, name)
                  catch
                    (args...; kwargs...)->eval(:(\$name(\$self, \$args...; \$kwargs...)))
                  end

But it's still a large overhead.

@ipod825
Copy link
Owner

ipod825 commented May 24, 2019

Also, instead of having an additional args to @class, a function in the module that switches on/off a global variable would be a better idea as the user don't need to change every class definition in case he/she wants to switch between two mode (though in any case, the function calling part still requires change).

amanica added 3 commits May 26, 2019 22:18
…es slower :'(

TrialJudgement(+4188.17% => regression)
"NoDotOpr" => 1-element BenchmarkTools.BenchmarkGroup:
    tags: []
    "get_field1" => TrialEstimate(3.921 ns)
"DotOpr" => 1-element BenchmarkTools.BenchmarkGroup:
    tags: []
    "get_field1" => TrialEstimate(3.921 ns)
TrialJudgement(+0.00% => invariant)

bdo = BasicDotOpr(1) = BasicDotOpr(1)
  0.023392 seconds (14.88 k allocations: 793.028 KiB)
  0.000007 seconds (4 allocations: 160 bytes)
  0.000006 seconds (4 allocations: 160 bytes)
bndo = BasicNoDotOpr(1) = BasicNoDotOpr(1)
  0.000006 seconds (4 allocations: 160 bytes)
  0.000003 seconds (4 allocations: 160 bytes)
  0.000003 seconds (4 allocations: 160 bytes)
@ipod825
Copy link
Owner

ipod825 commented May 31, 2019

isdefined seems to trigger the inline compilation optimation. That's nice. However, it doesn't seem to help for function calls. Changing the test case to:

bg["DotOpr"]["get_field1"] = @benchmarkable o.fun1(1) setup=(o = B1DotOpr(rand(Int)))
bg["NoDotOpr"]["get_field1"] = @benchmarkable fun1(o, 1) setup=(o = B1NoDotOpr(rand(Int)))

We still get a very large latency. In fact, I doubt that this is not even caused by the branch, as if I changed dotAccStr to:

    dotAccStr = """
        function Base.getproperty(self::$clsName, name::Symbol)
            (args...; kwargs...)->eval(:(\$name(\$self, \$args...; \$kwargs...)))                                         
        end
        """

making dot operator works only for function calls, I got pretty much the same latency on the modified test case shown above, indicating that it's the creation of the unnamed function that causes the great latency. Unless we have some remedy to ask the compiler not to recompile an unnamed function every time, I don't think there is a solution to this problem. Caching the unnamed function in OOPMacro module space might be a direction to go.

end
"""
function Base.getproperty(self::$clsName, name::Symbol)
if isdefined(self, name) || name ∉ ($clsFnNameList)
Copy link
Owner

@ipod825 ipod825 May 31, 2019

Choose a reason for hiding this comment

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

Why do we nee clsFnNameList here? I think when isdefined(self, name) is false, the best we could do is to try to call the function, so why do we still need to check if the name in the function list?

@amanica
Copy link
Collaborator Author

amanica commented May 31, 2019

Cool I'll look at the function call next :)

I check if in clsFnNameList so that if it is not in our list I want it to default to normal behaviour (is there a better way to check?). This gives a consistent error for myClass.invalidfield and maybe there are other dot operation use cases I have not thought about.

if isdefined(self, name) || name ∉ ($clsFnNameList)
getfield(self, name)
function Base.getproperty(self::$clsName, nameSymbol::Symbol)
if isdefined(self, nameSymbol) || nameSymbol ∉ ($clsFnNameList)
Copy link
Owner

Choose a reason for hiding this comment

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

nameSymbol ∉ ($clsFnNameList) should slow function calls down.

else
(args...; kwargs...)->eval(:(\$name(\$self, \$args...; \$kwargs...)))
if haskey($(OOPMacroModule).clsFnDefDict, self)
Copy link
Owner

@ipod825 ipod825 Jun 4, 2019

Choose a reason for hiding this comment

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

Instead of caching the functions on the fly inside getproperty, try cache them outside. Avoiding branch should always help.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure I understand. Maybe we can initialize it somehow in the constructor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants