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

Conditionals waste assignments. #842

Open
gilch opened this issue Jul 15, 2015 · 9 comments
Open

Conditionals waste assignments. #842

gilch opened this issue Jul 15, 2015 · 9 comments

Comments

@gilch
Copy link
Member

gilch commented Jul 15, 2015

Conditionals containing statements are doing unnecessary work. I know, Python is not renowned for its performance, but let's not make it worse. This kind of thing can matter inside nested loops.

Currently a cond like this:

(cond [0 "zero"]
      [1 "one"]
      [True (assert 0)])

Turns into this:

if 0:
    _hy_anon_var_3 = 'zero'
else:
    if 1:
        _hy_anon_var_2 = 'one'
    else:
        if True:
            assert 0
            _hy_anon_var_1 = None
        else:
            _hy_anon_var_1 = None
        _hy_anon_var_2 = _hy_anon_var_1
    _hy_anon_var_3 = _hy_anon_var_2

But wouldn't it would work just as well like this?

if 0:
    _hy_anon_var_1 = 'zero'
elif 1:
    _hy_anon_var_1 = 'one'
elif True:
    assert 0
    _hy_anon_var_1 = None

Half the assignments for the same effect. And the hy --spy output is much more readable. I don't think CPython can automatically optimize this kind of thing. I know not all of them happen for any given branch, but the deeper ones will still do more assignments than they should.

Python has no switch/case. Sometimes you can get away with dictionary lookups instead, but if you need side effects or performance, you're supposed to use cascading elif where you would have used a switch.

Currently there's no elif form in Hy. cond is the closest we've got. Now I could write nested ifs instead (or a macro to do it for me) but look at what it turns into:

=> (if 0 "zero"
... (if 1 "one"
...       (assert 0)))
if 0:
    _hy_anon_var_2 = 'zero'
else:
    if 1:
        _hy_anon_var_1 = 'one'
    else:
        assert 0
        _hy_anon_var_1 = None
    _hy_anon_var_2 = _hy_anon_var_1

Not quite as bad as the cond since we have a final else, but still twice what is necessary:

if 0:
    _hy_anon_var_1 = 'zero'
elif 1:
    _hy_anon_var_1 = 'one'
else:
    assert 0
    _hy_anon_var_1 = None

Maybe it's too hard to optimize nested if forms like that? Is cond defined in terms of Hy's if form in the first place? I didn't implement cond but you could totally do it as a macro in terms of if.

Why not extend Hy's if to work like Arc Lisp's (as I proposed in #830 ), which can accept two or more arguments:

=> (if a b)
if a:
    _hy_anon_var_1 = b
else:
    _hy_anon_var_1 = None
=> (if a b
         c)
if a:
    _hy_anon_var_1 = b
else:
    _hy_anon_var_1 = c
=> (if a b
       c d)
if a:
    _hy_anon_var_1 = b
elif c:
    _hy_anon_var_1 = d
else:
    _hy_anon_var_1 = None
=> (if a b
       c d
         e)
if a:
    _hy_anon_var_1 = b
elif c:
    _hy_anon_var_1 = d
else:
    _hy_anon_var_1 = e

etc., for any number of inner elifs you need.

  • Now you only have to generate AST for the new if form, instead of optimizing nested forms.
  • It make Hy's if act more like Python's cascading elif.
  • This doesn't break existing Hy code, since we're not using more than 3 arguments now, and the 2 and 3 argument cases work exactly like before.
  • cond can be redefined in the same way, and get the same benefits.
@refi64
Copy link
Contributor

refi64 commented Jul 15, 2015

I'm toying with just fixing up ifs to avoid useless assignments.

@gilch
Copy link
Member Author

gilch commented Aug 7, 2017

I thought #920 was supposed to fix this, but it seems to be broken again. After that PR, we had

=> (if 1 1 (if 2 (setv x 2) (if 3 3)))
if 1:
    _hy_anon_var_1 = 1
elif 2:
    x = 2
    _hy_anon_var_1 = x
else:
    _hy_anon_var_1 = (3 if 3 else None)

But now, on master,

=> (if 1 1 (if 2 (setv x 2) (if 3 3)))
if 1:
    _hy_anon_var_2 = 1
else:
    if 2:
        x = 2
        _hy_anon_var_1 = None
    else:
        _hy_anon_var_1 = (3 if 3 else None)
    _hy_anon_var_2 = _hy_anon_var_1
_hy_anon_var_2
1

Though now we'd use => (if 1 1 2 (setv x 2) 3 3), which compiles exactly the same way.
This seems like a regression that our testing didn't catch. @kirbyfan64 do you know what happened here?

@gilch gilch reopened this Aug 7, 2017
@refi64
Copy link
Contributor

refi64 commented Aug 7, 2017

@gilch Seems like this was broken by #962 (in f4afb0c)...still investigating further.

@gilch
Copy link
Member Author

gilch commented Aug 7, 2017

That doesn't seem right. The equivalent cond has also regressed:

=> (cond [1 1] [2 (setv x 2)] [3 3])
if 1:
    _hy_anon_var_2 = 1
else:
    if 2:
        x = 2
        _hy_anon_var_1 = None
    else:
        _hy_anon_var_1 = (3 if 3 else None)
    _hy_anon_var_2 = _hy_anon_var_1
_hy_anon_var_2

But #962 didn't touch cond, nor the compiler--it just added a macro in bootstrap? Wait, no, it renamed if to if* in the compiler. If you were checking for an if, in the compiler, it should look for if* now.

@refi64
Copy link
Contributor

refi64 commented Aug 7, 2017

Actually, no. When the compiler looks at the code, it's unexpanded, so it needs to now be checking for if and if*.

@gilch
Copy link
Member Author

gilch commented Aug 7, 2017

Special-casing a macro name seems bad. It might be better to move if into the compiler. But then, how did cond work before? Maybe we could re-implement cond in terms of pure if* and if in terms of cond?

@refi64
Copy link
Contributor

refi64 commented Aug 7, 2017

AFAIK cond uses if, so it's just the same core issue.

IMO I wouldn't find moving if into the compiler. I was toying with a rewrite of if that used reductions instead of recursion, but it was kind of confusing and IMO wasn't worth the pain.

@gilch
Copy link
Member Author

gilch commented Aug 7, 2017

Or maybe we could rewrite the if macro to be non-recursive. That might be easier. But then what if the user wanted to implement his own branching macro based on if*? It won't be named if in that case. Maybe the compiler needs to expand macros first. Or maybe it's easier to just move it into the compiler. Hmm.

@Kodiologist
Copy link
Member

These days, if and cond use Python if expressions rather than statements when possible, which definitely cuts down on assignments, but the basic problem is still visible when you force the use of statements. For example,

(cond  p1 (setv x 1)  p2 (setv x 2)   p3 (setv x 3))

becomes

if p1:
    x = 1
    _hy_anon_var_5 = None
else:
    if p2:
        x = 2
        _hy_anon_var_4 = None
    else:
        if p3:
            x = 3
            _hy_anon_var_3 = None
        else:
            _hy_anon_var_3 = None
        _hy_anon_var_4 = _hy_anon_var_3
    _hy_anon_var_5 = _hy_anon_var_4
_hy_anon_var_5

We should perhaps use a single if… elif… else construct for this sort of case, too, for the sake of generating simpler code.

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

No branches or pull requests

4 participants