Skip Menu |

Preferred bug tracker

Please visit the preferred bug tracker to report your issue.

This queue is for tickets about the Sereal-Decoder CPAN distribution.

Report information
The Basics
Id: 93609
Status: open
Priority: 0/
Queue: Sereal-Decoder

People
Owner: Nobody in particular
Requestors: zefram [...] fysh.org
Cc:
AdminCc:

Bug Information
Severity: (no value)
Broken in: (no value)
Fixed in: (no value)



Subject: custom op implementation
Date: Fri, 7 Mar 2014 14:18:09 +0000
To: bug-Sereal-Decoder [...] rt.cpan.org
From: Zefram <zefram [...] fysh.org>
Attached patch implements custom op optimisation for Sereal-Decoder. It lacks tests. I ran a simple timing test, with trivial input (thus exaggerating the speedup from avoiding the fixed overheads), and got these results: Rate method function op method 3.1587e+06+-0/s -- -16.0% -26.1% function 3.76037e+06+-0/s 19.0% -- -12.0% op 4.2719e+06+-380/s 35.2% 13.6% -- -zefram

Message body is not shown because sender requested not to inline it.

Subject: Re: [rt.cpan.org #93609] custom op implementation
Date: Fri, 7 Mar 2014 15:29:18 +0100
To: bug-Sereal-Decoder [...] rt.cpan.org
From: demerphq <demerphq [...] gmail.com>
On 7 March 2014 15:18, Zefram via RT <bug-Sereal-Decoder@rt.cpan.org> wrote: Show quoted text
> Fri Mar 07 09:18:25 2014: Request 93609 was acted upon. > Transaction: Ticket created by zefram@fysh.org > Queue: Sereal-Decoder > Subject: custom op implementation > Broken in: (no value) > Severity: (no value) > Owner: Nobody > Requestors: zefram@fysh.org > Status: new > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=93609 > > > > Attached patch implements custom op optimisation for Sereal-Decoder. > It lacks tests. I ran a simple timing test, with trivial input (thus > exaggerating the speedup from avoiding the fixed overheads), and got > these results: > > Rate method function op > method 3.1587e+06+-0/s -- -16.0% -26.1% > function 3.76037e+06+-0/s 19.0% -- -12.0% > op 4.2719e+06+-380/s 35.2% 13.6% --
Ooh. Juicy. Yves -- perl -Mre=debug -e "/just|another|perl|hacker/"
Subject: Re: [rt.cpan.org #93609] custom op implementation
Date: Fri, 7 Mar 2014 14:43:12 +0000
To: demerphq via RT <bug-Sereal-Decoder [...] rt.cpan.org>
From: Zefram <zefram [...] fysh.org>
Just had a "d'oh" moment: the code in pp1_sereal_decode() to return the array of header and body values puts the array itself on the stack, rather than a reference to the array. Need "retvalue = newRV_noinc((SV*)retav)". Obviously that's not being tested. -zefram
Subject: Re: [rt.cpan.org #93609] custom op implementation
Date: Fri, 7 Mar 2014 15:48:32 +0100
To: bug-Sereal-Decoder [...] rt.cpan.org
From: demerphq <demerphq [...] gmail.com>
On 7 March 2014 15:43, Zefram via RT <bug-Sereal-Decoder@rt.cpan.org> wrote: Show quoted text
> Queue: Sereal-Decoder > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=93609 > > > Just had a "d'oh" moment: the code in pp1_sereal_decode() to return the > array of header and body values puts the array itself on the stack, rather > than a reference to the array. Need "retvalue = newRV_noinc((SV*)retav)". > Obviously that's not being tested.
Hey Zefram, Steffen and I just reviewed your patch. We are cautiously optimistic that it can be applied. But frankly, it would be a lot easier applying this if it was commented more. This is territory neither of us are particularly familiar with and we are reluctant to apply a patch we can't maintain ourselves. We realize you probably will be around to help support it, but at the same time we need to be able to support it independently. If it was commented more it would be a lot easier for us to have confidence that we can maintain it. Could you take some time to add some comments and maybe resolve the issue you just mentioned, and supply a new patch? Also, another concern we have is that we want this to be as backwards compatible in terms of perl as possible. How far back will this patch work? thanks for the patch! Yves -- perl -Mre=debug -e "/just|another|perl|hacker/"
Subject: Re: [rt.cpan.org #93609] custom op implementation
Date: Fri, 7 Mar 2014 14:58:02 +0000
To: demerphq via RT <bug-Sereal-Decoder [...] rt.cpan.org>
From: Zefram <zefram [...] fysh.org>
demerphq via RT wrote: Show quoted text
>Could you take some time to add some comments and maybe resolve the >issue you just mentioned, and supply a new patch?
I don't have much clue what you think needs commenting. There's no tricky algorithm at work. If you ask me some specific questions, you can put the answers into comments at your discretion. Show quoted text
>Also, another concern we have is that we want this to be as backwards >compatible in terms of perl as possible. How far back will this patch >work?
It should be fine as far back as Sereal-Decoder already works. I was careful to preserve portability, though I haven't actually tested it. The custom op code itself won't operate prior to 5.13.something, but that only means you don't get the optimisation: you still get working subroutines, so the module API is unchanged. It's possible, by a more complex implementation, to do custom op work with compatibility back to 5.6, but I didn't think it worth the complexity in this case, at least for an initial iteration. -zefram
Subject: Re: [rt.cpan.org #93609] custom op implementation
Date: Fri, 7 Mar 2014 16:14:35 +0100
To: bug-Sereal-Decoder [...] rt.cpan.org
From: demerphq <demerphq [...] gmail.com>
On 7 March 2014 15:58, Zefram via RT <bug-Sereal-Decoder@rt.cpan.org> wrote: Show quoted text
> Queue: Sereal-Decoder > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=93609 > > > demerphq via RT wrote:
>>Could you take some time to add some comments and maybe resolve the >>issue you just mentioned, and supply a new patch?
> > I don't have much clue what you think needs commenting. There's no > tricky algorithm at work.
I understand that, but at the same time there is Deep Knowledge of perl internals at work. Show quoted text
> If you ask me some specific questions, you > can put the answers into comments at your discretion.
If you just went through it chunk by chunk and explained what was up that would be fine. An example of something cryptic we looked at was this: + cv_private = opopt | 0x020200; + CV *cv; + *p++ = '$'; + *p++ = '$'; + if(opopt & OPOPT_OFFSET) { + *p++ = '$'; + cv_private += 0x010100; + } + *p++ = ';'; + if(opopt & OPOPT_DO_BODY) { + *p++ = '$'; + cv_private += 0x010000; + } + if(opopt & OPOPT_DO_HEADER) { + *p++ = '$'; + cv_private += 0x010000; + } What is the significance of "0x020200" and the other constants here? Is p a string containing a prototype? The XS subs we had didn't have prototypes. The sub ck_entersub_args_sereal_decoder contains a lot of logic, I would say if you commented it pretty much line by line as to what the intent is it would make our job a lot easier. I mean, assume we know nothing about custom ops. (Steffen knows more than I do, but neither of us are really confident about it). Sorry for asking for this, but we need more to be able to maintain this if you decide to migrate to Argentina and open a bar on the beach :-) Show quoted text
>
>>Also, another concern we have is that we want this to be as backwards >>compatible in terms of perl as possible. How far back will this patch >>work?
> > It should be fine as far back as Sereal-Decoder already works. I was > careful to preserve portability, though I haven't actually tested it. > The custom op code itself won't operate prior to 5.13.something, but > that only means you don't get the optimisation: you still get working > subroutines, so the module API is unchanged. It's possible, by a more > complex implementation, to do custom op work with compatibility back to > 5.6, but I didn't think it worth the complexity in this case, at least > for an initial iteration.
We are fine with it "working" in older perl's, it doesnt have to be custom-op fast. But we do need it to "Just Work" on relatively old perls (we support back to 5.8.5). cheers, Yves -- perl -Mre=debug -e "/just|another|perl|hacker/"
Subject: Re: [rt.cpan.org #93609] custom op implementation
Date: Fri, 7 Mar 2014 15:33:35 +0000
To: demerphq via RT <bug-Sereal-Decoder [...] rt.cpan.org>
From: Zefram <zefram [...] fysh.org>
demerphq via RT wrote: Show quoted text
>What is the significance of "0x020200" and the other constants here?
The cv_private value incorporates flags describing the operation to be performed by the sub and precomputed arity limits. 0x020200 corresponds to min_arity=2 and max_arity=2. The various additions to cv_private increment one or both of these sub-values. The six subs created there share a single C body function, and are differentiated only by the option flags in cv_private. The custom ops likewise share one op_ppaddr function, and the operations they perform are differentiated by the same flags, stored in op_private. Show quoted text
>Is p a string containing a prototype? The XS subs we had didn't have prototypes.
Yes, the subs have prototypes. The protoypes have no effect when the subs are used as methods, so there's no break of compatibility for those using the documented API. There is a change that could be detected by code such as "Sereal::Decoder::decode($dec, @v)", that uses the methods directly in an undocumented way. The prototype, specifically the putting of argument expressions into scalar context, is required in order to be able to resolve arity at compile time. If this wasn't done, there would have to be a pushmark op preceding the argument ops, and pp_sereal_decode() would need the same code as xsfunc_sereal_decode() to check arity and resolve the optional-parameter flags. Show quoted text
>The sub ck_entersub_args_sereal_decoder contains a lot of logic, I >would say if you commented it pretty much line by line as to what the >intent is it would make our job a lot easier.
It's pulling apart a standard entersub op tree. The variable names give quite a few clues. To understand the code, it's more useful to look at a B::Concise rendition of an op tree than to describe the action line-by-line. Show quoted text
> we do need it to "Just Work" on relatively old >perls (we support back to 5.8.5).
It should Just Work that far back. -zefram
Subject: Re: [rt.cpan.org #93609] custom op implementation
Date: Fri, 7 Mar 2014 16:43:26 +0000
To: demerphq via RT <bug-Sereal-Decoder [...] rt.cpan.org>
From: Zefram <zefram [...] fysh.org>
Another "d'oh": THX_pp1_sereal_decode() needs a pTHX_. -zefram
Subject: Re: [rt.cpan.org #93609] custom op implementation
Date: Mon, 10 Mar 2014 08:36:31 +0100
To: bug-Sereal-Decoder [...] rt.cpan.org
From: demerphq <demerphq [...] gmail.com>
Please review "custom_op" branch. I have not merged your comments yet. Yves On 7 March 2014 15:18, Zefram via RT <bug-Sereal-Decoder@rt.cpan.org> wrote: Show quoted text
> Fri Mar 07 09:18:25 2014: Request 93609 was acted upon. > Transaction: Ticket created by zefram@fysh.org > Queue: Sereal-Decoder > Subject: custom op implementation > Broken in: (no value) > Severity: (no value) > Owner: Nobody > Requestors: zefram@fysh.org > Status: new > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=93609 > > > > Attached patch implements custom op optimisation for Sereal-Decoder. > It lacks tests. I ran a simple timing test, with trivial input (thus > exaggerating the speedup from avoiding the fixed overheads), and got > these results: > > Rate method function op > method 3.1587e+06+-0/s -- -16.0% -26.1% > function 3.76037e+06+-0/s 19.0% -- -12.0% > op 4.2719e+06+-380/s 35.2% 13.6% -- > > -zefram >
-- perl -Mre=debug -e "/just|another|perl|hacker/"
Subject: Re: [rt.cpan.org #93609] custom op implementation
Date: Wed, 12 Mar 2014 13:13:07 +0000
To: demerphq via RT <bug-Sereal-Decoder [...] rt.cpan.org>
From: Zefram <zefram [...] fysh.org>
demerphq via RT wrote: Show quoted text
>Please review "custom_op" branch. I have not merged your comments yet.
Looks fine from inspection, and passes tests for me on various Perls from 5.8.8 to 5.19.8. -zefram