[PATCH] Do not enable optimizations by default to work-around pycparser issue

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Do not enable optimizations by default to work-around pycparser issue

Michał Górny-5
dev-python/pycparser-2.18+ exposes a design flaw in dev-python/ply that
makes it unable to work with -OO code. Remove the optimizations from
Portage shebangs to prevent triggering the issue until we find a proper
solution for it.

Bug: https://bugs.gentoo.org/628386
---
 bin/clean_locks   | 2 +-
 bin/dispatch-conf | 2 +-
 bin/ebuild        | 2 +-
 bin/emaint        | 2 +-
 bin/env-update    | 2 +-
 bin/portageq      | 2 +-
 tabcheck.py       | 2 +-
 7 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/bin/clean_locks b/bin/clean_locks
index 13af06197..fb245972f 100755
--- a/bin/clean_locks
+++ b/bin/clean_locks
@@ -1,4 +1,4 @@
-#!/usr/bin/python -bO
+#!/usr/bin/python -b
 # Copyright 1999-2014 Gentoo Foundation
 # Distributed under the terms of the GNU General Public License v2
 
diff --git a/bin/dispatch-conf b/bin/dispatch-conf
index 099c37f57..49e7774bf 100755
--- a/bin/dispatch-conf
+++ b/bin/dispatch-conf
@@ -1,4 +1,4 @@
-#!/usr/bin/python -bO
+#!/usr/bin/python -b
 # Copyright 1999-2017 Gentoo Foundation
 # Distributed under the terms of the GNU General Public License v2
 
diff --git a/bin/ebuild b/bin/ebuild
index 7930b41dd..bda746f78 100755
--- a/bin/ebuild
+++ b/bin/ebuild
@@ -1,4 +1,4 @@
-#!/usr/bin/python -bO
+#!/usr/bin/python -b
 # Copyright 1999-2015 Gentoo Foundation
 # Distributed under the terms of the GNU General Public License v2
 
diff --git a/bin/emaint b/bin/emaint
index a634c0ead..08e75851a 100755
--- a/bin/emaint
+++ b/bin/emaint
@@ -1,4 +1,4 @@
-#!/usr/bin/python -bO
+#!/usr/bin/python -b
 # Copyright 2005-2014 Gentoo Foundation
 # Distributed under the terms of the GNU General Public License v2
 
diff --git a/bin/env-update b/bin/env-update
index c43459bd7..03fc5849f 100755
--- a/bin/env-update
+++ b/bin/env-update
@@ -1,4 +1,4 @@
-#!/usr/bin/python -bO
+#!/usr/bin/python -b
 # Copyright 1999-2014 Gentoo Foundation
 # Distributed under the terms of the GNU General Public License v2
 
diff --git a/bin/portageq b/bin/portageq
index 06c8e0e57..0ac124fde 100755
--- a/bin/portageq
+++ b/bin/portageq
@@ -1,4 +1,4 @@
-#!/usr/bin/python -bO
+#!/usr/bin/python -b
 # Copyright 1999-2016 Gentoo Foundation
 # Distributed under the terms of the GNU General Public License v2
 
diff --git a/tabcheck.py b/tabcheck.py
index 2d45cdead..a28ad93d9 100755
--- a/tabcheck.py
+++ b/tabcheck.py
@@ -1,4 +1,4 @@
-#!/usr/bin/python -bO
+#!/usr/bin/python -b
 
 import tabnanny,sys
 
--
2.14.1


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Do not enable optimizations by default to work-around pycparser issue

Zac Medico-2
On 09/02/2017 10:46 AM, Michał Górny wrote:

> dev-python/pycparser-2.18+ exposes a design flaw in dev-python/ply that
> makes it unable to work with -OO code. Remove the optimizations from
> Portage shebangs to prevent triggering the issue until we find a proper
> solution for it.
>
> Bug: https://bugs.gentoo.org/628386
> ---
>  bin/clean_locks   | 2 +-
>  bin/dispatch-conf | 2 +-
>  bin/ebuild        | 2 +-
>  bin/emaint        | 2 +-
>  bin/env-update    | 2 +-
>  bin/portageq      | 2 +-
>  tabcheck.py       | 2 +-
>  7 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/bin/clean_locks b/bin/clean_locks
> index 13af06197..fb245972f 100755
> --- a/bin/clean_locks
> +++ b/bin/clean_locks
> @@ -1,4 +1,4 @@
> -#!/usr/bin/python -bO
> +#!/usr/bin/python -b

