From 942a89be5c112842a3f4b8d9fa8d9bd7584cf58e Mon Sep 17 00:00:00 2001
From: misakwa <misakwa@noreply.com>
Date: Fri, 18 Nov 2016 01:35:57 -0600
Subject: [PATCH 01/12] Add option to use slots for struct payloads

---
 thriftpy/parser/parser.py | 37 +++++++++++++++++++++------
 thriftpy/thrift.py        | 53 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 82 insertions(+), 8 deletions(-)

diff --git a/thriftpy/parser/parser.py b/thriftpy/parser/parser.py
index d443442..5279b6e 100644
--- a/thriftpy/parser/parser.py
+++ b/thriftpy/parser/parser.py
@@ -15,7 +15,7 @@
 from .lexer import *  # noqa
 from .exc import ThriftParserError, ThriftGrammerError
 from thriftpy._compat import urlopen, urlparse
-from ..thrift import gen_init, TType, TPayload, TException
+from ..thrift import gen_init, TType, TPayload, TSPayload, TException
 
 
 def p_error(p):
@@ -209,15 +209,21 @@ def p_enum_item(p):
 
 def p_struct(p):
     '''struct : seen_struct '{' field_seq '}' '''
-    val = _fill_in_struct(p[1], p[3])
+    name, pos = p[1]
+    use_slots = p.parser.__use_slots__
+    val = _make_empty_struct(
+        name,
+        base_cls=TSPayload if use_slots else TPayload,
+        slots=_field_names(p[3]) if use_slots else [],
+    )
+    val = _fill_in_struct(val, p[3])
+    setattr(thrift_stack[pos], name, val)
     _add_thrift_meta('structs', val)
 
 
 def p_seen_struct(p):
     '''seen_struct : STRUCT IDENTIFIER '''
-    val = _make_empty_struct(p[2])
-    setattr(thrift_stack[-1], p[2], val)
-    p[0] = val
+    p[0] = (p[2], len(thrift_stack)-1)
 
 
 def p_union(p):
@@ -431,7 +437,7 @@ def p_definition_type(p):
 
 
 def parse(path, module_name=None, include_dirs=None, include_dir=None,
-          lexer=None, parser=None, enable_cache=True):
+          lexer=None, parser=None, enable_cache=True, use_slots=False):
     """Parse a single thrift file to module object, e.g.::
 
         >>> from thriftpy.parser.parser import parse
@@ -452,6 +458,7 @@ def parse(path, module_name=None, include_dirs=None, include_dir=None,
     :param enable_cache: if this is set to be `True`, parsed module will be
                          cached, this is enabled by default. If `module_name`
                          is provided, use it as cache key, else use the `path`.
+    :param use_slots: if set to `True` uses slots for struct members
     """
     if os.name == 'nt' and sys.version_info < (3, 2):
         os.path.samefile = lambda f1, f2: os.stat(f1) == os.stat(f2)
