develooper Front page | perl.perl5.porters | Postings from May 2013

Storable refactoring, was Re: [perl #118139] Storable in DESTROYblocks

Thread Previous | Thread Next
From:
Salvador Fandino
Date:
May 29, 2013 17:22
Subject:
Storable refactoring, was Re: [perl #118139] Storable in DESTROYblocks
Message ID:
20130529172219.10882.qmail@lists-nntp.develooper.com
On 05/23/2013 05:30 PM, rurban @ cpanel . net wrote:
> # New Ticket Created by  rurban@cpanel.net 
> # Please include the string:  [perl #118139]
> # in the subject line of all future correspondence about this issue. 
> # <URL: https://rt.perl.org:443/rt3/Ticket/Display.html?id=118139 >
> 
> 
> 
> This is a bug report for perl from rurban@cpanel.net,
> generated with the help of perlbug 1.39 running under perl 5.18.0.
> 
> 
> -----------------------------------------------------------------
> Storable segfaults when being used in DESTROY blocks during global
> destruction, when accessing an already freed PL_modglobal or the internal
> ctx.
> The best fix is to die if used in global destruction.
> See the new test t/destroy.t, which crashes in all perl versions.
> 
> Since this Storable fix needs to be released on CPAN also, I had to 
> add some compatibility fixes, tested back to 5.6.

I have been working for some time in refactoring Storable. See

  https://github.com/salva/perl5/tree/improve_storable

And a wrapped version for CPAN distribution:

  https://github.com/salva/p5-Storable

It is still a work in progress and still has some bugs (more notably it
doesn't work on 32 bit platforms where perl has been compiled with
use64bitint set).

Initially my plan was to send the patch to p5p once those bugs have been
solved but as this bug report is here and people is already trying to
solve it, I think is a good time to tell the world about my work on
Storable :-)

You can see the commits for a detailed history of the changes. But
briefly they are as follows:

- Move the context object into the C stack, so no more DESTROYing it!

- Break the context into a two structures store_cxt and retrieve_cxt.

- Use a perl SV as the output buffer when storing to memory (the old
structure has a O(N^2) performance.

- Convert all the IO macros into static C functions and let the compiler
optimize them when required (last time I looked the .so was 30% smaller)

- use Perl_croak to fail instead of returning 1 (for store) or NULL (for
retrieve) and propagate it all the way up.

- mortalize everything when required so that croak'ing doesn't leak memory.

- several memory leaks fixed.

- ptr_tables back-ported to old perls so that conditional code can be
removed.

- lots of premature optimizations removed

- lots of code de-duplication



I think the most important change is moving the context object into the
C stack because it was and absolute nightmare:

It seems that initially, probably to simplify the code, Storable used
some global variables to keep the context while recursing (note that the
context is reset in every call so no data is shared between successive
calls to store or retrieve functions).

Then, perl got support for threads and the context was moved to thread
storage and the funny thing is that, probably for performance reasons,
also passed from call to call when recursing!

Then, Storable got the hooks STORABLE_freeze and STORABLE_thaw, so in
order to make it reentrant, the context was moved into some kind of
perl-thread stack.

Then lots of work-arounds were added to solve common errors and memory
leaks.

I believe it is the worst case of code rot I have ever seen!!!


With my changes, the context is created when store or freeze functions
are called on the C stack and passed from function to function when
recurring. The temporal structures are mortalized so that they are freed
when the code comes back to Perl-land.

I might have introduced new bugs but IMO, now, the code is pretty much
simpler and bugs should also be much easier to fix than before.


If you agree that having that into blead is a good idea, I think that
the way to follow is to release it to CPAN as a development version
first so that it can be tested (I have been testing it myself with all
the latest perls on the 5.6 5.8, 5.10, 5.12, 5.14, 5.16, 5.18 and 5.19
series with and without threads)



Thread Previous | Thread Next


nntp.perl.org: Perl Programming lists via nntp and http.
Comments to Ask Bjørn Hansen at ask@perl.org | Group listing | About