The diff shows -O, but the commit messages says -OO, so which one is it really?

Are we sure that it's worth it to load all of the __doc__ strings into
memory, given that we have alternative implementations for everything
that pycrypto provides?

For the record, I measure an increase of from 30248K to 31504K for -OO vs without when
importing all portage modules, tested as follows:

$ python3.6 -OO pym/portage/tests/runTests.py pym/portage/tests/lint/test_import_modules.py
testImportModules (portage.tests.lint.test_import_modules.ImportModulesTestCase) ... 30248

$ python3.6 -O pym/portage/tests/runTests.py pym/portage/tests/lint/test_import_modules.py
testImportModules (portage.tests.lint.test_import_modules.ImportModulesTestCase) ... 31468

$ python3.6  pym/portage/tests/runTests.py pym/portage/tests/lint/test_import_modules.py
testImportModules (portage.tests.lint.test_import_modules.ImportModulesTestCase) ... 31504

Using this patch:

diff --git a/pym/portage/tests/lint/test_import_modules.py b/pym/portage/tests/lint/test_import_modules.py
index fcdcb3b33..6350197eb 100644
--- a/pym/portage/tests/lint/test_import_modules.py
+++ b/pym/portage/tests/lint/test_import_modules.py
@@ -2,6 +2,7 @@
 # Distributed under the terms of the GNU General Public License v2
 
 from itertools import chain
+import resource
 
 from portage.const import PORTAGE_PYM_PATH, PORTAGE_PYM_PACKAGES
 from portage.tests import TestCase
@@ -24,6 +25,7 @@ class ImportModulesTestCase(TestCase):
                                if mod not in expected_failures:
                                        self.assertTrue(False, "failed to import '%s': %s" % (mod, e))
                                del e
+               print(resource.getrusage(resource.RUSAGE_SELF).ru_maxrss)
 
        def _iter_modules(self, base_dir):
                for parent, dirs, files in os.walk(base_dir):

--
Thanks,
Zac

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Do not enable optimizations by default to work-around pycparser issue

Zac Medico-2
On 09/02/2017 12:19 PM, Zac Medico wrote:

> On 09/02/2017 10:46 AM, Michał Górny wrote:
>> dev-python/pycparser-2.18+ exposes a design flaw in dev-python/ply that
>> makes it unable to work with -OO code. Remove the optimizations from
>> Portage shebangs to prevent triggering the issue until we find a proper
>> solution for it.
>>
>> Bug: https://bugs.gentoo.org/628386
>> ---
>>  bin/clean_locks   | 2 +-
>>  bin/dispatch-conf | 2 +-
>>  bin/ebuild        | 2 +-
>>  bin/emaint        | 2 +-
>>  bin/env-update    | 2 +-
>>  bin/portageq      | 2 +-
>>  tabcheck.py       | 2 +-
>>  7 files changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/bin/clean_locks b/bin/clean_locks
>> index 13af06197..fb245972f 100755
>> --- a/bin/clean_locks
>> +++ b/bin/clean_locks
>> @@ -1,4 +1,4 @@
>> -#!/usr/bin/python -bO
>> +#!/usr/bin/python -b
>
> The diff shows -O, but the commit messages says -OO, so which one is it really?
>
> Are we sure that it's worth it to load all of the __doc__ strings into
> memory, given that we have alternative implementations for everything
> that pycrypto provides?

Seems that we inadvertently dropped -O from the emerge shebang during
some refactoring here:

https://gitweb.gentoo.org/proj/portage.git/commit/?id=941dc5b26301075545e5fb3580f5d72c5f78c3cd
--
Thanks,
Zac

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Do not enable optimizations by default to work-around pycparser issue

Michał Górny-5
In reply to this post by Zac Medico-2
W dniu sob, 02.09.2017 o godzinie 12∶19 -0700, użytkownik Zac Medico
napisał:

> On 09/02/2017 10:46 AM, Michał Górny wrote:
> > dev-python/pycparser-2.18+ exposes a design flaw in dev-python/ply that
> > makes it unable to work with -OO code. Remove the optimizations from
> > Portage shebangs to prevent triggering the issue until we find a proper
> > solution for it.
> >
> > Bug: https://bugs.gentoo.org/628386
> > ---
> >  bin/clean_locks   | 2 +-
> >  bin/dispatch-conf | 2 +-
> >  bin/ebuild        | 2 +-
> >  bin/emaint        | 2 +-
> >  bin/env-update    | 2 +-
> >  bin/portageq      | 2 +-
> >  tabcheck.py       | 2 +-
> >  7 files changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/bin/clean_locks b/bin/clean_locks
> > index 13af06197..fb245972f 100755
> > --- a/bin/clean_locks
> > +++ b/bin/clean_locks
> > @@ -1,4 +1,4 @@
> > -#!/usr/bin/python -bO
> > +#!/usr/bin/python -b
>
> The diff shows -O, but the commit messages says -OO, so which one is it really?

