Thread: WIP: Make timestamptz_out less slow.

Hi,

I recently once more noticed that timestamptz_out is really, really
slow. To quantify that, I created a bunch of similar sized tables:

CREATE TABLE tbl_timestamp AS SELECT NOW() FROM generate_series(1, 100000) a, generate_series(1, 100) b;
CREATE TABLE tbl_int8 AS SELECT 1::bigint FROM generate_series(1,  100000) a, generate_series(1, 100) b;
CREATE TABLE tbl_bytea AS SELECT '   '::bytea FROM generate_series(1, 100000) a, generate_series(1, 100) b;

These all end up being 346MB large.

COPY tbl_bytea TO '/dev/null';
Time: 1173.484 ms
COPY tbl_int8 TO '/dev/null';
Time: 1030.756 ms
COPY tbl_timestamp TO '/dev/null';
Time: 6598.030

(all best of three)

Yes, timestamp outputs more data as a whole, but being 5 times as slow
is still pretty darn insane. To make sure that's not the cause, here's
another table:

CREATE TABLE tbl_timestamptext AS SELECT NOW()::text FROM generate_series(1, 100000) a, generate_series(1, 100) b;
COPY tbl_timestamptext TO '/dev/null';
Time: 1449.554 ms

So it's really just the timestamp code.


Profiling it shows:
  Overhead  Command         Shared Object     Symbol
  -   38.33%  postgres_stock  libc-2.19.so      [.] vfprintf
     - 97.92% vfprintf
          _IO_vsprintf
        - sprintf
           + 70.25% EncodeDateTime
           + 29.75% AppendSeconds.constprop.10
     + 1.11% _IO_default_xsputn
  -    8.22%  postgres_stock  libc-2.19.so      [.] _IO_default_xsputn
     - 99.43% _IO_default_xsputn
        - 90.09% vfprintf
             _IO_vsprintf
           - sprintf
              + 74.15% EncodeDateTime
              + 25.85% AppendSeconds.constprop.10
        + 9.72% _IO_padn
     + 0.57% vfprintf
  +   7.76%  postgres_stock  postgres_stock    [.] CopyOneRowTo

So nearly all the time is spent somewhere inside the sprintf calls. Not
nice.

The only thing I could come up to make the sprintfs cheaper was to
combine them into one and remove some of the width specifiers that
aren't needed. That doesn't buy us very much.

I then proceeded to replace the sprintf call with hand-rolled
conversions. And wow, the benefit is far bigger than I'd assumed:
postgres[7236][1]=# COPY tbl_timestamp TO '/dev/null';
Time: 2430.521 ms

So, by hand-rolling the ISO conversion in EncodeDateTime() we got a
~250% performance improvement. I'd say that's worthwhile.

The attached patch shows what I did. While there's some polishing
possible, as a whole, it's pretty ugly. But I think timestamp data is so
common that it's worth the effort.

Does anybody have a fundamentally nicer idea than the attached to
improvide this?

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> So nearly all the time is spent somewhere inside the sprintf calls. Not
> nice.

What happens if you force use of port/snprintf.c instead of glibc's
version?
        regards, tom lane

On 2015-07-27 17:31:41 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > So nearly all the time is spent somewhere inside the sprintf calls. Not
> > nice.
> 
> What happens if you force use of port/snprintf.c instead of glibc's
> version?

Good question.

Even worse. 15900.014 ms.

Andres

Andres Freund <andres@anarazel.de> writes:
> On 2015-07-27 17:31:41 -0400, Tom Lane wrote:
>> What happens if you force use of port/snprintf.c instead of glibc's
>> version?

> Even worse. 15900.014 ms.

Interesting.  So as a separate optimization problem, we might consider
"try to put snprintf.c at least on a par with glibc".  I'm kind of
surprised by this result really, since snprintf.c lacks a lot of the
bells and whistles that are in glibc.
        regards, tom lane

