4. Do #3 but factor out everything but updateNode() from that code [into putVars or packVars] so it becomes

sub setVars {
    my($NODE,$vars)= @_;
    putVars($NODE,"vars",$vars);
    updateNode($NODE,-1);
}
# or
sub setVars {
    my($NODE,$vars)= @_;
    $NODE->{vars}= packVars($vars);
    updateNode($NODE,-1);
}
[download]

Yes, I'd rather give setVars() a name that more clearly conveys that it will updateNode(), but I don't want to change every chunk of code that is already using setVars().

Ok. I prefer your second option. Given the node cache, anytime you set a field you've already begun the process of updating and the updateNode should be close by.

To continue on from our CB conversation id like to see getVars() and setVars() bullet proofed. Currently its too possible to pollute the data. I propose switching to a new robust encoding (thats fast as well) and putting a version code on the strings. Something like '===01' as a magic number to start the strings (since this cant occur currently if I understand things right). Then convert to the new system on write. That way so long as folks use getVars() and setVars() they will always get their data through. This would resolve a longstanding nit of mine with settings. As I said before I had to rename Q&A_Section to QandASection to put it in the settings, likewise we've had trouble in the past with categorized answer and categorized question and underlining, deunderlining etc. Setting need/could need to store non \w chars all the time.

Sorry, a bit of free thought there for you...

I'd rather not have versioned strings if we can avoid it. Could you run a dumper prompt to see if there are currently any keys containing "%"? (Select nodes with vars containing '%' and check grep /%/, keys %{getVars($N)} .)

Update: also check for '+'

I don't understand why people are reluctant to use techniques like version codes here. (I say this because ive noticed others also dislike version codes.) To me a settings field is just a data format which is really a file format. And its usually better to code version strings into such systems early while its easy. The flexibility such a mechanism offers in a situation like here is quite nice if you ever have reason to change the way things work. And I've had call to use it elsewhere on PM to good effect already.

# note that these two vars go together to a certain extent.
my $magic="===&01";
my $escape="%=&";

sub unpack_hash {
  my ($str)=shift;
  my %hash;

  if (substr($str,0,6) eq $magic) {
    substr($str,0,6)='';
    %hash=map { map { 
                      s/%(\d)/substr($escape,$1,1)/ge;
                      $_
                   } split /=/,$_ 
                 } split /&/,$str;
    
  } else {
    # old getVars() support...
  }
}

sub pack_hash {
  my $hash=shift;
  return $magic.join("&",map { 
    join "=",map { 
         (my $v=$_)=~s/([$escape])/'%'.index($escape,$1)/ge 
         $v
    } $_ => $hash{$_}     
  } sort keys %$hash);
}
[download]

Since bullet proofing getVars() probably means it faster to do than the current method, I personally dont see why we shouldn't do it.



---
demerphq


First they ignore you, then they laugh at you, then they fight you, then you win.
-- Gandhi

Flux8
---
• Update:  
Note that only a sliver of extra code would allow using Data::Dumper for the values if requested to do so (with only a tiny overhead were the hash values reference free). Ultimately that would be where I would go. Encode the keys with extra info when packed to indicate their value needs thawing. Anyway.... :-)

Let me know what your requirements are for diff_strings and ill see what I can do to help (figure it) out. :-)

I tried to increase readability of the stored string (which seemed important to demerphq) by only escaping the necessary characters &, =, and %. I did not consider using unprintable characters and am a little uneasy about not escaping any there are in the data; I'd prefer it to be always all printable. This also meant bypassing calls to escape/unescape which would slow things down with a lot of vars.

cmpVars is what we need for current patch detection to work reliably; I haven't tested it at all yet.

sub packVars
{
    my $varsref = $_[0];

    # current format version: 01
    return join "&", "==01",
        map {
            my $typ;
            my $v = $varsref->{$_};

            # special data to pack?
            if (ref $v) {

                # only hash refs supported now
                $typ = "H";
                $v = packVars( $v );
            }
            # undef becomes empty, protect empty values
            elsif (!$v) {
                $v = $typ = '';
            }

            join '=',
                map {s/([%&=])/ sprintf '%%%02x', ord($1) /ge; $_}
                    $_, $v, (defined $typ ? $typ : ());
        } sort keys %$varsref;
}

