#15642: Integrate pickle protocol version 4 GSoC work by Stefan Mihaila
|
Created:
9 months, 1 week ago by alexandre Modified:
1 week, 5 days ago Reviewers:
mstefanro, pitrou CC:
jcea, AntoinePitrou, Alexandre Vassalotti, asvetlov, storchaka, Stefan M Visibility:
Public. |
Patch Set 1 #
Total comments: 118
Patch Set 2 #
Total comments: 11
Patch Set 3 #
Created: 1 week, 5 days ago
MessagesTotal messages: 10
Hi Stefan, I have started reviewing your implementation of pickle protocol 4. There is a lot things fix and discuss. I have been a bit terse in my review comment to go quickly. I hope my comments won't sound too harsh because of this. Anyway, I just want to let you know that you are on the good track! http://bugs.python.org/review/15642/diff/5733/Lib/copyreg.py File Lib/copyreg.py (right): http://bugs.python.org/review/15642/diff/5733/Lib/copyreg.py#newcode91 Lib/copyreg.py:91: #this is because reduce can't work with kwargs at this time. Can you format your comments like this? # Note the lack of ... This is mandated by PEP 8: http://www.python.org/dev/peps/pep-0008/#comments Also I would rewrite this completely to instead explain the *purpose* of __newobj_kw__. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py File Lib/pickle.py (left): http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#oldcode1135 Lib/pickle.py:1135: raise Ah, that was old cruft! http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py File Lib/pickle.py (right): http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode26 Lib/pickle.py:26: __version__ = "$Revision$" # Code version Please remove. We don't use magic version tags in Python. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode45 Lib/pickle.py:45: # See http://bugs.python.org/issue15397 Please include a summary of the issue, like this: # Issue 15397: Add pickling support bound method... # <explain how it work here> http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode48 Lib/pickle.py:48: _MethodWrapperType = type([].__add__) These should added to the 'types' module if they are missing. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode57 Lib/pickle.py:57: ### Remove. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode58 Lib/pickle.py:58: def _t(self, func): #used for pickling methods with REDUCE Please use a more descriptive name here. And add a docstring explaining how it is used, instead of the comment. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode59 Lib/pickle.py:59: return func.__get__(self) Add a newline. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode60 Lib/pickle.py:60: def _isclassmethod(func): I think this would work too: def _isclassmethod(func): return hasattr(func, '__self__') and type(func.__self__) is type and \ type(func) in [MethodType, BuiltinFunctionType] Also, this doesn't work for: >>> class A: ... @classmethod ... def f(cls): ... pass ... >>> _isclassmethod(A().f) False since >>> type(A().f.__self__) <type 'classobj'> http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode86 Lib/pickle.py:86: DEFAULT_PROTOCOL = 3 We should bump this too. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode211 Lib/pickle.py:211: BINBYTES64 = b'\x8c' Can you add comments for these too? http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode212 Lib/pickle.py:212: SEMISHORT_BINUNICODE = b'\x8d' Why not BINUNICODE16? http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode215 Lib/pickle.py:215: SEMISHORT_BINBYTES = b'\x90' Same thing here. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode245 Lib/pickle.py:245: common_modules = None): Align. And remove spaces around = http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode273 Lib/pickle.py:273: this exact same value must be provided to the unpickler. I am not totally convinced of this feature and optimization. The mechanism seems brittle to me. I feel this kind of optimization should be done as a separate step in pickletools.optimize() using the PUT and GET mechanism. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode320 Lib/pickle.py:320: raise Can you explain me the improvement of using BAIL_OUT over raising an exception? It seems to me that people would much rather have pickle to fail during the pickling step than at the unpickling step. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode324 Lib/pickle.py:324: def memoize(self, obj, v4_binput=False): Can you change the name of this argument? We might get a protocol v5 in the future and that name would be confusing. Also, what is the use-case of this argument? http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode339 Lib/pickle.py:339: # Note: As of version 4, this is no longer just a convention. The Nitpick: Put the note in a separate paragraph. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode349 Lib/pickle.py:349: # memoize automatically. Redundant with your above note. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode351 Lib/pickle.py:351: # "memos" in pickletools indicate which opcodes memoize (for debugging) I don't get this. Can you explain? http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode355 Lib/pickle.py:355: # real scenario Actually, I wouldn't be surprised to see this happen in real code. People tends to abuse pickle a *lot*. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode386 Lib/pickle.py:386: Why not raise an exception instead when this happens? http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode530 Lib/pickle.py:530: raise PicklingError('When pickling a reduced object, ' The term "reduced object" is confusing. I think we should say: When pickling an object via the reduce protocol... http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode538 Lib/pickle.py:538: if not isclass(cls): I think having a separate block for each cases is overkill here. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode865 Lib/pickle.py:865: # dict.fromkeys is dict.fromkeys = False It took me a few reads to parse what you are trying to say here. Can you reformat your comment to make it clearer? http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode885 Lib/pickle.py:885: module = 'builtins' Why is this needed? The fix_imports code handles this case no? http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode897 Lib/pickle.py:897: name_bin = bytes(name , 'utf-8') Remove aligning spaces. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode941 Lib/pickle.py:941: raise PicklingError('Can\'t pickle bound methods in pickle<4') You could use "" here to avoid the \ escape. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode958 Lib/pickle.py:958: # v4 Remove. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode964 Lib/pickle.py:964: return self._RefuseDispatchedSave() I think it would be better to just call save_reduce directly here. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode974 Lib/pickle.py:974: for i, el in enumerate(obj): Use more descriptive names for bs and el http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode974 Lib/pickle.py:974: for i, el in enumerate(obj): And why follow what batch_appends and batch_setitems do? http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode1004 Lib/pickle.py:1004: for el in obj: Same here for el http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode1007 Lib/pickle.py:1007: # Here follows the technique which is also used in save_tuple. Both Here we follow. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode1009 Lib/pickle.py:1009: # memoized until they are filled. This also means that they can't be What is filled? http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode1011 Lib/pickle.py:1011: # e.g.: a=A(); fs=frozenset([a]); a.fs = fs Format your example. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode1014 Lib/pickle.py:1014: # save_frozenset within a save_frozenset). Since a is mutable, it has Since 'a' is mutable... http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode1025 Lib/pickle.py:1025: self.write( POP_MARK + self.get(objmemo[0]) ) No spaces around the parentheses http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode1060 Lib/pickle.py:1060: def getattr_recurse (module, name, default=_None): No space before ( http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode1061 Lib/pickle.py:1061: r""" Why the r-prefix here? http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode1073 Lib/pickle.py:1073: ret = module Have compared your method with the one posted on issue 13520? http://bugs.python.org/issue13520 http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode1074 Lib/pickle.py:1074: for n in name.split('.'): Use more descriptive name for 'n' http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode1110 Lib/pickle.py:1110: elif hasattr(func, "__self__") and hasattr(func.__self__, "__module__") \ Why you don't use your _isclassmethod function here? http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode1164 Lib/pickle.py:1164: """ Format like this: """Like self.read(), but... """ http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode1170 Lib/pickle.py:1170: raise EOFError Add a message to the exception here. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode1196 Lib/pickle.py:1196: d=dispatch[key[0]] Use more descriptive name for 'd' and spaces around '='. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode1198 Lib/pickle.py:1198: raise UnpicklingError('Inexistant opcode '+str(key[0])) Spaces around + http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode1199 Lib/pickle.py:1199: raise Remove. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode1234 Lib/pickle.py:1234: 'unpickler\'s stack') Use " instead of ' http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode1247 Lib/pickle.py:1247: if self.proto >= 4: Can you add a comment to explain how self.memo is initialized? http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode1248 Lib/pickle.py:1248: if self.memo and type(self.memo) is dict: isinstance(self.memo, dict) http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode1250 Lib/pickle.py:1250: 'of pickle with newer versions') Same with " http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode1263 Lib/pickle.py:1263: raise UnpicklingError('Applying BINPERSID on empty unpickling stack') Reformat to fit in 80 columns. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode1333 Lib/pickle.py:1333: def load_string(self): How come load_string doesn't call self.memoize() here? http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode1580 Lib/pickle.py:1580: raise UnpicklingError('Couldn\'t find class %s.%s' % Quote with " instead http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode1762 Lib/pickle.py:1762: raise UnpicklingError('SETITEMS assignment failed') from e I am not sure if all the extra exception checking is really useful. We would probably just want to wrap load() in a try-except if we want to prevent exceptions other than UnpicklingError. Anyhow, we should benchmark this to make sure it doesn't slow things down. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode1818 Lib/pickle.py:1818: #v4 Remove. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode2015 Lib/pickle.py:2015: ## XXX shouldn't this call pickletools.optimize automatically? No. For many applications, speed is more important than size. http://bugs.python.org/review/15642/diff/5733/Lib/test/test_pickle.py File Lib/test/test_pickle.py (right): http://bugs.python.org/review/15642/diff/5733/Lib/test/test_pickle.py#newcode15 Lib/test/test_pickle.py:15: # to prevent pickletester et. al. to test against the C version Nitpick. It's "et al."
Sign in to reply to this message.
http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py File Lib/pickle.py (right): http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode58 Lib/pickle.py:58: def _t(self, func): #used for pickling methods with REDUCE On 2012/08/14 07:33:25, Alexandre Vassalotti wrote: > Please use a more descriptive name here. And add a docstring explaining how it > is used, instead of the comment. This was named _tie initially, but I thought a less descriptive name with a comment wouldn't hurt too badly since it's a "private" anyway. The smaller name was chosen because this function is pickled whenever a method is pickled (it is used by the unpickler to tie `self' to the unbound version of the method). dis(dumps([].append,4)) 0: \x80 PROTO 4 2: \x93 BINGLOBAL_COMMON '3 _t' 7: ] EMPTY_LIST 8: \x93 BINGLOBAL_COMMON '1 list.append' 22: \x86 TUPLE2 23: R REDUCE 24: . STOP http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode320 Lib/pickle.py:320: raise On 2012/08/14 07:33:25, Alexandre Vassalotti wrote: > Can you explain me the improvement of using BAIL_OUT over raising an exception? > It seems to me that people would much rather have pickle to fail during the > pickling step than at the unpickling step. Well, consider the case when you're using pickle over an internet socket or pipe. Assume one peer runs the pickler on the socket and the other runs the unpickler. If the pickler fails midway it raises an exception and aborts, but it might be that some data has already been flushed and the unpickler has read it. At this point, the unpickler would hang waiting for a STOP opcode that would never come, because one peer has failed without informing the other.
Sign in to reply to this message.
http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py File Lib/pickle.py (right): http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode58 Lib/pickle.py:58: def _t(self, func): #used for pickling methods with REDUCE On 2012/08/14 12:29:50, Stefan M wrote: > This was named _tie initially, but I thought a less descriptive name with a > comment wouldn't hurt too badly since it's a "private" anyway. The smaller name > was chosen because this function is pickled whenever a method is pickled (it is > used by the unpickler to tie `self' to the unbound version of the method). '_bind_method' would be a better name. We shouldn't worry about the length of the string since we can memoize it down to 2 bytes. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode320 Lib/pickle.py:320: raise On 2012/08/14 12:29:50, Stefan M wrote: > Assume one peer runs the pickler on the socket and the other runs the unpickler. > If the pickler fails midway it raises an exception and aborts, but it might be > that some data has already been flushed and the unpickler has read it. At this > point, the unpickler would hang waiting for a STOP opcode that would never come, > because one peer has failed without informing the other. Won't you get the same problem if your fail before emitting the BAIL_OUT opcode? --- e.g., if the network connection is lost. The unpickler side will still need to handle the situation.
Sign in to reply to this message.
http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py File Lib/pickle.py (right): http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode320 Lib/pickle.py:320: raise On 2012/08/14 19:20:23, Alexandre Vassalotti wrote: > On 2012/08/14 12:29:50, Stefan M wrote: > > Assume one peer runs the pickler on the socket and the other runs the > unpickler. > > If the pickler fails midway it raises an exception and aborts, but it might be > > that some data has already been flushed and the unpickler has read it. At this > > point, the unpickler would hang waiting for a STOP opcode that would never > come, > > because one peer has failed without informing the other. > > Won't you get the same problem if your fail before emitting the BAIL_OUT opcode? > --- e.g., if the network connection is lost. The unpickler side will still need > to handle the situation. But network errors are typically handled at a different layer. BAIL_OUT is to signal errors that have occurred during pickling (such as attempting to pickle objects that cannot be pickled), not network errors. If a network error occurs in the pickler, a read error should occur in the unpickler as well. For instance, if you attempt to pickle the tuple (1,os), the pickler will first write: PROTO 4 BININT1 1 and maybe flush these opcodes to the unpickler. It will then try to pickle a module, which is currently not possible (maybe it should be, but it isn't). So the pickler raises an error without writing anything else to the unpickler. However, the unpickler is still waiting for pickle data and has no reason to abort. And it's not like you can send a STOP, because then the unpickler will get: PROTO 4 BININT1 1 STOP which is valid pickle data for pickling the integer 1. So the BAIL_OUT opcode is sent instead to inform the unpickler that it should stop waiting for opcodes and abort the stack and the memo so far, like so: PROTO 4 BININT1 1 BAIL_OUT This opcode would indeed be unnecessary if we would pickle all the data prior to sending it, but this would obviously be a bad idea, so in my opinion there needs to be a way to safely roll back when you've done something stupid. But I have no problem removing it if you still consider it a bad idea.
Sign in to reply to this message.
On 2012/08/17 15:18:23, Stefan M wrote: > > But network errors are typically handled at a different layer. BAIL_OUT is to > signal errors that have occurred during pickling (such as attempting to pickle > objects that cannot be pickled), not network errors. If a network error occurs > in the pickler, a read error should occur in the unpickler as well. I don't think this is a job for pickle but, rather, the network protocol that encapsulates it. pickle is not a network protocol (e.g. it doesn't have framing). Instead of pickling to a socket, you can in most cases call dumps() and send the result over the socket. If you still feel that it is beneficial, I suggest you ask for advice on python-dev.
Sign in to reply to this message.
Fixed some of the stuff suggested in the comments. I don't know how to add new patch sets to rietveld to reflect the changes though. http://bugs.python.org/review/15642/diff/5733/Lib/copyreg.py File Lib/copyreg.py (right): http://bugs.python.org/review/15642/diff/5733/Lib/copyreg.py#newcode91 Lib/copyreg.py:91: #this is because reduce can't work with kwargs at this time. On 2012/08/14 07:33:25, Alexandre Vassalotti wrote: > Can you format your comments like this? > > # Note the lack of ... > > This is mandated by PEP 8: http://www.python.org/dev/peps/pep-0008/#comments > > Also I would rewrite this completely to instead explain the *purpose* of > __newobj_kw__. Done. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py File Lib/pickle.py (right): http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode45 Lib/pickle.py:45: # See http://bugs.python.org/issue15397 On 2012/08/14 07:33:25, Alexandre Vassalotti wrote: > Please include a summary of the issue, like this: > > # Issue 15397: Add pickling support bound method... > # <explain how it work here> Done. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode48 Lib/pickle.py:48: _MethodWrapperType = type([].__add__) On 2012/08/14 07:33:25, Alexandre Vassalotti wrote: > These should added to the 'types' module if they are missing. Yes, these are part of the patch provided in issue15397. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode57 Lib/pickle.py:57: ### On 2012/08/14 07:33:25, Alexandre Vassalotti wrote: > Remove. Done. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode58 Lib/pickle.py:58: def _t(self, func): #used for pickling methods with REDUCE On 2012/08/14 07:33:25, Alexandre Vassalotti wrote: > Please use a more descriptive name here. And add a docstring explaining how it > is used, instead of the comment. Done. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode59 Lib/pickle.py:59: return func.__get__(self) On 2012/08/14 07:33:25, Alexandre Vassalotti wrote: > Add a newline. Done. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode211 Lib/pickle.py:211: BINBYTES64 = b'\x8c' On 2012/08/14 07:33:25, Alexandre Vassalotti wrote: > Can you add comments for these too? Done. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode245 Lib/pickle.py:245: common_modules = None): On 2012/08/14 07:33:25, Alexandre Vassalotti wrote: > Align. And remove spaces around = Done. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode324 Lib/pickle.py:324: def memoize(self, obj, v4_binput=False): On 2012/08/14 07:33:25, Alexandre Vassalotti wrote: > Can you change the name of this argument? We might get a protocol v5 in the > future and that name would be confusing. > > Also, what is the use-case of this argument? It used to have an use-case but it doesn't anymore. Removed. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode339 Lib/pickle.py:339: # Note: As of version 4, this is no longer just a convention. The On 2012/08/14 07:33:25, Alexandre Vassalotti wrote: > Nitpick: Put the note in a separate paragraph. Done. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode349 Lib/pickle.py:349: # memoize automatically. On 2012/08/14 07:33:25, Alexandre Vassalotti wrote: > Redundant with your above note. Done. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode351 Lib/pickle.py:351: # "memos" in pickletools indicate which opcodes memoize (for debugging) On 2012/08/14 07:33:25, Alexandre Vassalotti wrote: > I don't get this. Can you explain? Basically, if save_bytes calls self.memoize() in the pickler, then load_bytes should also call self.memoize() in the unpickler. Because the BINPUT opcodes are not generated anymore, the pickler and the unpickler need to perform memoization in the same places. For instance: dis(dumps( (b'abc', b'abc'), 4)) 0: \x80 PROTO 4 2: C SHORT_BINBYTES 'abc' <-- automatic memoization here 7: h BINGET 0 9: \x86 TUPLE2 <-- and here 10: . STOP The pickler performs memoization at (2) and (9), i.e. in save_bytes and save_tuple. This means that the unpickler must also perform memoization in those places, so load_bytes and load_tuple2 must also call self.memoize. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode897 Lib/pickle.py:897: name_bin = bytes(name , 'utf-8') On 2012/08/14 07:33:25, Alexandre Vassalotti wrote: > Remove aligning spaces. Done. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode941 Lib/pickle.py:941: raise PicklingError('Can\'t pickle bound methods in pickle<4') On 2012/08/14 07:33:25, Alexandre Vassalotti wrote: > You could use "" here to avoid the \ escape. Done. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode958 Lib/pickle.py:958: # v4 On 2012/08/14 07:33:25, Alexandre Vassalotti wrote: > Remove. Done. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode1014 Lib/pickle.py:1014: # save_frozenset within a save_frozenset). Since a is mutable, it has On 2012/08/14 07:33:25, Alexandre Vassalotti wrote: > Since 'a' is mutable... Done. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode1025 Lib/pickle.py:1025: self.write( POP_MARK + self.get(objmemo[0]) ) On 2012/08/14 07:33:25, Alexandre Vassalotti wrote: > No spaces around the parentheses Done. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode1060 Lib/pickle.py:1060: def getattr_recurse (module, name, default=_None): On 2012/08/14 07:33:25, Alexandre Vassalotti wrote: > No space before ( Done. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode1061 Lib/pickle.py:1061: r""" On 2012/08/14 07:33:25, Alexandre Vassalotti wrote: > Why the r-prefix here? I don't know :) Fixed. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode1164 Lib/pickle.py:1164: """ On 2012/08/14 07:33:25, Alexandre Vassalotti wrote: > Format like this: > > """Like self.read(), but... > """ Done. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode1196 Lib/pickle.py:1196: d=dispatch[key[0]] On 2012/08/14 07:33:25, Alexandre Vassalotti wrote: > Use more descriptive name for 'd' and spaces around '='. Changed to dispatch_func, can't think of a good name. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode1198 Lib/pickle.py:1198: raise UnpicklingError('Inexistant opcode '+str(key[0])) On 2012/08/14 07:33:25, Alexandre Vassalotti wrote: > Spaces around + Done. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode1199 Lib/pickle.py:1199: raise On 2012/08/14 07:33:25, Alexandre Vassalotti wrote: > Remove. Done. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode1234 Lib/pickle.py:1234: 'unpickler\'s stack') On 2012/08/14 07:33:25, Alexandre Vassalotti wrote: > Use " instead of ' Done. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode1250 Lib/pickle.py:1250: 'of pickle with newer versions') On 2012/08/14 07:33:25, Alexandre Vassalotti wrote: > Same with " Done. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode1263 Lib/pickle.py:1263: raise UnpicklingError('Applying BINPERSID on empty unpickling stack') On 2012/08/14 07:33:25, Alexandre Vassalotti wrote: > Reformat to fit in 80 columns. Done. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode1333 Lib/pickle.py:1333: def load_string(self): On 2012/08/14 07:33:25, Alexandre Vassalotti wrote: > How come load_string doesn't call self.memoize() here? self.memoize() in the unpickler is only used for pickle>=4 to do the automatic memoization, but load_string never gets called as of pickle3, because the STRING opcode has been superseded by BINUNICODE/BINBYTES. The pickler never generates the STRING opcode as of pickle3 from what I can see. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode1580 Lib/pickle.py:1580: raise UnpicklingError('Couldn\'t find class %s.%s' % On 2012/08/14 07:33:25, Alexandre Vassalotti wrote: > Quote with " instead Done. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode1818 Lib/pickle.py:1818: #v4 On 2012/08/14 07:33:25, Alexandre Vassalotti wrote: > Remove. Done. http://bugs.python.org/review/15642/diff/5733/Lib/test/test_pickle.py File Lib/test/test_pickle.py (right): http://bugs.python.org/review/15642/diff/5733/Lib/test/test_pickle.py#newcode15 Lib/test/test_pickle.py:15: # to prevent pickletester et. al. to test against the C version On 2012/08/14 07:33:25, Alexandre Vassalotti wrote: > Nitpick. It's "et al." Comment was older than code. Removed.
Sign in to reply to this message.
If you upload the diff of your fork against the main repo on the bug, it will get uploaded here automatically after a minute or two. Also, could you make sure your code and comments follow the PEP 8 and PEP 7 style guides? http://bugs.python.org/review/15642/diff/5764/.hgtags File .hgtags (right): http://bugs.python.org/review/15642/diff/5764/.hgtags#newcode107 .hgtags:107: 9dfec5de4ed81398663b726c89c009298151219b pickle4-midterm You should remove these tags eventually. http://bugs.python.org/review/15642/diff/5764/Lib/pickle.py File Lib/pickle.py (right): http://bugs.python.org/review/15642/diff/5764/Lib/pickle.py#newcode58 Lib/pickle.py:58: There is an extra newline here. http://bugs.python.org/review/15642/diff/5764/Lib/pickle.py#newcode61 Lib/pickle.py:61: """used by reduce to pickle methods""" Use a complete sentence with proper capitalization here. http://bugs.python.org/review/15642/diff/5764/Lib/pickle.py#newcode963 Lib/pickle.py:963: # versions earlier than 4 do serialization of sets using reduce, don't Use proper sentences in your comments (including correct capitalization). http://bugs.python.org/review/15642/diff/5764/Lib/test/pickletester.py File Lib/test/pickletester.py (right): http://bugs.python.org/review/15642/diff/5764/Lib/test/pickletester.py#newcod... Lib/test/pickletester.py:559: for proto in range(1,4): Why you changed this? http://bugs.python.org/review/15642/diff/5764/Modules/_pickle.c File Modules/_pickle.c (right): http://bugs.python.org/review/15642/diff/5764/Modules/_pickle.c#newcode2903 Modules/_pickle.c:2903: char global_op; I am pretty sure these should be unsigned. Did you change it to fix a bug?
Sign in to reply to this message.
http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py File Lib/pickle.py (right): http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode26 Lib/pickle.py:26: __version__ = "$Revision$" # Code version On 2012/08/14 07:33:25, Alexandre Vassalotti wrote: > Please remove. We don't use magic version tags in Python. Done. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode60 Lib/pickle.py:60: def _isclassmethod(func): On 2012/08/14 07:33:25, Alexandre Vassalotti wrote: > I think this would work too: > > def _isclassmethod(func): > return hasattr(func, '__self__') and type(func.__self__) is type and \ > type(func) in [MethodType, BuiltinFunctionType] > > > Also, this doesn't work for: > > >>> class A: > ... @classmethod > ... def f(cls): > ... pass > ... > >>> _isclassmethod(A().f) > False > > since > > >>> type(A().f.__self__) > <type 'classobj'> How did you get that? I have >>> type(A().f.__self__) <class 'type'> >>> sys.version '3.3.0b1 (default:ec06b75e0570+, Aug 17 2012, 16:42:26) [MSC v.1600 32 bit (Intel)]' and _isclassmethod(A().f) returns True for me. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode865 Lib/pickle.py:865: # dict.fromkeys is dict.fromkeys = False On 2012/08/14 07:33:25, Alexandre Vassalotti wrote: > It took me a few reads to parse what you are trying to say here. Can you > reformat your comment to make it clearer? Does this sound better? Note: The 'is' operator does not currently work as expected when applied on functions which are classmethods ("dict.fromkeys is dict.fromkeys" is False). Therefore, we only perform the check below if the object we are dealing with ("obj") is not a classmethod. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode1110 Lib/pickle.py:1110: elif hasattr(func, "__self__") and hasattr(func.__self__, "__module__") \ On 2012/08/14 07:33:25, Alexandre Vassalotti wrote: > Why you don't use your _isclassmethod function here? Because not only classmethods can benefit from this "optimization". For some builtins, __module__ is None when it shouldn't be, but __self__.__module__ isn't. >>> random.random.__module__ >>> random.random.__self__.__module__ 'random' So in the past, whichmodule(random.random, 'random.random') would have had to look through sys.modules but now it only has to look in random.random.__self__.__module__. I should probably explain this in a comment. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode2015 Lib/pickle.py:2015: ## XXX shouldn't this call pickletools.optimize automatically? On 2012/08/14 07:33:25, Alexandre Vassalotti wrote: > No. For many applications, speed is more important than size. Removed. http://bugs.python.org/review/15642/diff/5764/.hgtags File .hgtags (right): http://bugs.python.org/review/15642/diff/5764/.hgtags#newcode107 .hgtags:107: 9dfec5de4ed81398663b726c89c009298151219b pickle4-midterm On 2012/08/18 01:09:52, Alexandre Vassalotti wrote: > You should remove these tags eventually. Done. http://bugs.python.org/review/15642/diff/5764/Lib/pickle.py File Lib/pickle.py (right): http://bugs.python.org/review/15642/diff/5764/Lib/pickle.py#newcode58 Lib/pickle.py:58: On 2012/08/18 01:09:52, Alexandre Vassalotti wrote: > There is an extra newline here. My intent was to mark the end of code related to issue 15397. Is there a better way to do it? http://bugs.python.org/review/15642/diff/5764/Lib/pickle.py#newcode61 Lib/pickle.py:61: """used by reduce to pickle methods""" On 2012/08/18 01:09:52, Alexandre Vassalotti wrote: > Use a complete sentence with proper capitalization here. Done. http://bugs.python.org/review/15642/diff/5764/Lib/test/pickletester.py File Lib/test/pickletester.py (right): http://bugs.python.org/review/15642/diff/5764/Lib/test/pickletester.py#newcod... Lib/test/pickletester.py:559: for proto in range(1,4): On 2012/08/18 01:09:52, Alexandre Vassalotti wrote: > Why you changed this? I was playing around to see if the memory leaks are only detected in pickle4. This wasn't supposed to make it in a commit, my bad. Fixed. http://bugs.python.org/review/15642/diff/5764/Modules/_pickle.c File Modules/_pickle.c (right): http://bugs.python.org/review/15642/diff/5764/Modules/_pickle.c#newcode2903 Modules/_pickle.c:2903: char global_op; On 2012/08/18 01:09:52, Alexandre Vassalotti wrote: > I am pretty sure these should be unsigned. Did you change it to fix a bug? I tried to blindly fix the warnings that you've pasted in comments in the issue tracker because I wasn't getting them on msvc. Basically, _Pickler_Write takes a const char* as opposed to a const unsigned char*, causing some signedness warnings when passing &global_op and the like to _Pickler_Write.
Sign in to reply to this message.
http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py File Lib/pickle.py (right): http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode60 Lib/pickle.py:60: def _isclassmethod(func): Ohhh, I probably used Python 2.x when testing that. Sorry for the confusion! http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode351 Lib/pickle.py:351: # "memos" in pickletools indicate which opcodes memoize (for debugging) OK, I see what you meant now. Can you clarify your comment with your new explanation? http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode865 Lib/pickle.py:865: # dict.fromkeys is dict.fromkeys = False Yes. I admit the behaviour of the 'is' operator is really counter-intuitive here, since >>> id(dict.fromkeys) == id(dict.fromkeys) True Maybe you should open a bug about it. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode1333 Lib/pickle.py:1333: def load_string(self): Ok I see. Can you add your explanation as a comment?
Sign in to reply to this message.
http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py File Lib/pickle.py (right): http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode212 Lib/pickle.py:212: SEMISHORT_BINUNICODE = b'\x8d' On 2012/08/14 07:33:25, Alexandre Vassalotti wrote: > Why not BINUNICODE16? Done. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode215 Lib/pickle.py:215: SEMISHORT_BINBYTES = b'\x90' On 2012/08/14 07:33:25, Alexandre Vassalotti wrote: > Same thing here. Done. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode351 Lib/pickle.py:351: # "memos" in pickletools indicate which opcodes memoize (for debugging) On 2012/08/21 23:43:04, Alexandre Vassalotti wrote: > OK, I see what you meant now. Can you clarify your comment with your new > explanation? Done. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode386 Lib/pickle.py:386: On 2012/08/14 07:33:25, Alexandre Vassalotti wrote: > Why not raise an exception instead when this happens? Removed. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode530 Lib/pickle.py:530: raise PicklingError('When pickling a reduced object, ' On 2012/08/14 07:33:25, Alexandre Vassalotti wrote: > The term "reduced object" is confusing. I think we should say: > > When pickling an object via the reduce protocol... Done. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode865 Lib/pickle.py:865: # dict.fromkeys is dict.fromkeys = False On 2012/08/21 23:43:04, Alexandre Vassalotti wrote: > Yes. > > I admit the behaviour of the 'is' operator is really counter-intuitive here, > since > > >>> id(dict.fromkeys) == id(dict.fromkeys) > True > > Maybe you should open a bug about it. Added the new comment. Also, created an issue at http://bugs.python.org/issue15773 http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode964 Lib/pickle.py:964: return self._RefuseDispatchedSave() On 2012/08/14 07:33:25, Alexandre Vassalotti wrote: > I think it would be better to just call save_reduce directly here. Done. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode974 Lib/pickle.py:974: for i, el in enumerate(obj): On 2012/08/14 07:33:25, Alexandre Vassalotti wrote: > Use more descriptive names for bs and el Well, bs is merely a shorthand alias for self._BATCHSIZE so I won't have to type it over and over. I could remove bs and use self._BATCHSIZE directly if you will. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode974 Lib/pickle.py:974: for i, el in enumerate(obj): On 2012/08/14 07:33:25, Alexandre Vassalotti wrote: > And why follow what batch_appends and batch_setitems do? I assume you meant why not follow. Well, I didn't really look at the implementation for _batch_appends when I wrote this, but I don't see an advantage of that approach over this one. I could move the code into a _batch_set for consistency. Also, I could mirror the _batch_appends implementation if you consider it matters. However, that implementation uses an additional temporary container to store each batch, so it's probably less efficient. Mine could be optimized a little if we would choose a BATCHSIZE which is a power of 2. That way computing the modulo is nearly a noop. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode1007 Lib/pickle.py:1007: # Here follows the technique which is also used in save_tuple. Both On 2012/08/14 07:33:25, Alexandre Vassalotti wrote: > Here we follow. Done. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode1009 Lib/pickle.py:1009: # memoized until they are filled. This also means that they can't be On 2012/08/14 07:33:25, Alexandre Vassalotti wrote: > What is filled? Modified the explanation a bit. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode1011 Lib/pickle.py:1011: # e.g.: a=A(); fs=frozenset([a]); a.fs = fs On 2012/08/14 07:33:25, Alexandre Vassalotti wrote: > Format your example. Done. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode1073 Lib/pickle.py:1073: ret = module On 2012/08/14 07:33:25, Alexandre Vassalotti wrote: > Have compared your method with the one posted on issue 13520? > > http://bugs.python.org/issue13520 Just took a quick look now, it appears they are trying to work with __name__ and if that's not working they fall back to __qualname__. My approach tries __qualname__ directly. They argue that their approach has a certain advantage upon looking at the comments, but I'm not sure that's the case. I'll consider this more carefully soon. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode1074 Lib/pickle.py:1074: for n in name.split('.'): On 2012/08/14 07:33:25, Alexandre Vassalotti wrote: > Use more descriptive name for 'n' Done. http://bugs.python.org/review/15642/diff/5733/Lib/pickle.py#newcode1333 Lib/pickle.py:1333: def load_string(self): On 2012/08/21 23:43:04, Alexandre Vassalotti wrote: > Ok I see. > > Can you add your explanation as a comment? I've added it for now, but I'm not sure load_string is the best place to add this comment, since it is not the only opcode in this situation. All obsolete opcodes are.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