On 2015-07-28 10:59:15 +1200, David Rowley wrote:
> It won't be quite as fast as what you've written, but I think it will be
> much neater and more likely to be used in other places if we invent a
> function like pg_ltoa() which returns a pointer to the new end of string.
> 
> Also if we're specifying padding with zeros then we can skip the reverse
> part that's in pg_ltoa(), (normally needed since the numeric string is
> build in reverse)
> 
> The code could then be written as:
> 
> str = pg_int2str_pad(str, year, 4);
> *str++ = '-';
> str = pg_int2str_pad(str, tm->tm_mon, 2);
> *str++ = '-';
> str = pg_int2str_pad(str, tm->tm_mday, 2);
> 
> etc
> 
> I've used this method before and found it to be about 10 times faster than
> snprintf(), but I was reversing the string, so quite likely it be more than
> 10 times.

Yes, that might be worthwhile to try. Certainly would look less
ugly. Willing to give it a try?

> I'm interested to see how much you're really gaining by manually unrolling
> the part that builds the fractional part of the second.

It seems to help a fair amount, but this really was more a quick POC
than something serious. My theory why it helps is that each loop
iteration is independent of the previous one and thus can take full
advantage of the pipeline.

Regards,

Andres

On 2015-07-29 03:10:41 +1200, David Rowley wrote:
> timestamp_out() = 2015-07-29 02:24:33.34 in 3.506000
> timestamp_out_old() = 2015-07-29 02:24:33.034 in 64.518000
> timestamp_out_af() = 2015-07-29 02:24:33.034 in 2.981000
>
> timestamp_out_old is master's version, the timestamp_out_af() is yours, and
> timestamp_out() is my one. times are in seconds to perform 100 million
> calls.

That looks good.

> So it appears your version is a bit faster than mine, but we're both about
> 20 times faster than the current one.
> Also mine needs fixed up as the fractional part is not padded the same as
> yours, but I doubt that'll affect the performance by much.

Worthwhile to finish that bit and try ;)

> My view: It's probably not worth going quite as far as you've gone for a
> handful of nanoseconds per call, but perhaps something along the lines of
> mine can be fixed up.

Yes, I agreee that your's is probably going to be fast enough.

> Have you thought about what to do when HAVE_INT64_TIMESTAMP is not defined?

I don't think it's actually important. The only difference vs float
timestamps is that in the latter case we set fsecs to zero BC.

Unless we want to slow down the common case it seems not unlikely that
we're going to end up with a separate slow path anyway. E.g. neither
your version nor mine handles 5 digit years (which is why I fell back to
the slow path in that case in my patch).

Regards,

Andres

On 2015-07-29 09:37:26 +1200, David Rowley wrote:
> On 29 July 2015 at 03:25, Andres Freund <andres@anarazel.de> wrote:
> 
> > On 2015-07-29 03:10:41 +1200, David Rowley wrote:
> > > Have you thought about what to do when HAVE_INT64_TIMESTAMP is not
> > defined?
> >
> > I don't think it's actually important. The only difference vs float
> > timestamps is that in the latter case we set fsecs to zero BC.
> >
> >
>  I was also thinking that the % 10 won't work when fsec_t is double.
> 
> typedef double fsec_t

It seems quite possible to move that bit to timestamp2tm and do it
without a dependency on HAVE_INT64_TIMESTAMP. As long as it doesn't slow
down the int timestamp case I'm happy to simplify code in this area.

Greetings,

Andres Freund

On 2015-08-09 12:47:53 +1200, David Rowley wrote:
> I took a bit of weekend time to finish this one off. Patch attached.
> 
> A quick test shows a pretty good performance increase:
> 
> create table ts (ts timestamp not null);
> insert into ts select generate_series('2010-01-01 00:00:00', '2011-01-01
> 00:00:00', '1 sec'::interval);
> vacuum freeze ts;
> 
> Master:
> david=# copy ts to 'l:/ts.sql';
> COPY 31536001
> Time: 20444.468 ms
> 
> Patched:
> david=# copy ts to 'l:/ts.sql';
> COPY 31536001
> Time: 10947.097 ms

Yes, that's pretty cool.

> There's probably a bit more to squeeze out of this.
> 1. EncodeDateTime() could return the length of the string to allow callers
> to use pnstrdup() instead of pstrdup(), which would save the strlen() call.
> 2. Have pg_int2str_zeropad() and pg_int2str() skip appending the NUL char
> and leave this up to the calling function.
> 3. Make something to replace the sprintf(str, " %.*s", MAXTZLEN, tzn); call.
> 
> Perhaps 1 and 3 are worth looking into, but I think 2 is likely too error
> prone for the small gain we'd get from it.

I'm inclined to first get the majority of the optimization - as in
somethign similar to the current patch. If we then feel a need to
optimize further we can do that. Otherwise we might end up not getting
the 95% performance improvement in 9.6 because we're playing with the
remaining 5 ;)