@@ -474,6 +481,8 @@ def parse(path, module_name=None, include_dirs=None, include_dir=None,
     if parser is None:
         parser = yacc.yacc(debug=False, write_tables=0)
 
+    parser.__use_slots__ = use_slots
+
     global include_dirs_
 
     if include_dirs is not None:
@@ -515,7 +524,7 @@ def parse(path, module_name=None, include_dirs=None, include_dir=None,
     return thrift
 
 
-def parse_fp(source, module_name, lexer=None, parser=None, enable_cache=True):
+def parse_fp(source, module_name, lexer=None, parser=None, enable_cache=True, use_slots=False):
     """Parse a file-like object to thrift module object, e.g.::
 
         >>> from thriftpy.parser.parser import parse_fp
@@ -530,6 +539,7 @@ def parse_fp(source, module_name, lexer=None, parser=None, enable_cache=True):
     :param parser: ply parser to use, if not provided, `parse` will new one.
     :param enable_cache: if this is set to be `True`, parsed module will be
                          cached by `module_name`, this is enabled by default.
+    :param use_slots: if set to `True` uses slots for struct members
     """
     if not module_name.endswith('_thrift'):
         raise ThriftParserError('ThriftPy can only generate module with '
@@ -547,6 +557,8 @@ def parse_fp(source, module_name, lexer=None, parser=None, enable_cache=True):
     if parser is None:
         parser = yacc.yacc(debug=False, write_tables=0)
 
+    parser.__use_slots__ = use_slots
+
     data = source.read()
 
     thrift = types.ModuleType(module_name)
@@ -747,11 +759,20 @@ def _make_enum(name, kvs):
     return cls
 
 
-def _make_empty_struct(name, ttype=TType.STRUCT, base_cls=TPayload):
+def _make_empty_struct(name, ttype=TType.STRUCT, base_cls=TPayload, slots=[]):
     attrs = {'__module__': thrift_stack[-1].__name__, '_ttype': ttype}
+    if issubclass(base_cls, TSPayload):
+        attrs['__slots__'] = slots
     return type(name, (base_cls, ), attrs)
 
 
+def _field_names(fields):
+    fnames = []
+    for _, _, _, name, _ in fields:
+        fnames.append(name)
+    return fnames
+
+
 def _fill_in_struct(cls, fields, _gen_init=True):
     thrift_spec = {}
     default_spec = []
diff --git a/thriftpy/thrift.py b/thriftpy/thrift.py
index bf1db20..7ed7c33 100644
--- a/thriftpy/thrift.py
+++ b/thriftpy/thrift.py
@@ -10,7 +10,9 @@
 from __future__ import absolute_import
 
 import functools
+import itertools
 import linecache
+import operator
 import types
 
 from ._compat import with_metaclass
@@ -167,6 +169,57 @@ def __ne__(self, other):
         return not self.__eq__(other)
 
 
+# XXX: Move payload methods into separate sharable location
+class TSPayload(with_metaclass(TPayloadMeta, object)):
+
+    __hash__ = None
+
+    __slots__ = tuple()
+
+    def read(self, iprot):
+        iprot.read_struct(self)
+
+    def write(self, oprot):
+        oprot.write_struct(self)
+
+    def __repr__(self):
+        keys = itertools.chain.from_iterable(
+            getattr(cls, '__slots__', tuple()) for cls in type(self).__mro__
+        )
+        keys = list(keys)
+        values = operator.attrgetter(*keys)(self)
+        l = ['%s=%r' % (key, value) for key, value in zip(keys, values)]
+        return '%s(%s)' % (self.__class__.__name__, ', '.join(l))
+
+    def __str__(self):
+        return repr(self)
+
+    def __eq__(self, other):
+        if not isinstance(other, self.__class__):
+            return False
+        keys = itertools.chain.from_iterable(
+            getattr(cls, '__slots__', tuple()) for cls in type(self).__mro__
+        )
+        keys = list(keys)
+        getter = operator.attrgetter(*keys)
+        return getter(self) == getter(other)
+
+    def __ne__(self, other):
+        return not self.__eq__(other)
+
+    def __getstate__(self):
+        keys = itertools.chain.from_iterable(
+            getattr(cls, '__slots__', tuple()) for cls in type(self).__mro__
+        )
+        keys = list(keys)
+        values = operator.attrgetter(*keys)(self)
+        return tuple(zip(keys, values))
+
+    def __setstate__(self, state):
+        for k, v in state:
+            setattr(self, k, v)
+
+
 class TClient(object):
 
     def __init__(self, service, iprot, oprot=None):

From b49039386eb27d479de52c1e7fc8fbf44d13e2a3 Mon Sep 17 00:00:00 2001
From: misakwa <misakwa@noreply.com>
Date: Fri, 18 Nov 2016 01:48:26 -0600
Subject: [PATCH 02/12] Expose use_slots to load and load_fp

- Expose using slot based generated code to load entry points
- Add first tests
- TODO:
    - Fix possible cache poisoning when loading slot vs no slot structs
    - Add tests for above
    - Add pickling tests for slot based structs
---
 tests/test_parser.py        | 24 ++++++++++++++++++++++++
 thriftpy/parser/__init__.py |  8 ++++----
 2 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/tests/test_parser.py b/tests/test_parser.py
index 291851a..2d7647c 100644
--- a/tests/test_parser.py
+++ b/tests/test_parser.py
@@ -260,3 +260,27 @@ def test_issue_215():
     thrift = load('parser-cases/issue_215.thrift')
     assert thrift.abool is True
     assert thrift.falseValue == 123
+
+
+def test_load_slots():
+    thrift = load('addressbook.thrift', use_slots=True)
+
+    # normal structs will have slots
+    assert thrift.PhoneNumber.__slots__ == ['type', 'number', 'mix_item']
+    assert thrift.Person.__slots__ == ['name', 'phones', 'created_at']
+    assert thrift.AddressBook.__slots__ == ['people']
+
+    # exceptions will not have slots
+    assert not hasattr(thrift.PersonNotExistsError, '__slots__')
+
+    # services will not have slots
+    assert not hasattr(thrift.AddressBookService, '__slots__')
+
+    # enums will not have slots
+    assert not hasattr(thrift.PhoneType, '__slots__')
+
+    # one cannot get/set undefined attributes
+    person = thrift.Person()
+    with pytest.raises(AttributeError):
+        person.attr_not_exist = "Does not work"
+        person.attr_not_exist
diff --git a/thriftpy/parser/__init__.py b/thriftpy/parser/__init__.py
index 7aa7e2e..0aa92f6 100644
--- a/thriftpy/parser/__init__.py
+++ b/thriftpy/parser/__init__.py
@@ -15,7 +15,7 @@
 from .parser import parse, parse_fp
 
 
-def load(path, module_name=None, include_dirs=None, include_dir=None):
+def load(path, module_name=None, include_dirs=None, include_dir=None, use_slots=False):
     """Load thrift file as a module.
 
     The module loaded and objects inside may only be pickled if module_name
@@ -27,17 +27,17 @@ def load(path, module_name=None, include_dirs=None, include_dir=None):
     """
     real_module = bool(module_name)
     thrift = parse(path, module_name, include_dirs=include_dirs,
-                   include_dir=include_dir)
+                   include_dir=include_dir, use_slots=use_slots)
 
     if real_module:
         sys.modules[module_name] = thrift
     return thrift
 
 
-def load_fp(source, module_name):
+def load_fp(source, module_name, use_slots=False):
     """Load thrift file like object as a module.
     """
-    thrift = parse_fp(source, module_name)
+    thrift = parse_fp(source, module_name, use_slots=use_slots)
     sys.modules[module_name] = thrift
     return thrift
 

From 8c6b36932a2e7466492a40694f73fd40121088e7 Mon Sep 17 00:00:00 2001
From: misakwa <misakwa@noreply.com>
Date: Fri, 18 Nov 2016 07:39:22 -0600
Subject: [PATCH 03/12] Clear cache before testing slots

---
 tests/test_parser.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/test_parser.py b/tests/test_parser.py
index 2d7647c..b19c6b0 100644
--- a/tests/test_parser.py
+++ b/tests/test_parser.py
@@ -3,6 +3,7 @@
 import pytest
 from thriftpy.thrift import TType
 from thriftpy.parser import load, load_fp
+from thriftpy.parser.parser import thrift_cache
 from thriftpy.parser.exc import ThriftParserError, ThriftGrammerError
 
 
@@ -263,6 +264,7 @@ def test_issue_215():
 
 
 def test_load_slots():
+    thrift_cache.clear()
     thrift = load('addressbook.thrift', use_slots=True)
 
     # normal structs will have slots

From 5f1f01eb44151a051cad89a79fa9ad1f7b389dec Mon Sep 17 00:00:00 2001
From: misakwa <misakwa@noreply.com>
Date: Fri, 18 Nov 2016 07:55:14 -0600
Subject: [PATCH 04/12] Cache slot based objects with different keys

---
 tests/test_parser.py      |  2 --
 thriftpy/parser/parser.py | 13 ++++++++++---
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/tests/test_parser.py b/tests/test_parser.py
index b19c6b0..2d7647c 100644
--- a/tests/test_parser.py
+++ b/tests/test_parser.py
@@ -3,7 +3,6 @@
 import pytest
 from thriftpy.thrift import TType
 from thriftpy.parser import load, load_fp
-from thriftpy.parser.parser import thrift_cache
 from thriftpy.parser.exc import ThriftParserError, ThriftGrammerError
 
 
@@ -264,7 +263,6 @@ def test_issue_215():
 
 
 def test_load_slots():
-    thrift_cache.clear()
     thrift = load('addressbook.thrift', use_slots=True)
 
     # normal structs will have slots
diff --git a/thriftpy/parser/parser.py b/thriftpy/parser/parser.py
index 5279b6e..a381bfe 100644
--- a/thriftpy/parser/parser.py
+++ b/thriftpy/parser/parser.py
@@ -436,6 +436,10 @@ def p_definition_type(p):
 thrift_cache = {}
 
 
+def _get_cache_key(prefix, use_slots=False):
+    return ('%s:slotted' % prefix) if use_slots else prefix
+
+
 def parse(path, module_name=None, include_dirs=None, include_dir=None,
           lexer=None, parser=None, enable_cache=True, use_slots=False):
     """Parse a single thrift file to module object, e.g.::
@@ -471,7 +475,8 @@ def parse(path, module_name=None, include_dirs=None, include_dir=None,
 
     global thrift_cache
 
-    cache_key = module_name or os.path.normpath(path)
+    cache_prefix = module_name or os.path.normpath(path)
+    cache_key = _get_cache_key(cache_prefix, use_slots)
 
     if enable_cache and cache_key in thrift_cache:
         return thrift_cache[cache_key]
@@ -545,8 +550,10 @@ def parse_fp(source, module_name, lexer=None, parser=None, enable_cache=True, us
         raise ThriftParserError('ThriftPy can only generate module with '
                                 '\'_thrift\' suffix')
 
+    cache_key = _get_cache_key(module_name, use_slots)
+
     if enable_cache and module_name in thrift_cache:
-        return thrift_cache[module_name]
+        return thrift_cache[cache_key]
 
     if not hasattr(source, 'read'):
         raise ThriftParserError('Except `source` to be a file-like object with'
@@ -569,7 +576,7 @@ def parse_fp(source, module_name, lexer=None, parser=None, enable_cache=True, us
     thrift_stack.pop()
 
     if enable_cache:
-        thrift_cache[module_name] = thrift
+        thrift_cache[cache_key] = thrift
     return thrift
 
 

From cc69f87b5558f010867b9bb62c1ccabff254f499 Mon Sep 17 00:00:00 2001
From: misakwa <misakwa@noreply.com>
Date: Fri, 18 Nov 2016 08:39:35 -0600
Subject: [PATCH 05/12] Move tests into test_hook.py

---
 tests/test_hook.py   | 36 ++++++++++++++++++++++++++++++++++++
 tests/test_parser.py | 24 ------------------------
 2 files changed, 36 insertions(+), 24 deletions(-)

diff --git a/tests/test_hook.py b/tests/test_hook.py
index fe66341..cc044d2 100644
--- a/tests/test_hook.py
+++ b/tests/test_hook.py
@@ -72,3 +72,39 @@ def test_tpayload_pickle():
     person_2 = pickle.loads(PICKLED_BYTES)
 
     assert person == person_2
+
+
+def test_load_slots():
+    thrift = thriftpy.load(
+        'addressbook.thrift',
+        use_slots=True,
+        module_name='addressbook_thrift'
+    )
+
+    # normal structs will have slots
+    assert thrift.PhoneNumber.__slots__ == ['type', 'number', 'mix_item']
+    assert thrift.Person.__slots__ == ['name', 'phones', 'created_at']
+    assert thrift.AddressBook.__slots__ == ['people']
+
+    # XXX: should unions have slots?
+
+    # exceptions will not have slots
+    assert not hasattr(thrift.PersonNotExistsError, '__slots__')
+
+    # services will not have slots
+    assert not hasattr(thrift.AddressBookService, '__slots__')
+
+    # enums will not have slots
+    assert not hasattr(thrift.PhoneType, '__slots__')
+
+    # one cannot get/set undefined attributes
+    person = thrift.Person()
+    with pytest.raises(AttributeError):
+        person.attr_not_exist = "Does not work"
+        person.attr_not_exist
+
+    # should be able to pickle slotted objects - if load with module_name
+    bob = thrift.Person(name="Bob")
+    p_str = pickle.dumps(bob)
+
+    assert pickle.loads(p_str) == bob
diff --git a/tests/test_parser.py b/tests/test_parser.py
index 2d7647c..291851a 100644
--- a/tests/test_parser.py
+++ b/tests/test_parser.py
@@ -260,27 +260,3 @@ def test_issue_215():
     thrift = load('parser-cases/issue_215.thrift')
     assert thrift.abool is True
     assert thrift.falseValue == 123
-
-
-def test_load_slots():
-    thrift = load('addressbook.thrift', use_slots=True)
-
-    # normal structs will have slots
-    assert thrift.PhoneNumber.__slots__ == ['type', 'number', 'mix_item']
-    assert thrift.Person.__slots__ == ['name', 'phones', 'created_at']
-    assert thrift.AddressBook.__slots__ == ['people']
-
-    # exceptions will not have slots
-    assert not hasattr(thrift.PersonNotExistsError, '__slots__')
-
-    # services will not have slots
-    assert not hasattr(thrift.AddressBookService, '__slots__')
-
-    # enums will not have slots
-    assert not hasattr(thrift.PhoneType, '__slots__')
-
-    # one cannot get/set undefined attributes
-    person = thrift.Person()
-    with pytest.raises(AttributeError):
-        person.attr_not_exist = "Does not work"
-        person.attr_not_exist

From 9aa7a615c20b6823c4eb60f57b54b3507cd731fe Mon Sep 17 00:00:00 2001
From: misakwa <misakwa@noreply.com>
Date: Sun, 20 Nov 2016 17:31:16 -0600
Subject: [PATCH 06/12] Slot optimization

- Replace class with a newer one with slot fields
- Implement subclass and instance checks
---
 tests/test_hook.py        | 42 ++++++++++++++++++----------
 thriftpy/parser/parser.py | 47 +++++++++++++++----------------
 thriftpy/thrift.py        | 59 +++++++++++++++++++++++++++++++++++++--
 3 files changed, 105 insertions(+), 43 deletions(-)

diff --git a/tests/test_hook.py b/tests/test_hook.py
index cc044d2..0745c68 100644
--- a/tests/test_hook.py
+++ b/tests/test_hook.py
@@ -82,26 +82,38 @@ def test_load_slots():
     )
 
     # normal structs will have slots
-    assert thrift.PhoneNumber.__slots__ == ['type', 'number', 'mix_item']
-    assert thrift.Person.__slots__ == ['name', 'phones', 'created_at']
-    assert thrift.AddressBook.__slots__ == ['people']
+    # assert set(thrift.PhoneNumber.__slots__) == set(['type', 'number', 'mix_item'])
+    # assert set(thrift.Person.__slots__) == set(['name', 'phones', 'created_at'])
+    # assert set(thrift.AddressBook.__slots__) == set(['people'])
+    # assert set(thrift.AddressBookService.__slots__) == set()
 
-    # XXX: should unions have slots?
+    # one cannot get/set undefined attributes
+    # person = thrift.Person()
+    # with pytest.raises(AttributeError):
+    #     person.attr_not_exist = "Does not work"
 
-    # exceptions will not have slots
-    assert not hasattr(thrift.PersonNotExistsError, '__slots__')
+    # with pytest.raises(AttributeError):
+    #     person.attr_not_exist
 
-    # services will not have slots
-    assert not hasattr(thrift.AddressBookService, '__slots__')
+    # pn = thrift.PhoneNumber()
+    # with pytest.raises(AttributeError):
+    #     pn.attr_not_exist = "Does not work"
 
-    # enums will not have slots
-    assert not hasattr(thrift.PhoneType, '__slots__')
+    # with pytest.raises(AttributeError):
+    #     pn.attr_not_exist
 
-    # one cannot get/set undefined attributes
-    person = thrift.Person()
-    with pytest.raises(AttributeError):
-        person.attr_not_exist = "Does not work"
-        person.attr_not_exist
+    # ab = thrift.AddressBook()
+    # with pytest.raises(AttributeError):
+    #     ab.attr_not_exist = "Does not work"
+
+    # with pytest.raises(AttributeError):
+    #     ab.attr_not_exist
+
+    # exceptions will not have slots
+    # assert not hasattr(thrift.PersonNotExistsError, '__slots__')
+
+    # enums will not have slots
+    # assert not hasattr(thrift.PhoneType, '__slots__')
 
     # should be able to pickle slotted objects - if load with module_name
     bob = thrift.Person(name="Bob")
diff --git a/thriftpy/parser/parser.py b/thriftpy/parser/parser.py
index a381bfe..61e911e 100644
--- a/thriftpy/parser/parser.py
+++ b/thriftpy/parser/parser.py
@@ -209,21 +209,17 @@ def p_enum_item(p):
 
 def p_struct(p):
     '''struct : seen_struct '{' field_seq '}' '''
-    name, pos = p[1]
-    use_slots = p.parser.__use_slots__
-    val = _make_empty_struct(
-        name,
-        base_cls=TSPayload if use_slots else TPayload,
-        slots=_field_names(p[3]) if use_slots else [],
-    )
-    val = _fill_in_struct(val, p[3])
-    setattr(thrift_stack[pos], name, val)
+    val = _fill_in_struct(p[1], p[3])
     _add_thrift_meta('structs', val)
 
 
 def p_seen_struct(p):
     '''seen_struct : STRUCT IDENTIFIER '''
-    p[0] = (p[2], len(thrift_stack)-1)
+    use_slots = p.parser.__use_slots__
+    base_cls = TSPayload if use_slots else TPayload
+    val = _make_empty_struct(p[2], base_cls=base_cls)
+    setattr(thrift_stack[-1], p[2], val)
+    p[0] = val
 
 
 def p_union(p):
@@ -234,7 +230,9 @@ def p_union(p):
 
 def p_seen_union(p):
     '''seen_union : UNION IDENTIFIER '''
-    val = _make_empty_struct(p[2])
+    use_slots = p.parser.__use_slots__
+    base_cls = TSPayload if use_slots else TPayload
+    val = _make_empty_struct(p[2], base_cls=base_cls)
     setattr(thrift_stack[-1], p[2], val)
     p[0] = val
 
@@ -268,7 +266,8 @@ def p_service(p):
     else:
         extends = None
 
-    val = _make_service(p[2], p[len(p) - 2], extends)
+    use_slots = p.parser.__use_slots__
+    val = _make_service(p[2], p[len(p) - 2], extends, use_slots=use_slots)
     setattr(thrift, p[2], val)
     _add_thrift_meta('services', val)
 
@@ -766,20 +765,11 @@ def _make_enum(name, kvs):
     return cls
 
 
-def _make_empty_struct(name, ttype=TType.STRUCT, base_cls=TPayload, slots=[]):
+def _make_empty_struct(name, ttype=TType.STRUCT, base_cls=TPayload):
     attrs = {'__module__': thrift_stack[-1].__name__, '_ttype': ttype}
-    if issubclass(base_cls, TSPayload):
-        attrs['__slots__'] = slots
     return type(name, (base_cls, ), attrs)
 
 
-def _field_names(fields):
-    fnames = []
-    for _, _, _, name, _ in fields:
-        fnames.append(name)
-    return fnames
-
-
 def _fill_in_struct(cls, fields, _gen_init=True):
     thrift_spec = {}
     default_spec = []
@@ -797,6 +787,10 @@ def _fill_in_struct(cls, fields, _gen_init=True):
     setattr(cls, 'thrift_spec', thrift_spec)
     setattr(cls, 'default_spec', default_spec)
     setattr(cls, '_tspec', _tspec)
+    # add __slots__ so we can check way before the class is created, even though
+    # it really does nothing here, the real work is done during instantiation
+    if issubclass(cls, TSPayload):
+        cls.__slots__ = tuple(field for field, _ in default_spec)
     if _gen_init:
         gen_init(cls, thrift_spec, default_spec)
     return cls
@@ -808,11 +802,14 @@ def _make_struct(name, fields, ttype=TType.STRUCT, base_cls=TPayload,
     return _fill_in_struct(cls, fields, _gen_init=_gen_init)
 
 
-def _make_service(name, funcs, extends):
+def _make_service(name, funcs, extends, use_slots=False):
     if extends is None:
         extends = object
 
     attrs = {'__module__': thrift_stack[-1].__name__}
+    base_cls = TSPayload if use_slots else TPayload
+    if use_slots:
+        attrs['__slots__'] = tuple()
     cls = type(name, (extends, ), attrs)
     thrift_services = []
 
@@ -821,7 +818,7 @@ def _make_service(name, funcs, extends):
         # args payload cls
         args_name = '%s_args' % func_name
         args_fields = func[3]
-        args_cls = _make_struct(args_name, args_fields)
+        args_cls = _make_struct(args_name, args_fields, base_cls=base_cls)
         setattr(cls, args_name, args_cls)
         # result payload cls
         result_name = '%s_result' % func_name
@@ -829,7 +826,7 @@ def _make_service(name, funcs, extends):
         result_throws = func[4]
         result_oneway = func[0]
         result_cls = _make_struct(result_name, result_throws,
-                                  _gen_init=False)
+                                  _gen_init=False, base_cls=base_cls)
         setattr(result_cls, 'oneway', result_oneway)
         if result_type != TType.VOID:
             result_cls.thrift_spec[0] = _ttype_spec(result_type, 'success')
diff --git a/thriftpy/thrift.py b/thriftpy/thrift.py
index 7ed7c33..8e1cd6f 100644
--- a/thriftpy/thrift.py
+++ b/thriftpy/thrift.py
@@ -128,12 +128,66 @@ class TMessageType(object):
 
 class TPayloadMeta(type):
 
+    _class_cache = {}
+
     def __new__(cls, name, bases, attrs):
         if "default_spec" in attrs:
             spec = attrs.pop("default_spec")
             attrs["__init__"] = init_func_generator(cls, spec)
         return super(TPayloadMeta, cls).__new__(cls, name, bases, attrs)
 
+# checks: instance and subclass checks
+    def __instancecheck__(cls, inst):
+        mod = inst.__class__.__module__
+        name = inst.__class__.__name__
+        key = "%s:%s" % (mod, name)
+        repl_cls = TPayloadMeta._class_cache.get(key)
+        if repl_cls is None:
+            return type.__instancecheck__(cls, inst)
+        return inst.__class__ is repl_cls
+
+    def __subclasscheck__(cls, subcls):
+        mod = cls.__module__
+        name = cls.__name__
+        key = "%s:%s" % (mod, name)
+        repl_cls = TPayloadMeta._class_cache.get(key)
+        if repl_cls is None:
+            return type.__subclasscheck__(cls, subcls)
+        if cls == repl_cls:
+            return True
+        # the first class in __mro__ is replaced by the replacement class so we
+        # can only look up from the second position
+        return cls.__mro__[1] in repl_cls.__mro__
+# eo: checks
+
+    def __call__(cls, *args, **kw):
+        if not cls.__mro__[1] == TSPayload:
+            return super(TPayloadMeta, cls).__call__(cls, *args, **kw)
+        # XXX: replaces class with new class using slot list from default_spec
+        cls_name = cls.__name__.split('.')[-1]
+        cache_key = '%s:%s' % (cls.__module__, cls_name)
+        # XXX: we trust default_spec more than __slots__ for this
+        fields = tuple(field for field, _ in cls.default_spec)
+        cls_obj = TPayloadMeta._class_cache.get(cache_key)
+        if not cls_obj:
+            cls_obj = type(
+                cls_name,
+                cls.__mro__,
+                {
+                    '__slots__': fields,
+                    '__module__': cls.__module__,
+                }
+            )
+            # XXX: need a better way to do this; its a dupe from parser.py
+            cls_obj._ttype = cls._ttype
+            cls_obj._tspec = cls._tspec
+            cls_obj.default_spec = cls.default_spec
+            cls_obj.thrift_spec = cls.thrift_spec
+            # cls.__init__ is already bound to cls
+            cls_obj.__init__ = init_func_generator(cls_obj, cls.default_spec)
+            TPayloadMeta._class_cache[cache_key] = cls_obj
+        return type.__call__(cls_obj, *args, **kw)
+
 
 def gen_init(cls, thrift_spec=None, default_spec=None):
     if thrift_spec is not None:
@@ -169,13 +223,12 @@ def __ne__(self, other):
         return not self.__eq__(other)
 
 
-# XXX: Move payload methods into separate sharable location
 class TSPayload(with_metaclass(TPayloadMeta, object)):
 
-    __hash__ = None
-
     __slots__ = tuple()
 
+    __hash__ = None
+
     def read(self, iprot):
         iprot.read_struct(self)
 

From 1a3b1bd8f84b84dbb430a375c5ea5a9f21dc3e0b Mon Sep 17 00:00:00 2001
From: misakwa <misakwa@noreply.com>
Date: Sun, 20 Nov 2016 22:28:23 -0600
Subject: [PATCH 07/12] Rewrite class during creation with supported slots

During the first creation of the loaded class with slots, a new class is
inserted into the inheritance chain to ensure that the slot fields are defined.

This is done because of the need to create an empty struct during the
parsing phase and fill it in later - slots require the fields to be known before hand.

All checks on the new replacement class will have it looking like the
original except an equality comparison between the replaced class and
its replacement.

>>> import thriftpy
>>> ab = thriftpy.load('addressbook.thrift', use_slots=True)
>>> ab_inst = ab.AddressBook()
>>> ab_inst.__class__ == ab.AddressBook # will return False
>>> # all other checks should work as expected
>>> isinstance(ab_inst, ab.AddressBook) # will return True
>>> issubclass(ab_inst.__class__, ab.AddressBook) # will return True

In order to get pickling to work as expected, a new extension type is
registered with copyreg (copy_reg for py2x) to avoid pickling errors.
---
 tests/test_hook.py        | 60 ++++++++++++++++----------
 thriftpy/parser/parser.py | 10 ++---
 thriftpy/thrift.py        | 90 ++++++++++-----------------------------
 3 files changed, 66 insertions(+), 94 deletions(-)

diff --git a/tests/test_hook.py b/tests/test_hook.py
index 0745c68..7485605 100644
--- a/tests/test_hook.py
+++ b/tests/test_hook.py
@@ -82,38 +82,54 @@ def test_load_slots():
     )
 
     # normal structs will have slots
-    # assert set(thrift.PhoneNumber.__slots__) == set(['type', 'number', 'mix_item'])
-    # assert set(thrift.Person.__slots__) == set(['name', 'phones', 'created_at'])
-    # assert set(thrift.AddressBook.__slots__) == set(['people'])
-    # assert set(thrift.AddressBookService.__slots__) == set()
+    assert thrift.PhoneNumber.__slots__ == ['type', 'number', 'mix_item']
+    assert thrift.Person.__slots__ == ['name', 'phones', 'created_at']
+    assert thrift.AddressBook.__slots__ == ['people']
 
-    # one cannot get/set undefined attributes
-    # person = thrift.Person()
-    # with pytest.raises(AttributeError):
-    #     person.attr_not_exist = "Does not work"
+    # get/set undefined attributes
+    person = thrift.Person()
+    with pytest.raises(AttributeError):
+        person.attr_not_exist = "Does not work"
 
-    # with pytest.raises(AttributeError):
-    #     person.attr_not_exist
+    with pytest.raises(AttributeError):
+        person.attr_not_exist
 
-    # pn = thrift.PhoneNumber()
-    # with pytest.raises(AttributeError):
-    #     pn.attr_not_exist = "Does not work"
+    pn = thrift.PhoneNumber()
+    with pytest.raises(AttributeError):
+        pn.attr_not_exist = "Does not work"
 
-    # with pytest.raises(AttributeError):
-    #     pn.attr_not_exist
+    with pytest.raises(AttributeError):
+        pn.attr_not_exist
 
-    # ab = thrift.AddressBook()
-    # with pytest.raises(AttributeError):
-    #     ab.attr_not_exist = "Does not work"
+    ab = thrift.AddressBook()
+    with pytest.raises(AttributeError):
+        ab.attr_not_exist = "Does not work"
 
-    # with pytest.raises(AttributeError):
-    #     ab.attr_not_exist
+    with pytest.raises(AttributeError):
+        ab.attr_not_exist
+    # eo: get/set
 
     # exceptions will not have slots
-    # assert not hasattr(thrift.PersonNotExistsError, '__slots__')
+    assert not hasattr(thrift.PersonNotExistsError, '__slots__')
 
     # enums will not have slots
-    # assert not hasattr(thrift.PhoneType, '__slots__')
+    assert not hasattr(thrift.PhoneType, '__slots__')
+
+    # service itself will not be created with slots
+    assert not hasattr(thrift.AddressBookService, '__slots__')
+
+    # service args will have slots
+    args_slots = thrift.AddressBookService.get_phonenumbers_args.__slots__
+    assert args_slots == ['name', 'count']
+
+    # XXX: service result will have their slot list empty until after they are
+    # created. This is because the success field is inserted after calling
+    # _make_struct. We could hardcode a check in the metaclass for names ending
+    # with _result, but I'm unsure if its a good idea. This should usually not
+    # be an issue since this object is only used internally, but we can revisit
+    # the need to have it available when required.
+    result_obj = thrift.AddressBookService.get_phonenumbers_result()
+    assert result_obj.__slots__ == ['success']
 
     # should be able to pickle slotted objects - if load with module_name
     bob = thrift.Person(name="Bob")
diff --git a/thriftpy/parser/parser.py b/thriftpy/parser/parser.py
index 61e911e..17432a7 100644
--- a/thriftpy/parser/parser.py
+++ b/thriftpy/parser/parser.py
@@ -767,6 +767,8 @@ def _make_enum(name, kvs):
 
 def _make_empty_struct(name, ttype=TType.STRUCT, base_cls=TPayload):
     attrs = {'__module__': thrift_stack[-1].__name__, '_ttype': ttype}
+    if issubclass(base_cls, TSPayload):
+        attrs['__slots__'] = []
     return type(name, (base_cls, ), attrs)
 
 
@@ -787,10 +789,9 @@ def _fill_in_struct(cls, fields, _gen_init=True):
     setattr(cls, 'thrift_spec', thrift_spec)
     setattr(cls, 'default_spec', default_spec)
     setattr(cls, '_tspec', _tspec)
-    # add __slots__ so we can check way before the class is created, even though
-    # it really does nothing here, the real work is done during instantiation
+    # add __slots__ for easy introspection
     if issubclass(cls, TSPayload):
-        cls.__slots__ = tuple(field for field, _ in default_spec)
+        cls.__slots__ = [field for field, _ in default_spec]
     if _gen_init:
         gen_init(cls, thrift_spec, default_spec)
     return cls
@@ -808,8 +809,7 @@ def _make_service(name, funcs, extends, use_slots=False):
 
     attrs = {'__module__': thrift_stack[-1].__name__}
     base_cls = TSPayload if use_slots else TPayload
-    if use_slots:
-        attrs['__slots__'] = tuple()
+    # service class itself will not be created with slots
     cls = type(name, (extends, ), attrs)
     thrift_services = []
 
diff --git a/thriftpy/thrift.py b/thriftpy/thrift.py
index 8e1cd6f..27871fe 100644
--- a/thriftpy/thrift.py
+++ b/thriftpy/thrift.py
@@ -9,10 +9,13 @@
 
 from __future__ import absolute_import
 
+try:
+    import copy_reg as copyreg
+except ImportError:
+    import copyreg
+
 import functools
-import itertools
 import linecache
-import operator
 import types
 
 from ._compat import with_metaclass
@@ -136,57 +139,27 @@ def __new__(cls, name, bases, attrs):
             attrs["__init__"] = init_func_generator(cls, spec)
         return super(TPayloadMeta, cls).__new__(cls, name, bases, attrs)
 
-# checks: instance and subclass checks
-    def __instancecheck__(cls, inst):
-        mod = inst.__class__.__module__
-        name = inst.__class__.__name__
-        key = "%s:%s" % (mod, name)
-        repl_cls = TPayloadMeta._class_cache.get(key)
-        if repl_cls is None:
-            return type.__instancecheck__(cls, inst)
-        return inst.__class__ is repl_cls
-
-    def __subclasscheck__(cls, subcls):
-        mod = cls.__module__
-        name = cls.__name__
-        key = "%s:%s" % (mod, name)
-        repl_cls = TPayloadMeta._class_cache.get(key)
-        if repl_cls is None:
-            return type.__subclasscheck__(cls, subcls)
-        if cls == repl_cls:
-            return True
-        # the first class in __mro__ is replaced by the replacement class so we
-        # can only look up from the second position
-        return cls.__mro__[1] in repl_cls.__mro__
-# eo: checks
-
     def __call__(cls, *args, **kw):
-        if not cls.__mro__[1] == TSPayload:
-            return super(TPayloadMeta, cls).__call__(cls, *args, **kw)
-        # XXX: replaces class with new class using slot list from default_spec
+        # if issubclass(cls, TSPayload):
+        if not issubclass(cls, TSPayload):
+            return type.__call__(cls, *args, **kw)
         cls_name = cls.__name__.split('.')[-1]
         cache_key = '%s:%s' % (cls.__module__, cls_name)
-        # XXX: we trust default_spec more than __slots__ for this
-        fields = tuple(field for field, _ in cls.default_spec)
-        cls_obj = TPayloadMeta._class_cache.get(cache_key)
-        if not cls_obj:
-            cls_obj = type(
+        kls = TPayloadMeta._class_cache.get(cache_key)
+        if not kls:
+            fields = [field for field, _ in cls.default_spec]
+            kls = type(
                 cls_name,
-                cls.__mro__,
+                (cls,),
                 {
                     '__slots__': fields,
                     '__module__': cls.__module__,
                 }
             )
-            # XXX: need a better way to do this; its a dupe from parser.py
-            cls_obj._ttype = cls._ttype
-            cls_obj._tspec = cls._tspec
-            cls_obj.default_spec = cls.default_spec
-            cls_obj.thrift_spec = cls.thrift_spec
-            # cls.__init__ is already bound to cls
-            cls_obj.__init__ = init_func_generator(cls_obj, cls.default_spec)
-            TPayloadMeta._class_cache[cache_key] = cls_obj
-        return type.__call__(cls_obj, *args, **kw)
+            TPayloadMeta._class_cache[cache_key] = kls
+            fn = lambda obj: (cls, tuple(getattr(obj, f) for f in fields))
+            copyreg.pickle(kls, fn)
+        return type.__call__(kls, *args, **kw)
 
 
 def gen_init(cls, thrift_spec=None, default_spec=None):
@@ -236,11 +209,8 @@ def write(self, oprot):
         oprot.write_struct(self)
 
     def __repr__(self):
-        keys = itertools.chain.from_iterable(
-            getattr(cls, '__slots__', tuple()) for cls in type(self).__mro__
-        )
-        keys = list(keys)
-        values = operator.attrgetter(*keys)(self)
+        keys = self.__slots__
+        values = [getattr(self, k) for k in keys]
         l = ['%s=%r' % (key, value) for key, value in zip(keys, values)]
         return '%s(%s)' % (self.__class__.__name__, ', '.join(l))
 
@@ -250,28 +220,14 @@ def __str__(self):
     def __eq__(self, other):
         if not isinstance(other, self.__class__):
             return False
-        keys = itertools.chain.from_iterable(
-            getattr(cls, '__slots__', tuple()) for cls in type(self).__mro__
-        )
-        keys = list(keys)
-        getter = operator.attrgetter(*keys)
-        return getter(self) == getter(other)
+        keys = self.__slots__
+        vals1 = [getattr(self, k) for k in keys]
+        vals2 = [getattr(self, k) for k in keys]
+        return vals1 == vals2
 
     def __ne__(self, other):
         return not self.__eq__(other)
 
-    def __getstate__(self):
-        keys = itertools.chain.from_iterable(
-            getattr(cls, '__slots__', tuple()) for cls in type(self).__mro__
-        )
-        keys = list(keys)
-        values = operator.attrgetter(*keys)(self)
-        return tuple(zip(keys, values))
-
-    def __setstate__(self, state):
-        for k, v in state:
-            setattr(self, k, v)
-
 
 class TClient(object):
 

From 403276348f3774ecfc246f1595c2632fe100ec81 Mon Sep 17 00:00:00 2001
From: misakwa <misakwa@noreply.com>
Date: Tue, 22 Nov 2016 06:51:45 -0600
Subject: [PATCH 08/12] fix service result slots

---
 tests/test_hook.py        | 10 ++--------
 thriftpy/parser/parser.py |  2 ++
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/tests/test_hook.py b/tests/test_hook.py
index 7485605..b7b69c0 100644
--- a/tests/test_hook.py
+++ b/tests/test_hook.py
@@ -122,14 +122,8 @@ def test_load_slots():
     args_slots = thrift.AddressBookService.get_phonenumbers_args.__slots__
     assert args_slots == ['name', 'count']
 
-    # XXX: service result will have their slot list empty until after they are
-    # created. This is because the success field is inserted after calling
-    # _make_struct. We could hardcode a check in the metaclass for names ending
-    # with _result, but I'm unsure if its a good idea. This should usually not
-    # be an issue since this object is only used internally, but we can revisit
-    # the need to have it available when required.
-    result_obj = thrift.AddressBookService.get_phonenumbers_result()
-    assert result_obj.__slots__ == ['success']
+    result_slots = thrift.AddressBookService.get_phonenumbers_result.__slots__
+    assert result_slots == ['success']
 
     # should be able to pickle slotted objects - if load with module_name
     bob = thrift.Person(name="Bob")
diff --git a/thriftpy/parser/parser.py b/thriftpy/parser/parser.py
index 17432a7..988198f 100644
--- a/thriftpy/parser/parser.py
+++ b/thriftpy/parser/parser.py
@@ -833,6 +833,8 @@ def _make_service(name, funcs, extends, use_slots=False):
             result_cls.default_spec.insert(0, ('success', None))
         gen_init(result_cls, result_cls.thrift_spec, result_cls.default_spec)
         setattr(cls, result_name, result_cls)
+        # default spec is modified after making struct so add slots here
+        result_cls.__slots__ = [f for f, _ in result_cls.default_spec]
         thrift_services.append(func_name)
     if extends is not None and hasattr(extends, 'thrift_services'):
         thrift_services.extend(extends.thrift_services)

From afd9acdd825892733d559398cd1f070f37e95479 Mon Sep 17 00:00:00 2001
From: misakwa <misakwa@noreply.com>
Date: Tue, 22 Nov 2016 06:54:38 -0600
Subject: [PATCH 09/12] Add test for recursive type

---
 tests/test_hook.py | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/tests/test_hook.py b/tests/test_hook.py
index b7b69c0..b0d06dc 100644
--- a/tests/test_hook.py
+++ b/tests/test_hook.py
@@ -130,3 +130,10 @@ def test_load_slots():
     p_str = pickle.dumps(bob)
 
     assert pickle.loads(p_str) == bob
+
+    # works for recursive types too
+    rec = thriftpy.load('parser-cases/recursive_union.thrift', use_slots=True)
+    rec_slots = rec.Dynamic.__slots__
+    assert rec_slots == ['boolean', 'integer', 'doubl', 'str', 'arr', 'object']
+    with pytest.raises(AttributeError):
+        rec.Dynamic.attr_not_exist = "shouldn't work"

From 65292c7c365b9b2b978deb54d5697ee74e4b8d32 Mon Sep 17 00:00:00 2001
From: misakwa <misakwa@noreply.com>
Date: Tue, 22 Nov 2016 06:59:44 -0600
Subject: [PATCH 10/12] Fix recursive type test

---
 tests/test_hook.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/test_hook.py b/tests/test_hook.py
index b0d06dc..c0d1fc8 100644
--- a/tests/test_hook.py
+++ b/tests/test_hook.py
@@ -135,5 +135,6 @@ def test_load_slots():
     rec = thriftpy.load('parser-cases/recursive_union.thrift', use_slots=True)
     rec_slots = rec.Dynamic.__slots__
     assert rec_slots == ['boolean', 'integer', 'doubl', 'str', 'arr', 'object']
+    dyn = rec.Dynamic()
     with pytest.raises(AttributeError):
-        rec.Dynamic.attr_not_exist = "shouldn't work"
+        dyn.attr_not_exist = "shouldn't work"

From 632c88ee0ca2839969394f100c707fd2f353fa60 Mon Sep 17 00:00:00 2001
From: misakwa <misakwa@noreply.com>
Date: Tue, 22 Nov 2016 07:02:20 -0600
Subject: [PATCH 11/12] Remove stale comment

---
 thriftpy/thrift.py | 1 -
 1 file changed, 1 deletion(-)

diff --git a/thriftpy/thrift.py b/thriftpy/thrift.py
index 27871fe..edcdfc2 100644
--- a/thriftpy/thrift.py
+++ b/thriftpy/thrift.py
@@ -140,7 +140,6 @@ def __new__(cls, name, bases, attrs):
         return super(TPayloadMeta, cls).__new__(cls, name, bases, attrs)
 
     def __call__(cls, *args, **kw):
-        # if issubclass(cls, TSPayload):
         if not issubclass(cls, TSPayload):
             return type.__call__(cls, *args, **kw)
         cls_name = cls.__name__.split('.')[-1]

From 682e191c32daf0edc0f536eb73da1e2534733ea9 Mon Sep 17 00:00:00 2001
From: misakwa <misakwa@noreply.com>
Date: Tue, 22 Nov 2016 07:21:58 -0600
Subject: [PATCH 12/12] Fix TSPayload equality check

---
 thriftpy/thrift.py | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/thriftpy/thrift.py b/thriftpy/thrift.py
index edcdfc2..7310a29 100644
--- a/thriftpy/thrift.py
+++ b/thriftpy/thrift.py
@@ -220,9 +220,9 @@ def __eq__(self, other):
         if not isinstance(other, self.__class__):
             return False
         keys = self.__slots__
-        vals1 = [getattr(self, k) for k in keys]
-        vals2 = [getattr(self, k) for k in keys]
-        return vals1 == vals2
+        this = [getattr(self, k) for k in keys]
+        other_ = [getattr(other, k) for k in keys]
+        return this == other_
 
     def __ne__(self, other):
         return not self.__eq__(other)