sub unpackVars
{
    my $vars_str = $_[0];
    my $format_version = "00";

    # version 00: original format
    # version 01: keys are escaped, not just values
    $format_version = $1 if $vars_str =~ s/^==(\d\d)&//;

    return {} unless $vars_str;

    my %vars;

    if ($format_version eq "01") {
        for (split /&/, $vars_str) {
            my ($k,$v,$typ) =
                map { s/%(\w\w)/ chr(hex($1)) /ge; $_ } split /=/, $_,
+ -1;

            # special data to unpack?
            if ($typ) {
                # nested hash
                if ($typ eq "H") {
                    $v = unpackVars($v);
                }
            }
            $vars{$k} = $v;
        }
    }
    # format version "00"
    else {
        %vars = map split(/=/, $_, 2), split /&/, $vars_str;
        unescape( values %vars );
        $vars{$_} eq ' ' and $vars{$_} = '' for keys %vars;
    }

    return \%vars;
}

sub cmpVars
{
    my ($var1str, $var2str) = @_;

    # return false immediately if strings match,
    # otherwise return true if both are current format version,
    # otherwise compare current format version.

    return $var1str cmp $var2str
        && ( $var1str =~ /^==01&/ && $var2str =~ /^==01&/ )
        || packVars( unpackVars ( $var1str ) )
        cmp packVars( unpackVars ( $var2str ) );
}
[download]

I took the liberty of taking your code and running with it. Unsurprisingly I added handling arrays, (Which required a modification to your packing scheme, type is tagged at the root level, right after the version string and not at the key/value level as you had it.) As well as code to prevent possible accidental infinite recursion. Additionally I used my personal settings as a test set and benchmarked the two approaches to see what the comparative perfomance was. The results were very interesting. First off, if we rip out DB related code and benchmark the two variants we find that the results are as follows:

       Rate   old  new
old   115/s    -- -47%
new   215/s   87%   --
[download]

When i then did the benchmark on the server _with_ update code the perfomance changes to as follows:

     Rate  old  new
old 107/s   -- -61%
new 271/s 153%   --
[download]

These results were somewhat gratifying to me as they appear to validate my point that by using a more compact encoding we reduce the size of the stored data and thus the time required to marshal that data back and forth to the db server. (The update was forced each time by incrementing a counter in the stored hash.) These are the lengths and first chars of the two packed vars:

L: 11549>external_user=demerphq&scratchpublic=%20
L: 6902 >==01:H&DomainNodeletExtras=&allow_dupe_p
[download]

So overall I think this code is worthy. We probably pack and unpack four or five vars per page fetch. Reducing the load this takes has got to be a good thing for all concerned.

Home test code (needs DDS to be installed)

Read more... (6 kB)

And the Dumper Prompt Benhcmark Code (in IE this means you have to save the response as a file as the cmpthese output comes before the html, havent figured out a better way to do this yet.)

Read more... (5 kB)


---
demerphq


First they ignore you, then they laugh at you, then they fight you, then you win.
-- Gandhi

Flux8

Played with it a little, mostly reindenting; removed the eval checks: we aren't preserving blessed refs, so no point in even allowing them. Put the hash check first in packVars. Don't need to escape = for arrays (unless I'm missing something).(Update: added back =). Started to mess with cmpVars but ran out of time. My inclination would be just to treat too high version numbers as if they were the current rather than return an error. All the error conditions should get logged rather than die or return.
Read more... (3 kB)

Your benchmarks show that adding the DB update work to the new method makes it run about 30% faster. This is a red flag and you should explain it before accepting the results.

I did scan the code quickly and didn't see an obvious reason, but the code is hard to read from here.

- tye        

Will look more at this soon, but I wanted to note that because of the lack of sort, cmpVars can't rely on just cmp when comparing version 0 strings.