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

New node cache (compatibility with Python 3.4) #311

Merged
merged 12 commits into from
Jan 15, 2014

Conversation

avalentino
Copy link
Member

This PR implements a possible solution to the incompatibility (see #306) of the node cache management currently implemented in Pytables 3.0 with the new object finalization strategy (see [1], [2]) introduced in Python 3.4 (tested on Python 3.4b1).

The node caching mechanism has been completely redesigned to be simpler and less dependent from specific behaviors of the __del__ method.

A new NodeManager class has been introduced that takes in charge all stuffs related to node caching and node finalization.
It includes a LRU cache for a fast node access and a registry that holds a (weak) reference to all open nodes (including the ones in the cache) to ensure a correct finalization when the file handler is closed.

The new implementation seems to work with all supported python versions (including Python 3.4 tip).
but no performance test has been performed (feedback is welcome).

P.S. probably the documentation regarding NODE_CACHE_SLOTS should be updated to reflect the new implementation.

[1] http://docs.python.org/3.4/whatsnew/3.4.html#pep-442-safe-object-finalization
[2] http://www.python.org/dev/peps/pep-0442

@scopatz
Copy link
Member

scopatz commented Dec 31, 2013

This looks great to me, but I haven't tested it yet. I'll try to get to that tonight. Thanks @avalentino!

@andreabedini
Copy link
Contributor

Great job, thanks for this @avalentino.

class _DictCache(dict):
def __init__(self, nslots):
if nslots < 1:
raise ValueError("Invaòlid number of slots: %d" % nslots)
Copy link
Member

Choose a reason for hiding this comment

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

Should be 'Invalid'

@FrancescAlted
Copy link
Member

Excellent job Antonio. Besides some syntactic errors, the only thing that I noticed is that a negative value for NODE_CACHE_SLOTS is not supported anymore. Have you added some check in code for detecting that? Some users may be using that.

However, and in order to avoid a large memory consumption, the user will be
warned when the number of loaded nodes will reach the -NODE_CACHE_SLOTS
warned when the number of loaded nodes will reach the ``-NODE_CACHE_SLOTS``
value.
Copy link
Member

Choose a reason for hiding this comment

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

Uh, is this inconsistent with https://github.com/PyTables/PyTables/pull/311/files#diff-0035a6b5c4e6a6c60fe4778f61bca5b2L170 where you explicitly removed the docs about negative values of NODE_CACHE_SLOTS or am I missing something?

Antonio Valentino added 2 commits January 2, 2014 10:54
The possibility to use a DictCache has been re-introduced in 310b355.
@avalentino
Copy link
Member Author

Hi @FrancescAlted, thank you very much for the code review

the only thing that I noticed is that a negative value for NODE_CACHE_SLOTS is not supported anymore. Have you added some check in code for detecting that? Some users may be using that.

Ah sorry, I re-introduced that feature in 310b355 but forgot to revert the change in the doc.

@andreabedini
Copy link
Contributor

#121 mentions NODE_CACHE_SLOTS too, can that be addressed here too?

@avalentino
Copy link
Member Author

#121 mentions NODE_CACHE_SLOTS too, can that be addressed here too?

yes, it would be nice.
I don't remember if we already have a small program to use for benchmarking.

@avalentino
Copy link
Member Author

Hi @andreabedini, just closed #121.
The current value of NODE_CACHE_SLOTS is OK.

@avalentino avalentino merged commit 67fce2b into PyTables:develop Jan 15, 2014
@avalentino avalentino deleted the issue306 branch January 15, 2014 22:34
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.

5 participants