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

Improve frame and traceback pickling #107

Closed
ghost opened this issue Dec 13, 2016 · 7 comments
Closed

Improve frame and traceback pickling #107

ghost opened this issue Dec 13, 2016 · 7 comments

Comments

@ghost
Copy link

ghost commented Dec 13, 2016

Originally reported by: Anselm Kruis (Bitbucket: akruis, GitHub: akruis)


Issue #62 discusses problems caused by the non recursive pickling of frames. In the end I closed #62 as "won't fix", because the proposed solution (recursive frame pickling) had compatibility problems.

But there is still room for improvements:

  1. Linkage of frames in unpickled tracebacks. It is fairly simple to reconstruct the linkage for the inner frames - as returned by inspect.getinnerframes(tb) - of a traceback. I already coded a test case and a patch to static PyObject * tb_setstate(PyObject *self, PyObject *args) in prickelpit.c. This could go into 2.7-slp, because it shouldn't cause any compatibility problems.

  2. In issue pickling of frames without a tasklet #62 we discussed the idea of a FrameReducer, that would avoid the requirement to add a reduce function for frames to copy_reg. See https://bitbucket.org/stackless-dev/stackless/issues/62/pickling-of-frames-without-a-tasklet#comment-10422733. I'll try to implement this proposal. I'm undecided, if this is a feature for 2.7-slp, but if it works well, it should definitely go into 3.4-slp. And it will help to fix 3.5-slp.


@ghost
Copy link
Author

ghost commented Dec 18, 2016

Original comment by Anselm Kruis (Bitbucket: akruis, GitHub: akruis):


I created two pull requests:

  1. pull request slpmodule_new() doesn't initialize extra members added by type_new() #22 for the traceback linkage. This PR has already been merged into 2.7-slp and 3.x-slp.

  2. pull request enhance tasklet.bind() #28 introduces the FameReducer. It is for 3.4-slp and default-slp only.

@ghost
Copy link
Author

ghost commented Dec 20, 2016

Original comment by Anselm Kruis (Bitbucket: akruis, GitHub: akruis):


I merged pull request #22 (part 1 of this issue). It is a clear win with a low risk and independent of part 2.
Now we have no more open issues for 2.7-slp and can pull in v2.7.13 which has been released a few days ago.

@ghost
Copy link
Author

ghost commented Apr 7, 2017

Original comment by Kristján Valur Jónsson (Bitbucket: krisvale, GitHub: kristjanvalur):


I know that all of this was sort of discussed three years ago, in issue #62, but.
well, aren't we making too much of a hypothetical compatibility problem?
Currently trackebacks cannog be unpickled. What could break? Are there use cases where you would want to unpickle frames from different versions?
In stackless, we are in a niche, and can afford to be a bit more generous with compatibility. We should be careful not to over-complicate things if there is no reason to.

@ghost
Copy link
Author

ghost commented Apr 7, 2017

Original comment by Anselm Kruis (Bitbucket: akruis, GitHub: akruis):


@krisvale I agree with you to not over-complicate things. But recursive frame pickling breaks code, that is in productive usage at our customers. Initially I proposed recursive frame pickling, but it didn't work out. The problem is not unpickling, the problem is pickling: recursive frame pickling pickles more frames and if one of the additional frames refers an unpickleable object, pickling fails.

Therefore there are two options:

  1. keep the current implementation
  2. merge pull request enhance tasklet.bind() #28

I'm not completely up to date with respect to the latest developments of async methods in C-Python 3.x, but I assume, that C-Python will add some pickling support for async code sooner or later. Then it would be better not to get in the way. Therefore I'm in favour of the second option.

@ghost
Copy link
Author

ghost commented Apr 7, 2017

Original comment by Kristján Valur Jónsson (Bitbucket: krisvale, GitHub: kristjanvalur):


Right, I understand. Unpicklable things are annoying.
I suppose it is best to pretend that frames can't be pickled, as you say. But if we do pickle them (as part of a tasklet), then I would recommend recursive pickling just to adhere to conventions as much as possible.

@ghost
Copy link
Author

ghost commented Apr 15, 2017

Original comment by Anselm Kruis (Bitbucket: akruis, GitHub: akruis):


During my work on issue #127 I wrote tests for copy.copy(). They failed for gernerator and traceback objects, because their __setstate__() method (in _stackless._wrap....) received a _FrameWrapper-object instead of a frame-object. This happens, because copy.copy() creates a shallow copy.

As a consequence I modified the setstate-methods to accept a _FrameWrapper object as an alternative to a frame-object. Change is 54d3428.

@ghost
Copy link
Author

ghost commented Apr 15, 2017

Original comment by Anselm Kruis (Bitbucket: akruis, GitHub: akruis):


Final commits for part 1 (frame.f_back linkage in unpickled tracebacks) are:

  • 2.7-slp: 3719f4e0d437
  • 3.3-slp: 3de42ba10d7b
  • 3.4-slp: 07a6504acae0
  • default-slp: 61c15cee8ba0

Branch 3.5-slp didn't exists then.

Final commits for part 2 (disable frame pickling) are:

  • 3.4-slp: 54d3428930cd
  • 3.5-slp: eff69deacad2

Currently the branch default-slp is behind 3.5-slp. Therefore it is not yet possible to merge eff69deacad2 into default-slp.

@ghost ghost added this to the 2.7.13-slp milestone Sep 24, 2017
@ghost ghost closed this as completed Sep 24, 2017
akruis pushed a commit that referenced this issue Oct 29, 2017
…d traceback objects

__setstate__ must accept the state returned by __reduce__. This was not the case for generator
and trace-back objects. This commit fixes this. The next commit (merge of issue #127) adds the relevant test cases.
Additionally amends changelog.txt.

https://bitbucket.org/stackless-dev/stackless/issues/107
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

0 participants