Rework Serializable class. #62

pipermerriam · 2018-04-12T00:53:25Z

What was wrong

The Serializable class was ready for some cleanup.

How was it fixed

replaced usage of __setattr_ and __getattr__ with metaclass mechanics and property based setters and getters.
removed the cls.exclude API.
changed is_mutable to be property based.
removed mutation based make_mutable API in favor of a copy based API.
implemented __iter__
improved __eq__ implementation to only return true on equality of rich values rather than serialized values.
added new obj[field_name] and obj[0] based field lookups.
removed cls.fields in favor of cls._meta which holds field names, sedes and some extra meta info.
reworked __init__ argument handling to be a bit more explicit in how it handles args and kwargs.

pipermerriam · 2018-04-13T00:10:16Z

@carver @gsalgado ready for high level preliminary review. The collections.namedtuple idea didn't work out as planned due to the way that namedtuple works. It doesn't really create a class that you can properly inherit from (or I was just doing it wrong because of metaclass wizardry).

gsalgado

Looks good overall, and should allow me to get rid of gsalgado/py-evm@28b9f69#diff-18b46e5ecb111d05094ba0820424e8c7R28

gsalgado · 2018-04-13T08:27:53Z

rlp/sedes/serializable.py

+        }
+        obj = cls(**dict(list(params.items()) + list(kwargs.items())))
+
+        # TODO: mutability


Is there anything really left todo here?

gsalgado · 2018-04-13T08:33:59Z

rlp/sedes/serializable.py

+        raise NotImplementedError("Not yet implemented")
+
+    @property
+    def is_immutable(self):


We probably don't need this as we can just inline the not self.is_mutable, no?

gsalgado · 2018-04-13T08:40:49Z

rlp/sedes/serializable.py

+    is_mutable = True
+
+    _cached_rlp_value = None
+    _cached_rlp_key = None


There's something about having two separate attributes for this that makes me uncomfortable... Did you consider using a dict for that? Maybe _cached_snapshot = {}, or maybe a _get_cached_rlp(snapshot_key) method decorated with a lru_cache

Alright, I've changed to use a class level cache. Each subclass of Serializable now has it's own cache which is shared across instances of those objects as well as across both the mutable and immutable versions of those objects.

carver · 2018-04-13T17:38:50Z

rlp/sedes/serializable.py

+            field = self._meta.field_names[idx]
+            return getattr(self, field)
+        else:
+            raise NotImplementedError("not implemented")


elif isinstance(idx, str): return getattr(self, idx)

Seems like a straightforward and nice thing to do here.

carver · 2018-04-13T17:47:57Z

rlp/sedes/serializable.py

+    def __eq__(self, other):
+        if not hasattr(other, 'serialize'):
+            return False
+        return self.serialize(self) == other.serialize(other)


I'm not sure this is enough. An rlp with type (bytes32, bytes32) and one with type (int, int) can be trivially made to be equal here, which feels wrong to me.

Maybe test the deserialized values?

Agreed.

Note that this is the current way that rlp.sedes.Serializable determines equality:

pyrlp/rlp/sedes/lists.py

Lines 204 to 208 in 188abd6

def __eq__(self, other):

"""Two objects are equal, if they are equal after serialization."""

if not hasattr(other.__class__, 'serialize'):

return False

return self.serialize(self) == other.serialize(other)

I may go back to using a version of the following:

return len(self) == len(other) and all( compare_eq(left, right) for left, right in zip(self, other) )

Where compare_eq is a function which handles things like equality between tuples and lists with the same values which a naive == comparison will return False.

pipermerriam · 2018-04-16T19:03:42Z

Performance Numbers:

This Branch

Block serializations / sec: 1914.65
Block deserializations / sec: 2519.28
TX serializations / sec: 24268.42
TX deserializations / sec: 33170.06

Master @ `f30fe1e`

Block serializations / sec: 1935.49
Block deserializations / sec: 774.44
TX serializations / sec: 25199.91
TX deserializations / sec: 13675.92

gsalgado · 2018-04-16T19:42:29Z

For block importing (i.e. Chain.import_block()), we seem to consistently spend more than twice as long in rlp.decode than in rlp.encode_raw, but that's with py-rlp 0.6.0

pipermerriam · 2018-04-16T20:06:26Z

@gsalgado @carver can I get a final round of 👁 👁

pipermerriam · 2018-04-16T20:11:48Z

@gsalgado looking at stats, it looks like we have 2x calls to decode than encode which means this could have a significant effect on the amount of time spend dealing width decoding rlp objects since this branch almost doubles the speed of decoding rlp Serializable instances.

