[issue26204] compiler: don't emit LOAD_CONST instructions for constant statements?

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

LGTM.

It looks to me that this optimization was added to avoid spending executing 
time for docstrings. Other cases almost never occur in real code and are not 
worth to be optimized. But the patch makes the code cleaner (it would even more 
cleaner if collapse all kinds of constants in Constant).

--
nosy: +serhiy.storchaka
stage:  -> commit review

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26204] compiler: don't emit LOAD_CONST instructions for constant statements?

[STINNER Victor]

STINNER Victor added the comment:

Serhiy: "It looks to me that this optimization was added to avoid spending 
executing time for docstrings."

Hum, let me dig Mercurial history.

changeset:   39364:8ef3f8cf90e1
branch:  legacy-trunk
user:Neal Norwitz 
date:Fri Aug 04 05:09:28 2006 +
files:   Lib/test/test_code.py Misc/NEWS Python/compile.c
description:
Bug #1333982: string/number constants were inappropriately stored
in the byte code and co_consts even if they were not used, ie
immediately popped off the stack.
---

https://bugs.python.org/issue1333982#msg26659:
"For reference, an optimization that got lost: (...) 'a' is the docstring, but 
the 'b' previously did not show up anywhere in the code object.  Now there is 
the LOAD_CONST/POP_TOP pair."

Ah, it was a regression introduced by the new AST compiler. But the change 
introduced a new optimization: numbers are now also ignored. In Python 2.4 
(before AST), numbers were not ignored:
---
>>> def f():
...  "a"
...  1
...  "b"
... 

>>> import dis; dis.dis(f)
  3   0 LOAD_CONST   1 (1)
  3 POP_TOP 
  4 LOAD_CONST   2 (None)
  7 RETURN_VALUE
---


If we continue to dig deeper, before AST, I found:
---
changeset:   4991:8276916e1ea8
branch:  legacy-trunk
user:Guido van Rossum 
date:Fri Jan 17 21:04:03 1997 +
files:   Python/compile.c
description:
Add co_stacksize field to codeobject structure, and stacksize argument
to PyCode_New() argument list.  Move MAXBLOCKS constant to conpile.h.

Added accurate calculation of the actual stack size needed by the
generated code.

Also commented out all fprintf statements (except for a new one to
diagnose stack underflow, and one in #ifdef'ed out code), and added
some new TO DO suggestions (now that the stacksize is taken of the TO
DO list).
---

This patch added the following code to com_expr_stmt() in Python/compile.c:

+   /* Forget it if we have just a doc string here */
+   if (NCH(n) == 1 && get_rawdocstring(n) != NULL)
+   return;

I'm unable to find the exact part of the compiler which ignores strings in 
statement expressions.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26204] compiler: don't emit LOAD_CONST instructions for constant statements?

[STINNER Victor]

STINNER Victor added the comment:

"""
Will the following code compile OK?  What will it compile to?

   if 1:
  42
"""

compile OK: what do you mean? This code doesn't make any sense to me :-)


Currently text strings statements are ignored:
---
>>> def f():
...  if 1:
...   "abc"
... 
>>> import dis
>>> dis.dis(f)
  3   0 LOAD_CONST   0 (None)
  3 RETURN_VALUE
---


But byte strings emit a LOAD_CONST+POP_TOP:
---
>>> def g():
...  if 1:
...   b'bytes'
... 
>>> dis.dis(g)
  3   0 LOAD_CONST   1 (b'bytes')
  3 POP_TOP
  4 LOAD_CONST   0 (None)
  7 RETURN_VALUE
---


With my patch, all constants statements will be ignored. Example with my patch:
---
>>> def h():
...  if 1:
...   b'bytes'
... 
>>> import dis; dis.dis(h)
  3   0 LOAD_CONST   0 (None)
  3 RETURN_VALUE
---

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26204] compiler: don't emit LOAD_CONST instructions for constant statements?

[STINNER Victor]

STINNER Victor added the comment:

Serhiy: "It looks to me that this optimization was added to avoid spending 
executing time for docstrings. Other cases almost never occur in real code and 
are not worth to be optimized. But the patch makes the code cleaner (it would 
even more cleaner if collapse all kinds of constants in Constant)."

Oh, I don't really care of performance. The bytecode just doesn't make any 
sense to me. I don't understand why we load a constant.

Maybe the compiler should emit a warning to say that the code doesn't make 
sense at all and is ignored?

Example with GCC:

$ cat x.c 
int main()
{
1;
}


$ gcc x.c -Wall -o x
x.c: In function 'main':
x.c:3:5: warning: statement with no effect [-Wunused-value]
 1;
 ^

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26204] compiler: don't emit LOAD_CONST instructions for constant statements?

[STINNER Victor]

STINNER Victor added the comment:

> Maybe the compiler should emit a warning to say that the code doesn't make 
> sense at all and is ignored?

Oh ok, now I recall a similar issue that I posted 3 years ago: issue #17516, 
"Dead code should be removed".

Example of suspicious code:

def func():
   func2(),

func() calls func2() and then create a tuple of 1 item with the func2() result. 
See my changeset 33bdd0a985b9 for examples in the Python source code. The 
parser or compiler should maybe help to developer to catch such strange code :-)

In some cases, the code really contains code explicitly dead:

def test_func():
   return
   do_real_stuff()

do_real_stuff() is never called. Maybe it was a deliberate choice, maybe it was 
a mistake in a very old code base, bug introduced after multiple refactoring, 
and high turn over in a team? Again, we should emit a warning on such case?

Hum, these warnings have a larger scope than this specific issue (don't emit 
LOAD_CONST for constants in expressions).

See also the related thread on python-dev for the specific case of triple 
quoted strings ("""string"""): 
https://mail.python.org/pipermail/python-dev/2013-March/124925.html

It was admitted that it's a convenient way to insert a comment and it must not 
emit a warning (at least, not by default?)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26204] compiler: don't emit LOAD_CONST instructions for constant statements?

[Yury Selivanov]

Yury Selivanov added the comment:

The patch looks alright.

Will the following code compile OK?  What will it compile to?

   if 1:
  42

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26204] compiler: don't emit LOAD_CONST instructions for constant statements?

[STINNER Victor]

Changes by STINNER Victor :


--
title: compiler: don't emit LOAD_CONST instructions for constant expressions? 
-> compiler: don't emit LOAD_CONST instructions for constant statements?

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com