> Also I was not too sure on if pg_int2str() was too similar to pg_ltoa().
> Perhaps pg_ltoa() should just be modified to return the end of string?

I don't think the benefits are worth breaking pg_ltoa interface.

>  /*
> - * Append sections and fractional seconds (if any) at *cp.
> + * Append seconds and fractional seconds (if any) at *cp.
>   * precision is the max number of fraction digits, fillzeros says to
>   * pad to two integral-seconds digits.
>   * Note that any sign is stripped from the input seconds values.
> + * Note 'precision' must not be a negative number.
>   */
> -static void
> +static char *
>  AppendSeconds(char *cp, int sec, fsec_t fsec, int precision, bool fillzeros)
>  {
> +#ifdef HAVE_INT64_TIMESTAMP

Wouldn't it be better to just normalize fsec to an integer in that case?
Afaics that's the only remaining reason for the alternative path?

> +/*
> + * pg_int2str
> + *        Converts 'value' into a decimal string representation of the number.
> + *
> + * Caller must ensure that 'str' points to enough memory to hold the result
> + * (at least 12 bytes, counting a leading sign and trailing NUL).
> + * Return value is a pointer to the new NUL terminated end of string.
> + */
> +char *
> +pg_int2str(char *str, int32 value)
> +{
> +    char *start;
> +    char *end;
> +
> +    /*
> +     * Handle negative numbers in a special way. We can't just append a '-'
> +     * prefix and reverse the sign as on two's complement machines negative
> +     * numbers can be 1 further from 0 than positive numbers, we do it this way
> +     * so we properly handle the smallest possible value.
> +     */
> +    if (value < 0)
> +    {

We could alternatively handle this by special-casing INT_MIN, probably
resulting in a bit less duplicated code.
>  
>  /*
>   *        Per-opclass comparison functions for new btrees.  These are
> diff --git a/src/tools/msvc/build.pl b/src/tools/msvc/build.pl
> index e107d41..1e2dd62 100644
> --- a/src/tools/msvc/build.pl
> +++ b/src/tools/msvc/build.pl
> @@ -61,7 +61,7 @@ elsif ($buildwhat)
>  }
>  else
>  {
> -    system("msbuild pgsql.sln /verbosity:detailed /p:Configuration=$bconf");
> +    system("msbuild pgsql.sln /verbosity:quiet /p:Configuration=$bconf");
>  }

Uh? I assume that's an acciental change?

Greetings,

Andres Freund

On 2015-09-03 16:28:40 +1200, David Rowley wrote:
> > Wouldn't it be better to just normalize fsec to an integer in that case?
> > Afaics that's the only remaining reason for the alternative path?
> >
> > A special case would need to exist somewhere, unless we just modified
> timestamp2tm() to multiply fsec by 1 million when HAVE_INT64_TIMESTAMP is
> not defined, but that changes the function signature.

Sure, but that's a one line #ifdef?

> > We could alternatively handle this by special-casing INT_MIN, probably
> > resulting in a bit less duplicated code.
> >
> 
> Those special cases seem to do some weird stuff to the performance
> characteristics of those functions. I find my method to handle negative
> numbers to produce much faster code.

That's a bit odd.

> I experimented with finding the fastest way to convert an int to a string
> at the weekend, I did happen to be testing with int64's but likely int32
> will be the same.
> 
> This is the time taken to convert 30 into "30" 2 billion times.
> 
> The closest implementation I'm using in the patch is pg_int64tostr() one.
> The pg_int64tostr_memcpy() only performs better with bigger numbers /
> longer strings.
> 
> david@ubuntu:~/C$ gcc5.2 tostring.c -o tostring -O2
> david@ubuntu:~/C$ ./tostring
> pg_int64tostr_memcpy in 13.653252 seconds
> pg_int64tostr in 2.042616 seconds
> pg_int64tostr_new in 2.056688 seconds
> pg_lltoa in 13.604653 seconds
> 
> david@ubuntu:~/C$ clang tostring.c -o tostring_clang -O2
> david@ubuntu:~/C$ ./tostring_clang
> pg_int64tostr_memcpy in 0.000004 seconds
> pg_int64tostr in 2.063335 seconds
> pg_int64tostr_new in 2.102068 seconds
> pg_lltoa in 3.424630 seconds

Are you sure this isn't an optimization artifact where the actual work
is optimized away? Doing 2 billion conversions in 2s kinda implies that
the string conversion is done in ~4 cycles. That seems unrealistically
fast, even for a pipelined cpu.


Greetings,

Andres Freund

On 2015-09-12 04:00:26 +1200, David Rowley wrote:
> I've not done anything yet to remove the #ifdef HAVE_INT64_TIMESTAMP from
> AppendSeconds(). The only way I can think to handle this is to just
> make fsec_t unconditionally an int64 (since with float datetimes the
> precision is 10 decimal digits after the dec point, this does not fit in
> int32). I'd go off and do this, but this means I need to write an int64
> version of pg_ltostr(). Should I do it this way?

I don't have time to look into this in detail right now. But isn't that
the wrong view?

The precision with float timestamps is still microseconds which you can
easily express in an integer. That the value can be higher is just
because fsec_t for floating points also includes seconds, right? It
shouldn't be too hard to separate those?

Greetings,

Andres Freund

On 9/13/15 2:43 AM, David Rowley wrote:
> Are you worried about this because I've not focused on optimising float
> timestamps as much as int64 timestamps?  Are there many people compiling
> with float timestamps in the real world?

My $0.02: the default was changed some 5 years ago so FP time is 
probably pretty rare now. I don't think it's worth a bunch of extra work 
to speed them up.
-- 
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com

On 2015-09-14 12:03:31 -0500, Jim Nasby wrote:
> On 9/13/15 2:43 AM, David Rowley wrote:
> >Are you worried about this because I've not focused on optimising float
> >timestamps as much as int64 timestamps?  Are there many people compiling
> >with float timestamps in the real world?
> 
> My $0.02: the default was changed some 5 years ago so FP time is probably
> pretty rare now. I don't think it's worth a bunch of extra work to speed
> them up.

Agreed. That's not why I'm arguing for it. It's about having kind of duplicate
code that's not exercised at al. by any common setup.

Greetings,

Andres Freund

Jim Nasby wrote:
> On 9/13/15 2:43 AM, David Rowley wrote:
> >Are you worried about this because I've not focused on optimising float
> >timestamps as much as int64 timestamps?  Are there many people compiling
> >with float timestamps in the real world?
> 
> My $0.02: the default was changed some 5 years ago so FP time is probably
> pretty rare now.

The default was FP for 8.3 and was changed before 8.4.  Anybody who was
running with the default back then and who has pg_upgraded all the way
to current releases is still using floating-point date/times.

> I don't think it's worth a bunch of extra work to speed them up.

Not sure about that.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

David Rowley wrote:

> It's not like nothing is improved in float timestamps with this patch, it's
> only appending the non-zero fractional seconds that I've left alone. Every
> other portion of the timestamp has been improved.

OK, sounds good enough to me.

> I did, however spend some time a few weekends ago writing a function to
> improve this, which is more aimed at improving poor performance of
> float4out and float8out. It's quite likely if I can get finished one day,
> it can help improve this case too.

Even better, prolly not a blocker for this patch.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Fri, Sep 11, 2015 at 9:00 AM, David Rowley
<david.rowley@2ndquadrant.com> wrote:
> Attached is also timestamp_delta.patch which changes pg_ltostr() to use the
> INT_MIN special case that pg_ltoa also uses. I didn't make a similar change
> to pg_ltostr_zeropad() as I'd need to add even more special code to prepend
> the correct number of zero before the 2147483648. This zero padding reason
> was why I originally came up with the alternative way to handle the negative
> numbers. I had just thought that it was best to keep both functions as
> similar as possible.

I've started looking at this -- "timestamp_out_speedup_2015-09-12.patch".

> I've not done anything yet to remove the #ifdef HAVE_INT64_TIMESTAMP from
> AppendSeconds(). The only way I can think to handle this is to just make
> fsec_t unconditionally an int64 (since with float datetimes the precision is
> 10 decimal digits after the dec point, this does not fit in int32). I'd go
> off and do this, but this means I need to write an int64 version of
> pg_ltostr(). Should I do it this way?

Have you thought about *just* having an int64 pg_ltostr()? Are you
aware of an appreciable overhead from having int32 callers just rely
on a promotion to int64?

In general, years are 1900-based in Postgres:

struct pg_tm
{...   int         tm_mday;   int         tm_mon;         /* origin 0, not 1 */   int         tm_year;        /*
relativeto 1900 */...
 
};

While it's not your patch's fault, it is not noted by EncodeDateTime()
and EncodeDateOnly() and others that there pg_tm structs are not
1900-based. This is in contrast to multiple functions within
formatting.c, nabstime.c, and timestamp.c (some of which are clients
or clients of clients for this new code). I think that the rule has
been undermined so much that it doesn't make sense to indicate
exceptions directly, though. I think pg_tm should have comments saying
that there are many cases where tm_year is not relative to 1900.

I have a few minor nitpicks about the patch.

No need for "negative number" comment here -- just use
"Assert(precision >= 0)" code:

+ * Note 'precision' must not be a negative number.
+ * Note callers should assume cp will not be NUL terminated. */
-static void
+static char *AppendSeconds(char *cp, int sec, fsec_t fsec, int precision, bool fillzeros){
+#ifdef HAVE_INT64_TIMESTAMP
+

I would just use a period/full stop here:

+ * Note str is not NUL terminated, callers are responsible for NUL terminating
+ * str themselves.

Don't think you need so many "Note:" specifiers here:

+ * Note: Callers should ensure that 'padding' is above zero.
+ * Note: This function is optimized for the case where the number is not too
+ *      big to fit inside of the specified padding.
+ * Note: Caller must ensure that 'str' points to enough memory to hold the
+        result (at least 12 bytes, counting a leading sign and trailing NUL,
+        or padding + 1 bytes, whichever is larger).
+ */
+char *
+pg_ltostr_zeropad(char *str, int32 value, int32 padding)

Not so sure about this, within the new pg_ltostr_zeropad() function:

+   /*
+    * Handle negative numbers in a special way. We can't just append a '-'
+    * prefix and reverse the sign as on two's complement machines negative
+    * numbers can be 1 further from 0 than positive numbers, we do it this way
+    * so we properly handle the smallest possible value.
+    */
+   if (num < 0)
+   {     ...
+   }
+   else
+   {     ...
+   }

Maybe it's better to just special case INT_MIN and the do an Abs()?
Actually, would any likely caller of pg_ltostr_zeropad() actually care
if INT_MIN was a case that you cannot handle, and assert against? You
could perhaps reasonably make it the caller's problem. Haven't made up
my mind if your timestamp_delta.patch is better than that; cannot
immediately see a problem with putting it on the caller.

More generally, maybe routines like EncodeDateTime() ought now to use
the Abs() macro.

Those are all of my nitpicks. :-)

> I also did some tests on server grade hardware, and the performance increase
> is looking a bit better.
>
> create table ts (ts timestamp not null);
> insert into ts select generate_series('2010-01-01 00:00:00', '2011-01-01
> 00:00:00', '1 sec'::interval);
> vacuum freeze ts;

On my laptop, I saw a 2.4X - 2.6X speedup for this case. That seems
pretty nice to me.

Will make another pass over this soon.

-- 
Peter Geoghegan

On Wed, Nov 4, 2015 at 6:30 PM, David Rowley
<david.rowley@2ndquadrant.com> wrote:
>> Have you thought about *just* having an int64 pg_ltostr()? Are you
>> aware of an appreciable overhead from having int32 callers just rely
>> on a promotion to int64?
>
> I'd not thought of that. It would certainly add unnecessary overhead on
> 32bit machines.
> To be honest, I don't really like the idea of making fsec_t an int64, there
> are just to many places around the code base that need to be updated. Plus
> with float datetimes, we'd need to multiple the fractional seconds by
> 1000000000, which is based on the MAX_TIME_PRECISION setting as defined when
> HAVE_INT64_TIMESTAMP is undefined.

OK.

>> I would just use a period/full stop here:
>>
>> + * Note str is not NUL terminated, callers are responsible for NUL
>> terminating
>> + * str themselves.
>>
>
> Do you mean change the comment to "Note str is not NUL terminated. Callers
> are responsible for NUL terminating str themselves." ?

Yes.

> Yes, that's a bit ugly. How about just Assert(padding > 0) just like above?
> That gets rid of one Note:
> The optimized part is probably not that important anyway. I just mentioned
> it to try and reduce the changes of someone using it when the padding is
> always too small.

Cool.

>> Maybe it's better to just special case INT_MIN and the do an Abs()?
>> Actually, would any likely caller of pg_ltostr_zeropad() actually care
>> if INT_MIN was a case that you cannot handle, and assert against? You
>> could perhaps reasonably make it the caller's problem. Haven't made up
>> my mind if your timestamp_delta.patch is better than that; cannot
>> immediately see a problem with putting it on the caller.
>>
>
> I can make that case work the same as pg_ltoa() for pg_ltostr(), that's not
> a problem, I did that in the delta patch. With pg_ltostr_zeropad() I'd need
> to add some corner case code to prepend the correct number of zeros onto
> -2147483648. This is likely not going to look very nice, and also it's
> probably going to end up about the same number of lines of code to what's in
> the patch already. If you think it's better for performance, then I've
> already done tests to show that it's not better. I really don't think it'll
> look very nice either. I quite like my negative number handling, since it's
> actually code that would be exercised 50% of the time ran than 1/ 2^32 of
> the time, assuming all numbers have an equal chance of being passed.

Fair enough.

>> More generally, maybe routines like EncodeDateTime() ought now to use
>> the Abs() macro.
>
> You might be right about that. Then we can just have pg_ltostr() do unsigned
> only. The reason I didn't do that is because I was trying to minimise what I
> was changing in the patch, and I thought it was less likely to make it if I
> started adding calls to Abs() around the code base.

I am very familiar with being conflicted about changing things beyond
what is actually strictly necessary. It's still something I deal with
a lot. One case that I think I have right in recommending commenting
on is the need to centrally document that there are many exceptions to
the 1900-based behavior of struct pg_tm. As I said, we should not
continue to inconsistently locally note the exceptions, even if your
patch doesn't make it any worse.

-- 
Peter Geoghegan

On Sun, Nov 22, 2015 at 2:20 AM, David Rowley
<david.rowley@2ndquadrant.com> wrote:
> Just to confirm, you mean this comment?
>
> int tm_year; /* relative to 1900 */
>
> Please let me know if you disagree, but I'm not sure it's the business of
> this patch to fix that. If it's wrong now, then it was wrong before my
> patch, so it should be a separate patch which fixes it.
>
> At this stage I don't quite know what the fix should be, weather it's doing
> tm->tm_year -= 1900; in timestamp2tm() after the j2date() call, or if it's
> just removing the misleading comment.
>
> I also don't quite understand why we bother having it relative to 1900 and
> not just base it on 0.

That's fair. I defer to the judgement of the committer here.

> Is there anything else you see that's pending before it can be marked as
> ready for committer?

Can't think of any reason not to. It's been marked "ready for committer".

Thanks

-- 
Peter Geoghegan

David Rowley <david.rowley@2ndquadrant.com> writes:
[ timestamp_out_speedup_2015-11-05.patch ]

Pushed with a bunch of cosmetic tweaks.
        regards, tom lane

<p dir="ltr"><br /> On 7/02/2016 4:14 am, "Tom Lane" <<a href="mailto:tgl@sss.pgh.pa.us">tgl@sss.pgh.pa.us</a>>
wrote:<br/> ><br /> > David Rowley <<a
href="mailto:david.rowley@2ndquadrant.com">david.rowley@2ndquadrant.com</a>>writes:<br /> > [
timestamp_out_speedup_2015-11-05.patch]<br /> ><br /> > Pushed with a bunch of cosmetic tweaks.<p
dir="ltr">Great.Thanks for pushing this.<p dir="ltr">David