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

Investigate cython #52

Closed
jamadden opened this issue Apr 15, 2018 · 9 comments
Closed

Investigate cython #52

jamadden opened this issue Apr 15, 2018 · 9 comments

Comments

@jamadden
Copy link
Member

I’ve learned a lot about cythonizing pure-Python code during the release of gevent 1.3. It can produce dramatics speed ups on CPython while still being compatible with PyPy and debugging (depending on the bottlenecks, of course). That should be investigated here with respect to #36 to see how it works out and if there are low-hanging fruits.

@jamadden
Copy link
Member Author

jamadden commented Jun 26, 2018

Initial results are very promising. Without doing any Cython specific tuning (which can improve speed dramatically, depending on the situation), CPython 2.7 sees gains of around 10-30%:

Benchmark 27_plain 27_cython
Construct Singleton 198 ns 118 ns: 1.68x faster (-40%)
Construct non-Singleton 202 ns 195 ns: 1.03x faster (-3%)
Construct Singleton with args 213 ns 140 ns: 1.51x faster (-34%)
.bm_simple_iface: toExternalObject 65.4 us 51.5 us: 1.27x faster (-21%)
.bm_simple_iface: fromExternalObject 109 us 96.5 us: 1.13x faster (-11%)
.bm_simple_registered_class: toExternalObject 52.9 us 40.2 us: 1.31x faster (-24%)
.bm_simple_registered_class: fromExternalObject 20.4 us 14.5 us: 1.41x faster (-29%)

Not significant (1): Construct non-Singleton with args

In this case, the 'non-Singleton' benchmarks are our baseline, so they show a 0-3% variance between the test runs as our margin of error.

Of particular note: the two Singleton tests construct objects that descend from Singleton, which is compiled with Cython. Those are now actually faster than constructing a type that descends from object. This is code starting from pr #53 (issue #48), so it had some speedups already, but I wonder what would happen with the old code...Cython may make #53 obsolete.

BTW, that was with doing just this in setup.py (not the real solution we want, but it shows how easy; I also had to move one interface definition into interfaces.py, fix a bug in zcml.py and tweak the use of getargspec in internalization.py, see #30):

setup(
    ...
    ext_modules=cythonize(
        'src/nti/externalization/*.py',
        # zope.interface.Interface subclasses cannot be cython
        # compiled
        exclude=['src/nti/externalization/interfaces.py'] ),
)

@jamadden
Copy link
Member Author

We have some uses of zope.deprecation and zope.deferredimport for BWC. Those aren't compatible with extension modules (they use Python frame tricks). With some effort we could refactor the project so that we can still provide those BWC aliases, but I'd prefer just to drop them, they've been deprecated/moved for quite awhile. Thoughts @papachoco and @jzuech3 ?

@papachoco
Copy link
Contributor

It's OK w/ me. I think there are some still references in the OUBound projects to deprecated code; but those can be fixed easily.

@papachoco
Copy link
Contributor

The platform code should be upto date

@cutz
Copy link

cutz commented Jun 26, 2018

cc @hagenrd

@jamadden
Copy link
Member Author

datastructures.py would be losing this code:

-zope.deferredimport.deprecatedFrom(
-    "Moved to nti.externalization.interfaces",
-    "nti.externalization.interfaces",
-    "IExternalObject",
-    "LocatedExternalDict",
-    "LocatedExternalList",
-    "ILocatedExternalMapping",
-    "ILocatedExternalSequence")

externalization.py would be losing this:

-# Things that have moved
-
-zope.deferredimport.initialize()
-zope.deferredimport.deprecatedFrom(
-    "Import from .persistence",
-    "nti.externalization.persistence",
-    "NoPickle")

-zope.deferredimport.deprecatedFrom(
-    "Import from .representation",
-    "nti.externalization.representation",
-    "to_external_representation",
-    "to_json_representation",
-    "to_json_representation_externalized",
-    "make_repr",
-    "WithRepr")
-
-zope.deferredimport.initialize()
-zope.deferredimport.deprecatedFrom(
-    "Import from .nti.ntiids.oids",
-    "nti.ntiids.oids",
-    "to_external_ntiid_oid")

oids.py would be losing this:

-# Things that have moved
-zope.deferredimport.initialize()
-zope.deferredimport.deprecatedFrom(
-    "Import from nti.ntiids.oids",
-    "nti.ntiids.oids",
-    "MASKED_EXTERNAL_CREATOR",
-    "DEFAULT_EXTERNAL_CREATOR",
-    "to_external_ntiid_oid")

@cutz
Copy link

cutz commented Jun 26, 2018

Will usage of those deprecated imports result in log messages like so?

/home/ntiuser/buildout/eggs/ZODB-5.4.0-py2.7.egg/ZODB/broken.py:199: FutureWarning: UserBlacklistedStorage is deprecated. Moved to nti.dataserver.users.black_list

@jamadden
Copy link
Member Author

Yes.

@cutz
Copy link

cutz commented Jun 26, 2018

Looking across all environments logs for year-to-date the only deprecation warning I see that looks relevant is for to_external_ntiid_oid in the bound environments. That looks like it was addressed over 30 days ago though.

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

No branches or pull requests

3 participants