pipermerriam · 2018-04-16T20:34:27Z

rlp/codec.py

@@ -218,7 +222,6 @@ def decode(rlp, sedes=None, strict=True, **kwargs):
        obj = sedes.deserialize(item, **kwargs)
        if hasattr(obj, '_cached_rlp'):
            obj._cached_rlp = rlp
-            assert not isinstance(obj, Serializable) or not obj.is_mutable()


This was removed in favor of having mutable Serializable instances automatically invalidate their cache.

carver

Sweet

carver · 2018-04-17T00:28:30Z

tests/test_serializable.py

@@ -29,185 +71,209 @@ class RLPType2(Serializable):
        (RLPType2, [RLPType1(8, b'a', (7, ''))], 'field2_2'),
    ),
 )
-def test_validation(rlptype, fields, exception_includes):
+def test_serializable_initialization_validation(rlptype, fields, exception_includes):
+    # TODO: expand tests to combine *args and **kwargs instantiation


Time to do?

carver · 2018-04-17T00:31:34Z

tests/test_serializable.py

+    assert rlp_obj.is_mutable is True
+
+    # cache should be populated now.
+    assert encode(rlp_obj, cache=True) == rlp_code


Does it make sense to turn on caching by default?

Use updated CircleCI images

pipermerriam mentioned this pull request Apr 12, 2018

Library cleanup: Drop un-used APIs, optimizations, cleanup #57

Merged

pipermerriam force-pushed the piper/rework-serializable-class branch from 7ef3ef3 to 29cc832 Compare April 12, 2018 23:00

gsalgado reviewed Apr 13, 2018

View reviewed changes

pipermerriam changed the title ~~STUB~~ Rework Serializable class. Apr 13, 2018

carver approved these changes Apr 13, 2018

View reviewed changes

pipermerriam force-pushed the piper/rework-serializable-class branch from 66b5d4a to 738d9da Compare April 16, 2018 19:57

Rewrite of Serializable class

fd14033

pipermerriam force-pushed the piper/rework-serializable-class branch from 738d9da to fd14033 Compare April 16, 2018 20:01

pipermerriam commented Apr 16, 2018

View reviewed changes

carver approved these changes Apr 17, 2018

View reviewed changes

pipermerriam added 2 commits April 16, 2018 20:49

expand instantiation tests to include args and kwargs combos

9a71597

turn caching on by default

a14b785

pipermerriam merged commit e5018ce into ethereum:master Apr 17, 2018

pipermerriam deleted the piper/rework-serializable-class branch April 17, 2018 02:54

pipermerriam mentioned this pull request Apr 17, 2018

RLP fields as proper object attributes. #44

Open

pacrob pushed a commit to pacrob/pyrlp that referenced this pull request Sep 21, 2023

Merge pull request ethereum#62 from ethereum/circleci-image-update

4533bf6

Use updated CircleCI images

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework Serializable class. #62

Rework Serializable class. #62

pipermerriam commented Apr 12, 2018 •

edited

Loading

pipermerriam commented Apr 13, 2018

gsalgado left a comment

gsalgado Apr 13, 2018

gsalgado Apr 13, 2018

gsalgado Apr 13, 2018

pipermerriam Apr 13, 2018

carver Apr 13, 2018

carver Apr 13, 2018

pipermerriam Apr 13, 2018

pipermerriam commented Apr 16, 2018 •

edited

Loading

gsalgado commented Apr 16, 2018

pipermerriam commented Apr 16, 2018

pipermerriam commented Apr 16, 2018

pipermerriam Apr 16, 2018

carver left a comment

carver Apr 17, 2018

carver Apr 17, 2018

	def __eq__(self, other):
	"""Two objects are equal, if they are equal after serialization."""
	if not hasattr(other.__class__, 'serialize'):
	return False
	return self.serialize(self) == other.serialize(other)

Rework Serializable class. #62

Rework Serializable class. #62

Conversation

pipermerriam commented Apr 12, 2018 • edited Loading

What was wrong

How was it fixed

pipermerriam commented Apr 13, 2018

gsalgado left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pipermerriam commented Apr 16, 2018 • edited Loading

This Branch

Master @ f30fe1e

gsalgado commented Apr 16, 2018

pipermerriam commented Apr 16, 2018

pipermerriam commented Apr 16, 2018

Choose a reason for hiding this comment

carver left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pipermerriam commented Apr 12, 2018 •

edited

Loading

pipermerriam commented Apr 16, 2018 •

edited

Loading

Master @ `f30fe1e`