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

Support for ZODB on PyPy #20

Merged
merged 86 commits into from
May 19, 2015
Merged

Support for ZODB on PyPy #20

merged 86 commits into from
May 19, 2015

Conversation

jamadden
Copy link
Member

@jamadden jamadden commented Apr 9, 2015

This PR brings the necessary changes to make the complete set of ZODB unit and functional tests pass (1410 in total). (See zopefoundation/ZODB#33)

Mostly this consisted of making the Python implementation more closely mirror the C implementation. A few notable things:

  • The PickleCache now supports the byte-size limit and the total_estimated_size.
  • The PickleCache now ghostifies objects during a GC, and handles objects that either deliberately or accidentally override _p_invalidate and _p_activate without calling super (ZODB's Blob is an example of a deliberate use of both).
  • The Persistent class now calls the same methods in the same order as the C class; code in the wild could easily depend on this accidentally. It's also safe to use many of the _p attributes in more circumstances than before (ZODB's Broken class relies on this).
  • A few restrictions had to be relaxed to match the C implementation (e.g., allowing arbitrary values for _p_oid, something that could easily crop up in test code in the wild like it did in ZODB) ; a few restrictions had to be added to match the C implementation (e.g., requiring objects in the PickleCache to have a jar).

All of the existing test cases should pass with very minor changes. I've already added a few test cases for the easier things and will see about adding a few more if I can extract them from ZODB in a reasonable form to try to get the coverage a bit higher (although it's not too bad as it is). Some of the scenarios are fairly complex though so it may be difficult to separate them. (Although, especially in the case of the C implementations it seems like many of the test cases still effectively live in ZODB.)

I'm opening this PR a bit early because I know it's large and involves some changes in the way the cache and Persistent objects have to interact, so I'm open to any feedback and suggestions. Plus I probably can't open a PR for the ZODB changes until this gets merged and released :)

Name                     Stmts   Miss Branch BrMiss  Cover   Missing
--------------------------------------------------------------------
persistent                  21     11      0      0    48%   24-46
persistent._compat           1      0      0      0   100%
persistent.dict              1      0      0      0   100%
persistent.interfaces        9      3      0      0    67%   23-25
persistent.list             60      0      0      0   100%
persistent.mapping          44      0      4      0   100%
persistent.persistence     297      0    150      0   100%
persistent.picklecache     223     15    106     12    92%   65-67, 100, 118, 124, 165, 295-305, 399-401
persistent.timestamp       125      0     14      0   100%
persistent.timestamp       125      0     14      0   100%
persistent.wref             68      0     18      0   100%
--------------------------------------------------------------------
TOTAL                      972     31    308     14    96%

…tion needs to bypass the _setattr_ implementation when working with its own internal state variables. This much gets the ZODB broken tests to pass. Need to figure out how to test this locally.
…the C version in more cases. Also, make the behaviour of comparisons and hashing match as well; test all this.
…'s testCache passes. Required some small updates and additions to our own test cases; the update_object_size_estimation is not tested here yet.
…, and clear out references to ejected objects when done. This makes the ZODB testConnection and testConnectionSavepoint tests pass but is not tested here.
…t. There are some interconnected lifetime issues that this solves for ZODB's cache tests, and what appear to be some genuine bugs in invalidation.
…tion needs to bypass the _setattr_ implementation when working with its own internal state variables. This much gets the ZODB broken tests to pass. Need to figure out how to test this locally.
…'s testCache passes. Required some small updates and additions to our own test cases; the update_object_size_estimation is not tested here yet.
…, and clear out references to ejected objects when done. This makes the ZODB testConnection and testConnectionSavepoint tests pass but is not tested here.
…t. There are some interconnected lifetime issues that this solves for ZODB's cache tests, and what appear to be some genuine bugs in invalidation.
@jamadden
Copy link
Member Author

jamadden commented Apr 9, 2015

I was actually able to restore 100% coverage pretty easily, but I'm not sure that really covers all of ZODB's use cases.

@mgedmin
Copy link
Member

mgedmin commented Apr 9, 2015

Speaking of test coverage, I couldn't stop thinking about Hypothesis when I was looking at your recent series of pull requests. Sounds like the perfect tool to discover differences between C and Python implementations, e.g. the timestamp second rounding mismatch.

Unfortunately I don't have time to play with it :(

from .. import ring

if _is_pypy or os.getenv('USING_CFFI'):
assert ring.Ring is ring._CFFIRing
Copy link
Member

Choose a reason for hiding this comment

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

you can use 'self.assertTrue(ring.Ring is ring._CFFIRing)`.

@tseaver
Copy link
Member

tseaver commented May 19, 2015

Whew! That's a lot of back and forth -- thanks for the cheerful effort!

I have opened issues for the non-consensus issues here:

tseaver added a commit that referenced this pull request May 19, 2015
@tseaver tseaver merged commit 39c1f03 into zopefoundation:master May 19, 2015
@tseaver
Copy link
Member

tseaver commented May 19, 2015

@jamadden
Copy link
Member Author

Thank you, @tseaver and everyone else involved. It's been a learning experience and the interactions have been great.

@ctismer
Copy link

ctismer commented May 19, 2015

Whow, I saw this thread way too late. Extremely well thought things going on.
Had an interesting evening studying all this. See you soon.

fabiomaggio pushed a commit to fabiomaggio/persistent that referenced this pull request Feb 20, 2017
…ally.

Performance is way up again, beating CPython on half of the tests:

"Transaction",                mysql
"Add 3000 Objects",              8259
"Update 3000 Objects",           9454
"Read 3000 Warm Objects",        5460
"Read 3000 Cold Objects",        5454
"Read 3000 Hot Objects",        24943
"Read 3000 Steamin' Objects", 1099616

Still needs some GC work to get all the ZODB tests to pass like they do under zopefoundation#20
fabiomaggio pushed a commit to fabiomaggio/persistent that referenced this pull request Feb 20, 2017
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.

None yet

6 participants