Yes, it's a curious problem. -OO is the one breaking it but py<3.5 seems
to be happy to load -OO files with -O:

$ python2.7 -O -c 'import pycparser; print(pycparser.__file__)'
/usr/lib64/python2.7/site-packages/pycparser/c_parser.py:20: RuntimeWarning: parsing methods must have __doc__ for pycparser to work properly
  class CParser(PLYParser):
/usr/lib64/python2.7/site-packages/pycparser/__init__.pyo

$ python3.4 -O -c 'import pycparser; print(pycparser.__cached__)'
/usr/lib64/python3.4/site-packages/pycparser/c_parser.py:20: RuntimeWarning: parsing methods must have __doc__ for pycparser to work properly
  class CParser(PLYParser):
/usr/lib64/python3.4/site-packages/pycparser/__pycache__/__init__.cpython-34.pyo

This doesn't seem to be the case anymore for py3.5+:

$ python3.5 -O -c 'import pycparser; print(pycparser.__cached__)'
/usr/lib64/python3.5/site-packages/pycparser/__pycache__/__init__.cpython-35.opt-1.pyc

> Are we sure that it's worth it to load all of the __doc__ strings into
> memory, given that we have alternative implementations for everything
> that pycrypto provides?

I dare say that if -O can cause completely random total breakage, then
we shouldn't risk doing that for the sake of some minor memory savings.
It's not a case of 'support for pycryptodome' vs '1M memory saving'.
It's a case of 'loading random module can wreak total havoc in Portage'.

> For the record, I measure an increase of from 30248K to 31504K for -OO vs without when
> importing all portage modules, tested as follows:
>
> $ python3.6 -OO pym/portage/tests/runTests.py pym/portage/tests/lint/test_import_modules.py
> testImportModules (portage.tests.lint.test_import_modules.ImportModulesTestCase) ... 30248
>
> $ python3.6 -O pym/portage/tests/runTests.py pym/portage/tests/lint/test_import_modules.py
> testImportModules (portage.tests.lint.test_import_modules.ImportModulesTestCase) ... 31468
>
> $ python3.6  pym/portage/tests/runTests.py pym/portage/tests/lint/test_import_modules.py
> testImportModules (portage.tests.lint.test_import_modules.ImportModulesTestCase) ... 31504
>
> Using this patch:
>
> diff --git a/pym/portage/tests/lint/test_import_modules.py b/pym/portage/tests/lint/test_import_modules.py
> index fcdcb3b33..6350197eb 100644
> --- a/pym/portage/tests/lint/test_import_modules.py
> +++ b/pym/portage/tests/lint/test_import_modules.py
> @@ -2,6 +2,7 @@
>  # Distributed under the terms of the GNU General Public License v2
>  
>  from itertools import chain
> +import resource
>  
>  from portage.const import PORTAGE_PYM_PATH, PORTAGE_PYM_PACKAGES
>  from portage.tests import TestCase
> @@ -24,6 +25,7 @@ class ImportModulesTestCase(TestCase):
>                                 if mod not in expected_failures:
>                                         self.assertTrue(False, "failed to import '%s': %s" % (mod, e))
>                                 del e
> +               print(resource.getrusage(resource.RUSAGE_SELF).ru_maxrss)
>  
>         def _iter_modules(self, base_dir):
>                 for parent, dirs, files in os.walk(base_dir):
>

--
Best regards,
Michał Górny


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Do not enable optimizations by default to work-around pycparser issue

Zac Medico-2
On 09/02/2017 02:05 PM, Michał Górny wrote:

> W dniu sob, 02.09.2017 o godzinie 12∶19 -0700, użytkownik Zac Medico
> napisał:
>> On 09/02/2017 10:46 AM, Michał Górny wrote:
>>> dev-python/pycparser-2.18+ exposes a design flaw in dev-python/ply that
>>> makes it unable to work with -OO code. Remove the optimizations from
>>> Portage shebangs to prevent triggering the issue until we find a proper
>>> solution for it.
>>>
>>> Bug: https://bugs.gentoo.org/628386
>>> ---
>>>  bin/clean_locks   | 2 +-
>>>  bin/dispatch-conf | 2 +-
>>>  bin/ebuild        | 2 +-
>>>  bin/emaint        | 2 +-
>>>  bin/env-update    | 2 +-
>>>  bin/portageq      | 2 +-
>>>  tabcheck.py       | 2 +-
>>>  7 files changed, 7 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/bin/clean_locks b/bin/clean_locks
>>> index 13af06197..fb245972f 100755
>>> --- a/bin/clean_locks
>>> +++ b/bin/clean_locks
>>> @@ -1,4 +1,4 @@
>>> -#!/usr/bin/python -bO
>>> +#!/usr/bin/python -b
>>
>> The diff shows -O, but the commit messages says -OO, so which one is it really?
>
> Yes, it's a curious problem. -OO is the one breaking it but py<3.5 seems
> to be happy to load -OO files with -O:
>
> $ python2.7 -O -c 'import pycparser; print(pycparser.__file__)'
> /usr/lib64/python2.7/site-packages/pycparser/c_parser.py:20: RuntimeWarning: parsing methods must have __doc__ for pycparser to work properly
>   class CParser(PLYParser):
> /usr/lib64/python2.7/site-packages/pycparser/__init__.pyo
>
> $ python3.4 -O -c 'import pycparser; print(pycparser.__cached__)'
> /usr/lib64/python3.4/site-packages/pycparser/c_parser.py:20: RuntimeWarning: parsing methods must have __doc__ for pycparser to work properly
>   class CParser(PLYParser):
> /usr/lib64/python3.4/site-packages/pycparser/__pycache__/__init__.cpython-34.pyo
>
> This doesn't seem to be the case anymore for py3.5+:
>
> $ python3.5 -O -c 'import pycparser; print(pycparser.__cached__)'
> /usr/lib64/python3.5/site-packages/pycparser/__pycache__/__init__.cpython-35.opt-1.pyc
>
>> Are we sure that it's worth it to load all of the __doc__ strings into
>> memory, given that we have alternative implementations for everything
>> that pycrypto provides?
>
> I dare say that if -O can cause completely random total breakage, then
> we shouldn't risk doing that for the sake of some minor memory savings.
> It's not a case of 'support for pycryptodome' vs '1M memory saving'.
> It's a case of 'loading random module can wreak total havoc in Portage'.

Seems reasonable. Please merge.
--
Thanks,
Zac

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Do not enable optimizations by default to work-around pycparser issue

Michał Górny-5
In reply to this post by Michał Górny-5
W dniu sob, 02.09.2017 o godzinie 23∶05 +0200, użytkownik Michał Górny
napisał:

> W dniu sob, 02.09.2017 o godzinie 12∶19 -0700, użytkownik Zac Medico
> napisał:
> > On 09/02/2017 10:46 AM, Michał Górny wrote:
> > > dev-python/pycparser-2.18+ exposes a design flaw in dev-python/ply that
> > > makes it unable to work with -OO code. Remove the optimizations from
> > > Portage shebangs to prevent triggering the issue until we find a proper
> > > solution for it.
> > >
> > > Bug: https://bugs.gentoo.org/628386
> > > ---
> > >  bin/clean_locks   | 2 +-
> > >  bin/dispatch-conf | 2 +-
> > >  bin/ebuild        | 2 +-
> > >  bin/emaint        | 2 +-
> > >  bin/env-update    | 2 +-
> > >  bin/portageq      | 2 +-
> > >  tabcheck.py       | 2 +-
> > >  7 files changed, 7 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/bin/clean_locks b/bin/clean_locks
> > > index 13af06197..fb245972f 100755
> > > --- a/bin/clean_locks
> > > +++ b/bin/clean_locks
> > > @@ -1,4 +1,4 @@
> > > -#!/usr/bin/python -bO
> > > +#!/usr/bin/python -b
> >
> > The diff shows -O, but the commit messages says -OO, so which one is it really?
>
> Yes, it's a curious problem. -OO is the one breaking it but py<3.5 seems
> to be happy to load -OO files with -O:
>
> $ python2.7 -O -c 'import pycparser; print(pycparser.__file__)'
> /usr/lib64/python2.7/site-packages/pycparser/c_parser.py:20: RuntimeWarning: parsing methods must have __doc__ for pycparser to work properly
>   class CParser(PLYParser):
> /usr/lib64/python2.7/site-packages/pycparser/__init__.pyo
>
> $ python3.4 -O -c 'import pycparser; print(pycparser.__cached__)'
> /usr/lib64/python3.4/site-packages/pycparser/c_parser.py:20: RuntimeWarning: parsing methods must have __doc__ for pycparser to work properly
>   class CParser(PLYParser):
> /usr/lib64/python3.4/site-packages/pycparser/__pycache__/__init__.cpython-34.pyo
>
> This doesn't seem to be the case anymore for py3.5+:
>
> $ python3.5 -O -c 'import pycparser; print(pycparser.__cached__)'
> /usr/lib64/python3.5/site-packages/pycparser/__pycache__/__init__.cpython-35.opt-1.pyc

My roundabout thinking here was really silly. I was mistaking -O/-OO for
.pyc/.pyo. For the record, the correct explanation:

Python < 3.5 supports only either -O or -OO, and compiles both levels to
the same .pyo files. So if we compiled the relevant packages with -OO,
then -O implicitly meant -OO.

--
Best regards,
Michał Górny


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Do not enable optimizations by default to work-around pycparser issue

Michał Górny-5
In reply to this post by Zac Medico-2
W dniu sob, 02.09.2017 o godzinie 14∶23 -0700, użytkownik Zac Medico
napisał:

> On 09/02/2017 02:05 PM, Michał Górny wrote:
> > W dniu sob, 02.09.2017 o godzinie 12∶19 -0700, użytkownik Zac Medico
> > napisał:
> > > On 09/02/2017 10:46 AM, Michał Górny wrote:
> > > > dev-python/pycparser-2.18+ exposes a design flaw in dev-python/ply that
> > > > makes it unable to work with -OO code. Remove the optimizations from
> > > > Portage shebangs to prevent triggering the issue until we find a proper
> > > > solution for it.
> > > >
> > > > Bug: https://bugs.gentoo.org/628386
> > > > ---
> > > >  bin/clean_locks   | 2 +-
> > > >  bin/dispatch-conf | 2 +-
> > > >  bin/ebuild        | 2 +-
> > > >  bin/emaint        | 2 +-
> > > >  bin/env-update    | 2 +-
> > > >  bin/portageq      | 2 +-
> > > >  tabcheck.py       | 2 +-
> > > >  7 files changed, 7 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/bin/clean_locks b/bin/clean_locks
> > > > index 13af06197..fb245972f 100755
> > > > --- a/bin/clean_locks
> > > > +++ b/bin/clean_locks
> > > > @@ -1,4 +1,4 @@
> > > > -#!/usr/bin/python -bO
> > > > +#!/usr/bin/python -b
> > >
> > > The diff shows -O, but the commit messages says -OO, so which one is it really?
> >
> > Yes, it's a curious problem. -OO is the one breaking it but py<3.5 seems
> > to be happy to load -OO files with -O:
> >
> > $ python2.7 -O -c 'import pycparser; print(pycparser.__file__)'
> > /usr/lib64/python2.7/site-packages/pycparser/c_parser.py:20: RuntimeWarning: parsing methods must have __doc__ for pycparser to work properly
> >   class CParser(PLYParser):
> > /usr/lib64/python2.7/site-packages/pycparser/__init__.pyo
> >
> > $ python3.4 -O -c 'import pycparser; print(pycparser.__cached__)'
> > /usr/lib64/python3.4/site-packages/pycparser/c_parser.py:20: RuntimeWarning: parsing methods must have __doc__ for pycparser to work properly
> >   class CParser(PLYParser):
> > /usr/lib64/python3.4/site-packages/pycparser/__pycache__/__init__.cpython-34.pyo
> >
> > This doesn't seem to be the case anymore for py3.5+:
> >
> > $ python3.5 -O -c 'import pycparser; print(pycparser.__cached__)'
> > /usr/lib64/python3.5/site-packages/pycparser/__pycache__/__init__.cpython-35.opt-1.pyc
> >
> > > Are we sure that it's worth it to load all of the __doc__ strings into
> > > memory, given that we have alternative implementations for everything
> > > that pycrypto provides?
> >
> > I dare say that if -O can cause completely random total breakage, then
> > we shouldn't risk doing that for the sake of some minor memory savings.
> > It's not a case of 'support for pycryptodome' vs '1M memory saving'.
> > It's a case of 'loading random module can wreak total havoc in Portage'.
>
> Seems reasonable. Please merge.

Merged, thanks.
--
Best regards,
Michał Górny