Refactor Pickle Read methods to use higher performance PickleIterator.

Refactor Pickle Read methods to use higher performance PickleIterator.

There was a lot of redundant error checking and initialization code in all Pickle Read methods because of the void** iterator type. This change replaces the void* iterator with PickleIterator, which encapsulates the read pointer so that less error checking and initialization code is needed for reading.

PickleIterator has all the necessary data to do the actual reading. The advantage of having it provide Read methods (as opposed to leaving them solely in the Pickle interface) is that the callers do not need to pass around the const Pickle* once they have a PickleIterator.

Followup CLs will refactor the call sites to remove const Pickle* arguments where they are now unnecessary. Then the Pickle::Read* methods can be removed entirely.

The alternative approach would have been to change the Pickle::Read methods to non-const and remove the iterator parameter (making Read methods advance an internal read pointer). Unfortunately, the const Read with iterator design is entrenched throughout the chromium code, making this a much more complex change with the same performance outcome.

BUG=13108
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=125447

[jbates, 2012-02-27 17:45:51]

Starting with you Jim for a high level design review. Feel free to reassign if
you think someone else is more appropriate.

Some line lengths may still be >80 chars.

[jbates, 2012-02-27 18:12:50]

Adding John as well for a high level design review. PTAL

[jar (doing other things), 2012-02-29 22:04:08]

LGTM

You mentioned you were "adding John" but  I don't see an additional reviewer or
cc.

https://chromiumcodereview.appspot.com/9447084/diff/13002/base/pickle.cc
File base/pickle.cc (right):

https://chromiumcodereview.appspot.com/9447084/diff/13002/base/pickle.cc#newc...
base/pickle.cc:158: read_ptr_ = read_end_ptr_ = NULL;
This needs to be reflected in the header file comment (comment says it "does
nothing."

https://chromiumcodereview.appspot.com/9447084/diff/13002/base/pickle.h
File base/pickle.h (right):

https://chromiumcodereview.appspot.com/9447084/diff/13002/base/pickle.h#newco...
base/pickle.h:52: // available. Otherwise, does nothing and returns false.
The code suggests if this fails, that it nulls out the read_ptr_ and
read_end_ptr_.

[jbates, 2012-02-29 23:21:41]

Adding darin to cover the majority of the OWNERS. Do I need all the owners for
this kind of refactor?

[jbates, 2012-02-29 23:22:25]

https://chromiumcodereview.appspot.com/9447084/diff/13002/base/pickle.cc
File base/pickle.cc (right):

https://chromiumcodereview.appspot.com/9447084/diff/13002/base/pickle.cc#newc...
base/pickle.cc:158: read_ptr_ = read_end_ptr_ = NULL;
On 2012/02/29 22:04:08, jar wrote:
> This needs to be reflected in the header file comment (comment says it "does
> nothing."

Done.

https://chromiumcodereview.appspot.com/9447084/diff/13002/base/pickle.h
File base/pickle.h (right):

https://chromiumcodereview.appspot.com/9447084/diff/13002/base/pickle.h#newco...
base/pickle.h:52: // available. Otherwise, does nothing and returns false.
On 2012/02/29 22:04:08, jar wrote:
> The code suggests if this fails, that it nulls out the read_ptr_ and
> read_end_ptr_.

Done.

[darin (slow to review), 2012-03-02 23:12:34]

I suppose the "Pickle has an internal read cursor" approach could be done even
with const Pickle instances by using the 'mutable' keyword.  Maybe that would
abuse the interface too much.

Have you seen IPC::MessageIterator?  We should probably remove that / replace
that with PickleReader.

Had you considered just changing the "void**" parameters to something like
"PickleIterator*" to allow you to hold more state?  I realize this means passing
around "const Pickle*" and "PickleIterator*", but all of the code already passes
around two items when reading.

[jbates, 2012-03-02 23:43:22]

On 2012/03/02 23:12:34, darin wrote:
> I suppose the "Pickle has an internal read cursor" approach could be done even
> with const Pickle instances by using the 'mutable' keyword.  Maybe that would
> abuse the interface too much.

Yeah, if we went that route I would probably just replace const Pickle* with
Pickle* and update the call sites to make sure they call Pickle::ResetRead (eg)
to reread the data (if that's actually ever done).

> 
> Have you seen IPC::MessageIterator?  We should probably remove that / replace
> that with PickleReader.

Yeah, IPC::MessageIterator became a pass-through to PickleReader with this CL.
It could be removed as a followup.

> 
> Had you considered just changing the "void**" parameters to something like
> "PickleIterator*" to allow you to hold more state? 

Pickle::Read(PickleIterator, ...) and the current void** approach come with the
slight risk of dealing with mismatched Pickle/iterator calls. I noticed a few
places in the code where multiple Pickle objects were in scope, so it may reduce
the likelihood of user error if reading can only be done from PickleReader. I
could be convinced either way though if you think there are advantages.

> I realize this means passing
> around "const Pickle*" and "PickleIterator*", but all of the code already
passes
> around two items when reading.

[darin (slow to review), 2012-03-03 00:03:55]

On Fri, Mar 2, 2012 at 3:43 PM, <jbates@chromium.org> wrote:

> On 2012/03/02 23:12:34, darin wrote:
>
>> I suppose the "Pickle has an internal read cursor" approach could be done
>> even
>> with const Pickle instances by using the 'mutable' keyword.  Maybe that
>> would
>> abuse the interface too much.
>>
>
> Yeah, if we went that route I would probably just replace const Pickle*
> with
> Pickle* and update the call sites to make sure they call Pickle::ResetRead
> (eg)
> to reread the data (if that's actually ever done).
>
>
>
>  Have you seen IPC::MessageIterator?  We should probably remove that /
>> replace
>> that with PickleReader.
>>
>
> Yeah, IPC::MessageIterator became a pass-through to PickleReader with this
> CL.
> It could be removed as a followup.
>
>
>
>  Had you considered just changing the "void**" parameters to something like
>> "PickleIterator*" to allow you to hold more state?
>>
>
> Pickle::Read(PickleIterator, ...) and the current void** approach come
> with the
> slight risk of dealing with mismatched Pickle/iterator calls. I noticed a
> few
> places in the code where multiple Pickle objects were in scope, so it may
> reduce
> the likelihood of user error if reading can only be done from
> PickleReader. I
> could be convinced either way though if you think there are advantages.



I'm just trying to understand what led to the design.  It seems like you
thought
through everything.

-Darin



>
>
>  I realize this means passing
>> around "const Pickle*" and "PickleIterator*", but all of the code already
>>
> passes
>
>> around two items when reading.
>>
>
>
>
https://chromiumcodereview.**appspot.com/9447084/<https://chromiumcodereview....
>

[jbates, 2012-03-05 21:55:27]

On 2012/03/03 00:03:55, darin wrote:
> On Fri, Mar 2, 2012 at 3:43 PM, <mailto:jbates@chromium.org> wrote:
> 
> > On 2012/03/02 23:12:34, darin wrote:
> >
> >> I suppose the "Pickle has an internal read cursor" approach could be done
> >> even
> >> with const Pickle instances by using the 'mutable' keyword.  Maybe that
> >> would
> >> abuse the interface too much.
> >>
> >
> > Yeah, if we went that route I would probably just replace const Pickle*
> > with
> > Pickle* and update the call sites to make sure they call Pickle::ResetRead
> > (eg)
> > to reread the data (if that's actually ever done).
> >
> >
> >
> >  Have you seen IPC::MessageIterator?  We should probably remove that /
> >> replace
> >> that with PickleReader.
> >>
> >
> > Yeah, IPC::MessageIterator became a pass-through to PickleReader with this
> > CL.
> > It could be removed as a followup.
> >
> >
> >
> >  Had you considered just changing the "void**" parameters to something like
> >> "PickleIterator*" to allow you to hold more state?
> >>
> >
> > Pickle::Read(PickleIterator, ...) and the current void** approach come
> > with the
> > slight risk of dealing with mismatched Pickle/iterator calls. I noticed a
> > few
> > places in the code where multiple Pickle objects were in scope, so it may
> > reduce
> > the likelihood of user error if reading can only be done from
> > PickleReader. I
> > could be convinced either way though if you think there are advantages.
> 
> 
> 
> I'm just trying to understand what led to the design.  It seems like you
> thought
> through everything.

I hope so. It's certainly faster and smaller code this way and the tests pass.
Let me know if there is anything else that needs to be done before committing
this.

> 
> -Darin
> 
> 
> 
> >
> >
> >  I realize this means passing
> >> around "const Pickle*" and "PickleIterator*", but all of the code already
> >>
> > passes
> >
> >> around two items when reading.
> >>
> >
> >
> >
>
https://chromiumcodereview.**appspot.com/9447084/%3Chttps://chromiumcoderevie...>
> >

[darin (slow to review), 2012-03-06 18:02:13]

what's the performance benefit of this change?  what kinds of measurements have
you done?

https://chromiumcodereview.appspot.com/9447084/diff/21002/base/file_path.h
File base/file_path.h (right):

https://chromiumcodereview.appspot.com/9447084/diff/21002/base/file_path.h#ne...
base/file_path.h:343: bool ReadFromPickle(PickleReader* iter);
sorry to nit pick, but it'd be great if you could rename the parameter to
'reader' instead of 'iter'... or maybe go with PickleIterator?

https://chromiumcodereview.appspot.com/9447084/diff/21002/base/pickle.h
File base/pickle.h (right):

https://chromiumcodereview.appspot.com/9447084/diff/21002/base/pickle.h#newco...
base/pickle.h:142: bool ReadBool(PickleReader* iter, bool* result) const {
same nit:  rename 'iter' to 'reader'

https://chromiumcodereview.appspot.com/9447084/diff/21002/chrome/browser/book...
File chrome/browser/bookmarks/bookmark_node_data.cc (right):

https://chromiumcodereview.appspot.com/9447084/diff/21002/chrome/browser/book...
chrome/browser/bookmarks/bookmark_node_data.cc:57: bool
BookmarkNodeData::Element::ReadFromPickle(Pickle* pickle,
so the idea is to do a follow-up patch that will clean code like this up to just
pass PickleReader?

[jbates, 2012-03-06 18:23:23]

On 2012/03/06 18:02:13, darin wrote:
> what's the performance benefit of this change?  what kinds of measurements
have
> you done?

For each Read method, the old code is:

0000000000000000 <_ZNK6Pickle8ReadLongEPPvPl>:
   0:	48 8b 06             	mov    (%rsi),%rax
   3:	48 85 c0             	test   %rax,%rax
   6:	74 60                	je     68 <_ZNK6Pickle8ReadLongEPPvPl+0x68>
   8:	48 8b 4f 08          	mov    0x8(%rdi),%rcx
   c:	48 39 c8             	cmp    %rcx,%rax
   f:	72 1b                	jb     2c <_ZNK6Pickle8ReadLongEPPvPl+0x2c>
  11:	45 31 c0             	xor    %r8d,%r8d
  14:	48 85 c9             	test   %rcx,%rcx
  17:	74 0e                	je     27 <_ZNK6Pickle8ReadLongEPPvPl+0x27>
  19:	49 89 c9             	mov    %rcx,%r9
  1c:	44 8b 01             	mov    (%rcx),%r8d
  1f:	4c 03 4f 10          	add    0x10(%rdi),%r9
  23:	4f 8d 04 01          	lea    (%r9,%r8,1),%r8
  27:	4c 39 c0             	cmp    %r8,%rax
  2a:	76 04                	jbe    30 <_ZNK6Pickle8ReadLongEPPvPl+0x30>
  2c:	31 c0                	xor    %eax,%eax
  2e:	c3                   	retq   
  2f:	90                   	nop
  30:	49 89 c1             	mov    %rax,%r9
  33:	49 83 c1 08          	add    $0x8,%r9
  37:	72 f3                	jb     2c <_ZNK6Pickle8ReadLongEPPvPl+0x2c>
  39:	45 31 c0             	xor    %r8d,%r8d
  3c:	48 85 c9             	test   %rcx,%rcx
  3f:	90                   	nop
  40:	74 0c                	je     4e <_ZNK6Pickle8ReadLongEPPvPl+0x4e>
  42:	49 89 c8             	mov    %rcx,%r8
  45:	8b 09                	mov    (%rcx),%ecx
  47:	4c 03 47 10          	add    0x10(%rdi),%r8
  4b:	49 01 c8             	add    %rcx,%r8
  4e:	4d 39 c1             	cmp    %r8,%r9
  51:	77 d9                	ja     2c <_ZNK6Pickle8ReadLongEPPvPl+0x2c>
  53:	48 8b 00             	mov    (%rax),%rax
  56:	48 89 02             	mov    %rax,(%rdx)
  59:	b8 01 00 00 00       	mov    $0x1,%eax
  5e:	48 83 06 08          	addq   $0x8,(%rsi)
  62:	c3                   	retq   
  63:	0f 1f 44 00 00       	nopl   0x0(%rax,%rax,1)
  68:	48 8b 47 08          	mov    0x8(%rdi),%rax
  6c:	48 03 47 10          	add    0x10(%rdi),%rax
  70:	48 89 06             	mov    %rax,(%rsi)
  73:	eb 93                	jmp    8 <_ZNK6Pickle8ReadLongEPPvPl+0x8>

With this CL, Read methods look like this:

0000000000000000 <_ZN6Pickle9ReadLong2EPl>:
   0:	48 8b 57 28          	mov    0x28(%rdi),%rdx
   4:	31 c0                	xor    %eax,%eax
   6:	48 8d 4a 08          	lea    0x8(%rdx),%rcx
   a:	48 39 4f 38          	cmp    %rcx,0x38(%rdi)
   e:	73 08                	jae    18 <_ZN6Pickle9ReadLong2EPl+0x18>
  10:	f3 c3                	repz retq 
  12:	66 0f 1f 44 00 00    	nopw   0x0(%rax,%rax,1)
  18:	48 8b 02             	mov    (%rdx),%rax
  1b:	48 89 06             	mov    %rax,(%rsi)
  1e:	b8 01 00 00 00       	mov    $0x1,%eax
  23:	48 83 47 28 08       	addq   $0x8,0x28(%rdi)
  28:	c3                   	retq   

The code size reduction reduces icache pressure and fewer branches make it
faster. Do we have any micro perf tests that measure this kind of code?

> 
> https://chromiumcodereview.appspot.com/9447084/diff/21002/base/file_path.h
> File base/file_path.h (right):
> 
>
https://chromiumcodereview.appspot.com/9447084/diff/21002/base/file_path.h#ne...
> base/file_path.h:343: bool ReadFromPickle(PickleReader* iter);
> sorry to nit pick, but it'd be great if you could rename the parameter to
> 'reader' instead of 'iter'... or maybe go with PickleIterator?

PickleIterator sounds good.

> 
> https://chromiumcodereview.appspot.com/9447084/diff/21002/base/pickle.h
> File base/pickle.h (right):
> 
>
https://chromiumcodereview.appspot.com/9447084/diff/21002/base/pickle.h#newco...
> base/pickle.h:142: bool ReadBool(PickleReader* iter, bool* result) const {
> same nit:  rename 'iter' to 'reader'
> 
>
https://chromiumcodereview.appspot.com/9447084/diff/21002/chrome/browser/book...
> File chrome/browser/bookmarks/bookmark_node_data.cc (right):
> 
>
https://chromiumcodereview.appspot.com/9447084/diff/21002/chrome/browser/book...
> chrome/browser/bookmarks/bookmark_node_data.cc:57: bool
> BookmarkNodeData::Element::ReadFromPickle(Pickle* pickle,
> so the idea is to do a follow-up patch that will clean code like this up to
just
> pass PickleReader?

Exactly, that's the plan.

[darin (slow to review), 2012-03-06 18:46:17]

On Tue, Mar 6, 2012 at 10:23 AM, <jbates@chromium.org> wrote:

> On 2012/03/06 18:02:13, darin wrote:
>
>> what's the performance benefit of this change?  what kinds of measurements
>>
> have
>
>> you done?
>>
>
> For each Read method, the old code is:
>
> 0000000000000000 <_ZNK6Pickle8ReadLongEPPvPl>:
>   0:   48 8b 06                mov    (%rsi),%rax
>   3:   48 85 c0                test   %rax,%rax
>   6:   74 60                   je     68 <_ZNK6Pickle8ReadLongEPPvPl+**
> 0x68>
>   8:   48 8b 4f 08             mov    0x8(%rdi),%rcx
>   c:   48 39 c8                cmp    %rcx,%rax
>   f:   72 1b                   jb     2c <_ZNK6Pickle8ReadLongEPPvPl+**
> 0x2c>
>  11:   45 31 c0                xor    %r8d,%r8d
>  14:   48 85 c9                test   %rcx,%rcx
>  17:   74 0e                   je     27 <_ZNK6Pickle8ReadLongEPPvPl+**
> 0x27>
>  19:   49 89 c9                mov    %rcx,%r9
>  1c:   44 8b 01                mov    (%rcx),%r8d
>  1f:   4c 03 4f 10             add    0x10(%rdi),%r9
>  23:   4f 8d 04 01             lea    (%r9,%r8,1),%r8
>  27:   4c 39 c0                cmp    %r8,%rax
>  2a:   76 04                   jbe    30 <_ZNK6Pickle8ReadLongEPPvPl+**
> 0x30>
>  2c:   31 c0                   xor    %eax,%eax
>  2e:   c3                      retq
>  2f:   90                      nop
>  30:   49 89 c1                mov    %rax,%r9
>  33:   49 83 c1 08             add    $0x8,%r9
>  37:   72 f3                   jb     2c <_ZNK6Pickle8ReadLongEPPvPl+**
> 0x2c>
>  39:   45 31 c0                xor    %r8d,%r8d
>  3c:   48 85 c9                test   %rcx,%rcx
>  3f:   90                      nop
>  40:   74 0c                   je     4e <_ZNK6Pickle8ReadLongEPPvPl+**
> 0x4e>
>  42:   49 89 c8                mov    %rcx,%r8
>  45:   8b 09                   mov    (%rcx),%ecx
>  47:   4c 03 47 10             add    0x10(%rdi),%r8
>  4b:   49 01 c8                add    %rcx,%r8
>  4e:   4d 39 c1                cmp    %r8,%r9
>  51:   77 d9                   ja     2c <_ZNK6Pickle8ReadLongEPPvPl+**
> 0x2c>
>  53:   48 8b 00                mov    (%rax),%rax
>  56:   48 89 02                mov    %rax,(%rdx)
>  59:   b8 01 00 00 00          mov    $0x1,%eax
>  5e:   48 83 06 08             addq   $0x8,(%rsi)
>  62:   c3                      retq
>  63:   0f 1f 44 00 00          nopl   0x0(%rax,%rax,1)
>  68:   48 8b 47 08             mov    0x8(%rdi),%rax
>  6c:   48 03 47 10             add    0x10(%rdi),%rax
>  70:   48 89 06                mov    %rax,(%rsi)
>  73:   eb 93                   jmp    8 <_ZNK6Pickle8ReadLongEPPvPl+**0x8>
>
> With this CL, Read methods look like this:
>
> 0000000000000000 <_ZN6Pickle9ReadLong2EPl>:
>   0:   48 8b 57 28             mov    0x28(%rdi),%rdx
>   4:   31 c0                   xor    %eax,%eax
>   6:   48 8d 4a 08             lea    0x8(%rdx),%rcx
>   a:   48 39 4f 38             cmp    %rcx,0x38(%rdi)
>   e:   73 08                   jae    18 <_ZN6Pickle9ReadLong2EPl+0x18>
>  10:   f3 c3                   repz retq
>  12:   66 0f 1f 44 00 00       nopw   0x0(%rax,%rax,1)
>  18:   48 8b 02                mov    (%rdx),%rax
>  1b:   48 89 06                mov    %rax,(%rsi)
>  1e:   b8 01 00 00 00          mov    $0x1,%eax
>  23:   48 83 47 28 08          addq   $0x8,0x28(%rdi)
>  28:   c3                      retq
>
> The code size reduction reduces icache pressure and fewer branches make it
> faster. Do we have any micro perf tests that measure this kind of code?



OK, but there are very few Read methods.  I don't know of any tests that
would
specifically help us benchmark this code.

While this change seems like it could be an improvement, I think it would be
helpful to know if this optimization actually manifests itself in terms of
any real
improvement to the product.

-Darin



>
>
>
>  https://chromiumcodereview.**appspot.com/9447084/diff/**
>>
21002/base/file_path.h<https://chromiumcodereview.appspot.com/9447084/diff/21002/base/file_path.h>
>> File base/file_path.h (right):
>>
>
>
> https://chromiumcodereview.**appspot.com/9447084/diff/**
>
21002/base/file_path.h#**newcode343<https://chromiumcodereview.appspot.com/9447084/diff/21002/base/file_path.h#newcode343>
>
>> base/file_path.h:343: bool ReadFromPickle(PickleReader* iter);
>> sorry to nit pick, but it'd be great if you could rename the parameter to
>> 'reader' instead of 'iter'... or maybe go with PickleIterator?
>>
>
> PickleIterator sounds good.
>
>
>
>  https://chromiumcodereview.**appspot.com/9447084/diff/**
>>
21002/base/pickle.h<https://chromiumcodereview.appspot.com/9447084/diff/21002/base/pickle.h>
>> File base/pickle.h (right):
>>
>
>
> https://chromiumcodereview.**appspot.com/9447084/diff/**
>
21002/base/pickle.h#newcode142<https://chromiumcodereview.appspot.com/9447084/diff/21002/base/pickle.h#newcode142>
>
>> base/pickle.h:142: bool ReadBool(PickleReader* iter, bool* result) const {
>> same nit:  rename 'iter' to 'reader'
>>
>
>
> https://chromiumcodereview.**appspot.com/9447084/diff/**
>
21002/chrome/browser/**bookmarks/bookmark_node_data.**cc<https://chromiumcodereview.appspot.com/9447084/diff/21002/chrome/browser/bookmarks/bookmark_node_data.cc>
>
>> File chrome/browser/bookmarks/**bookmark_node_data.cc (right):
>>
>
>
> https://chromiumcodereview.**appspot.com/9447084/diff/**
>
21002/chrome/browser/**bookmarks/bookmark_node_data.**cc#newcode57<https://chromiumcodereview.appspot.com/9447084/diff/21002/chrome/browser/bookmarks/bookmark_node_data.cc#newcode57>
>
>> chrome/browser/bookmarks/**bookmark_node_data.cc:57: bool
>> BookmarkNodeData::Element::**ReadFromPickle(Pickle* pickle,
>> so the idea is to do a follow-up patch that will clean code like this up
>> to
>>
> just
>
>> pass PickleReader?
>>
>
> Exactly, that's the plan.
>
>
https://chromiumcodereview.**appspot.com/9447084/<https://chromiumcodereview....
>

[jbates, 2012-03-06 19:47:29]

On 2012/03/06 18:46:17, darin wrote:
> On Tue, Mar 6, 2012 at 10:23 AM, <mailto:jbates@chromium.org> wrote:
> 
> > On 2012/03/06 18:02:13, darin wrote:
> >
> >> what's the performance benefit of this change?  what kinds of measurements
> >>
> > have
> >
> >> you done?
> >>
> >
> > For each Read method, the old code is:
> >
> > 0000000000000000 <_ZNK6Pickle8ReadLongEPPvPl>:
> >   0:   48 8b 06                mov    (%rsi),%rax
> >   3:   48 85 c0                test   %rax,%rax
> >   6:   74 60                   je     68 <_ZNK6Pickle8ReadLongEPPvPl+**
> > 0x68>
> >   8:   48 8b 4f 08             mov    0x8(%rdi),%rcx
> >   c:   48 39 c8                cmp    %rcx,%rax
> >   f:   72 1b                   jb     2c <_ZNK6Pickle8ReadLongEPPvPl+**
> > 0x2c>
> >  11:   45 31 c0                xor    %r8d,%r8d
> >  14:   48 85 c9                test   %rcx,%rcx
> >  17:   74 0e                   je     27 <_ZNK6Pickle8ReadLongEPPvPl+**
> > 0x27>
> >  19:   49 89 c9                mov    %rcx,%r9
> >  1c:   44 8b 01                mov    (%rcx),%r8d
> >  1f:   4c 03 4f 10             add    0x10(%rdi),%r9
> >  23:   4f 8d 04 01             lea    (%r9,%r8,1),%r8
> >  27:   4c 39 c0                cmp    %r8,%rax
> >  2a:   76 04                   jbe    30 <_ZNK6Pickle8ReadLongEPPvPl+**
> > 0x30>
> >  2c:   31 c0                   xor    %eax,%eax
> >  2e:   c3                      retq
> >  2f:   90                      nop
> >  30:   49 89 c1                mov    %rax,%r9
> >  33:   49 83 c1 08             add    $0x8,%r9
> >  37:   72 f3                   jb     2c <_ZNK6Pickle8ReadLongEPPvPl+**
> > 0x2c>
> >  39:   45 31 c0                xor    %r8d,%r8d
> >  3c:   48 85 c9                test   %rcx,%rcx
> >  3f:   90                      nop
> >  40:   74 0c                   je     4e <_ZNK6Pickle8ReadLongEPPvPl+**
> > 0x4e>
> >  42:   49 89 c8                mov    %rcx,%r8
> >  45:   8b 09                   mov    (%rcx),%ecx
> >  47:   4c 03 47 10             add    0x10(%rdi),%r8
> >  4b:   49 01 c8                add    %rcx,%r8
> >  4e:   4d 39 c1                cmp    %r8,%r9
> >  51:   77 d9                   ja     2c <_ZNK6Pickle8ReadLongEPPvPl+**
> > 0x2c>
> >  53:   48 8b 00                mov    (%rax),%rax
> >  56:   48 89 02                mov    %rax,(%rdx)
> >  59:   b8 01 00 00 00          mov    $0x1,%eax
> >  5e:   48 83 06 08             addq   $0x8,(%rsi)
> >  62:   c3                      retq
> >  63:   0f 1f 44 00 00          nopl   0x0(%rax,%rax,1)
> >  68:   48 8b 47 08             mov    0x8(%rdi),%rax
> >  6c:   48 03 47 10             add    0x10(%rdi),%rax
> >  70:   48 89 06                mov    %rax,(%rsi)
> >  73:   eb 93                   jmp    8 <_ZNK6Pickle8ReadLongEPPvPl+**0x8>
> >
> > With this CL, Read methods look like this:
> >
> > 0000000000000000 <_ZN6Pickle9ReadLong2EPl>:
> >   0:   48 8b 57 28             mov    0x28(%rdi),%rdx
> >   4:   31 c0                   xor    %eax,%eax
> >   6:   48 8d 4a 08             lea    0x8(%rdx),%rcx
> >   a:   48 39 4f 38             cmp    %rcx,0x38(%rdi)
> >   e:   73 08                   jae    18 <_ZN6Pickle9ReadLong2EPl+0x18>
> >  10:   f3 c3                   repz retq
> >  12:   66 0f 1f 44 00 00       nopw   0x0(%rax,%rax,1)
> >  18:   48 8b 02                mov    (%rdx),%rax
> >  1b:   48 89 06                mov    %rax,(%rsi)
> >  1e:   b8 01 00 00 00          mov    $0x1,%eax
> >  23:   48 83 47 28 08          addq   $0x8,0x28(%rdi)
> >  28:   c3                      retq
> >
> > The code size reduction reduces icache pressure and fewer branches make it
> > faster. Do we have any micro perf tests that measure this kind of code?
> 
> 
> 
> OK, but there are very few Read methods.  I don't know of any tests that
> would
> specifically help us benchmark this code.
> 
> While this change seems like it could be an improvement, I think it would be
> helpful to know if this optimization actually manifests itself in terms of
> any real
> improvement to the product.

I instrumented the PickleTest.EncodeDecode test (calls Write 7 times with
various data and then reads it back out) to reread the Pickle many times to
compare the old Pickle with the new Pickle. I ran each version 8 times and took
the best result for each.

Old code: 1351 ms
New code: 848 ms

37% speedup. I'm sure this is a drop in the bucket, but it's a step in the right
direction.

> 
> -Darin
> 
> 
> 
> >
> >
> >
> >  https://chromiumcodereview.**appspot.com/9447084/diff/**
> >>
>
21002/base/file_path.h<https://chromiumcodereview.appspot.com/9447084/diff/21002/base/file_path.h>
> >> File base/file_path.h (right):
> >>
> >
> >
> > https://chromiumcodereview.**appspot.com/9447084/diff/**
> >
>
21002/base/file_path.h#**newcode343<https://chromiumcodereview.appspot.com/9447084/diff/21002/base/file_path.h#newcode343>
> >
> >> base/file_path.h:343: bool ReadFromPickle(PickleReader* iter);
> >> sorry to nit pick, but it'd be great if you could rename the parameter to
> >> 'reader' instead of 'iter'... or maybe go with PickleIterator?
> >>
> >
> > PickleIterator sounds good.
> >
> >
> >
> >  https://chromiumcodereview.**appspot.com/9447084/diff/**
> >>
>
21002/base/pickle.h<https://chromiumcodereview.appspot.com/9447084/diff/21002/base/pickle.h>
> >> File base/pickle.h (right):
> >>
> >
> >
> > https://chromiumcodereview.**appspot.com/9447084/diff/**
> >
>
21002/base/pickle.h#newcode142<https://chromiumcodereview.appspot.com/9447084/diff/21002/base/pickle.h#newcode142>
> >
> >> base/pickle.h:142: bool ReadBool(PickleReader* iter, bool* result) const {
> >> same nit:  rename 'iter' to 'reader'
> >>
> >
> >
> > https://chromiumcodereview.**appspot.com/9447084/diff/**
> >
>
21002/chrome/browser/**bookmarks/bookmark_node_data.**cc<https://chromiumcodereview.appspot.com/9447084/diff/21002/chrome/browser/bookmarks/bookmark_node_data.cc>
> >
> >> File chrome/browser/bookmarks/**bookmark_node_data.cc (right):
> >>
> >
> >
> > https://chromiumcodereview.**appspot.com/9447084/diff/**
> >
>
21002/chrome/browser/**bookmarks/bookmark_node_data.**cc#newcode57<https://chromiumcodereview.appspot.com/9447084/diff/21002/chrome/browser/bookmarks/bookmark_node_data.cc#newcode57>
> >
> >> chrome/browser/bookmarks/**bookmark_node_data.cc:57: bool
> >> BookmarkNodeData::Element::**ReadFromPickle(Pickle* pickle,
> >> so the idea is to do a follow-up patch that will clean code like this up
> >> to
> >>
> > just
> >
> >> pass PickleReader?
> >>
> >
> > Exactly, that's the plan.
> >
> >
>
https://chromiumcodereview.**appspot.com/9447084/%3Chttps://chromiumcoderevie...>
> >

[jbates, 2012-03-06 22:16:38]

ptal

(I mistakenly merged as well in the latest CL. Luckily the only change is
PickleReader -> PickleIterator).

https://chromiumcodereview.appspot.com/9447084/diff/21002/base/file_path.h
File base/file_path.h (right):

https://chromiumcodereview.appspot.com/9447084/diff/21002/base/file_path.h#ne...
base/file_path.h:343: bool ReadFromPickle(PickleReader* iter);
On 2012/03/06 18:02:13, darin wrote:
> sorry to nit pick, but it'd be great if you could rename the parameter to
> 'reader' instead of 'iter'... or maybe go with PickleIterator?

Done: changed to PickleIterator.

[darin (slow to review), 2012-03-06 23:03:19]

On Tue, Mar 6, 2012 at 11:47 AM, <jbates@chromium.org> wrote:

> On 2012/03/06 18:46:17, darin wrote:
>
>  On Tue, Mar 6, 2012 at 10:23 AM, <mailto:jbates@chromium.org> wrote:
>>
>
>  > On 2012/03/06 18:02:13, darin wrote:
>> >
>> >> what's the performance benefit of this change?  what kinds of
>> measurements
>> >>
>> > have
>> >
>> >> you done?
>> >>
>> >
>> > For each Read method, the old code is:
>> >
>> > 0000000000000000 <_ZNK6Pickle8ReadLongEPPvPl>:
>> >   0:   48 8b 06                mov    (%rsi),%rax
>> >   3:   48 85 c0                test   %rax,%rax
>> >   6:   74 60                   je     68 <_ZNK6Pickle8ReadLongEPPvPl+**
>>
>> > 0x68>
>> >   8:   48 8b 4f 08             mov    0x8(%rdi),%rcx
>> >   c:   48 39 c8                cmp    %rcx,%rax
>> >   f:   72 1b                   jb     2c <_ZNK6Pickle8ReadLongEPPvPl+**
>>
>> > 0x2c>
>> >  11:   45 31 c0                xor    %r8d,%r8d
>> >  14:   48 85 c9                test   %rcx,%rcx
>> >  17:   74 0e                   je     27 <_ZNK6Pickle8ReadLongEPPvPl+**
>>
>> > 0x27>
>> >  19:   49 89 c9                mov    %rcx,%r9
>> >  1c:   44 8b 01                mov    (%rcx),%r8d
>> >  1f:   4c 03 4f 10             add    0x10(%rdi),%r9
>> >  23:   4f 8d 04 01             lea    (%r9,%r8,1),%r8
>> >  27:   4c 39 c0                cmp    %r8,%rax
>> >  2a:   76 04                   jbe    30 <_ZNK6Pickle8ReadLongEPPvPl+**
>>
>> > 0x30>
>> >  2c:   31 c0                   xor    %eax,%eax
>> >  2e:   c3                      retq
>> >  2f:   90                      nop
>> >  30:   49 89 c1                mov    %rax,%r9
>> >  33:   49 83 c1 08             add    $0x8,%r9
>> >  37:   72 f3                   jb     2c <_ZNK6Pickle8ReadLongEPPvPl+**
>>
>> > 0x2c>
>> >  39:   45 31 c0                xor    %r8d,%r8d
>> >  3c:   48 85 c9                test   %rcx,%rcx
>> >  3f:   90                      nop
>> >  40:   74 0c                   je     4e <_ZNK6Pickle8ReadLongEPPvPl+**
>>
>> > 0x4e>
>> >  42:   49 89 c8                mov    %rcx,%r8
>> >  45:   8b 09                   mov    (%rcx),%ecx
>> >  47:   4c 03 47 10             add    0x10(%rdi),%r8
>> >  4b:   49 01 c8                add    %rcx,%r8
>> >  4e:   4d 39 c1                cmp    %r8,%r9
>> >  51:   77 d9                   ja     2c <_ZNK6Pickle8ReadLongEPPvPl+**
>>
>> > 0x2c>
>> >  53:   48 8b 00                mov    (%rax),%rax
>> >  56:   48 89 02                mov    %rax,(%rdx)
>> >  59:   b8 01 00 00 00          mov    $0x1,%eax
>> >  5e:   48 83 06 08             addq   $0x8,(%rsi)
>> >  62:   c3                      retq
>> >  63:   0f 1f 44 00 00          nopl   0x0(%rax,%rax,1)
>> >  68:   48 8b 47 08             mov    0x8(%rdi),%rax
>> >  6c:   48 03 47 10             add    0x10(%rdi),%rax
>> >  70:   48 89 06                mov    %rax,(%rsi)
>> >  73:   eb 93                   jmp    8 <_ZNK6Pickle8ReadLongEPPvPl+***
>> *0x8>
>>
>> >
>> > With this CL, Read methods look like this:
>> >
>> > 0000000000000000 <_ZN6Pickle9ReadLong2EPl>:
>> >   0:   48 8b 57 28             mov    0x28(%rdi),%rdx
>> >   4:   31 c0                   xor    %eax,%eax
>> >   6:   48 8d 4a 08             lea    0x8(%rdx),%rcx
>> >   a:   48 39 4f 38             cmp    %rcx,0x38(%rdi)
>> >   e:   73 08                   jae    18 <_ZN6Pickle9ReadLong2EPl+0x18>
>> >  10:   f3 c3                   repz retq
>> >  12:   66 0f 1f 44 00 00       nopw   0x0(%rax,%rax,1)
>> >  18:   48 8b 02                mov    (%rdx),%rax
>> >  1b:   48 89 06                mov    %rax,(%rsi)
>> >  1e:   b8 01 00 00 00          mov    $0x1,%eax
>> >  23:   48 83 47 28 08          addq   $0x8,0x28(%rdi)
>> >  28:   c3                      retq
>> >
>> > The code size reduction reduces icache pressure and fewer branches make
>> it
>> > faster. Do we have any micro perf tests that measure this kind of code?
>>
>
>
>
>  OK, but there are very few Read methods.  I don't know of any tests that
>> would
>> specifically help us benchmark this code.
>>
>
>  While this change seems like it could be an improvement, I think it would
>> be
>> helpful to know if this optimization actually manifests itself in terms of
>> any real
>> improvement to the product.
>>
>
> I instrumented the PickleTest.EncodeDecode test (calls Write 7 times with
> various data and then reads it back out) to reread the Pickle many times to
> compare the old Pickle with the new Pickle. I ran each version 8 times and
> took
> the best result for each.
>
> Old code: 1351 ms
> New code: 848 ms
>
> 37% speedup. I'm sure this is a drop in the bucket, but it's a step in the
> right
> direction.
>
>
Well, that is helpful.  At the very least, this is a code simplification
for consumers.

LGTM

-Darin



>
>  -Darin
>>
>
>
>
>  >
>> >
>> >
>> > 
https://chromiumcodereview.**a**ppspot.com/9447084/diff/**<http://appspot.com...
>> >>
>>
>
> 21002/base/file_path.h<https:/**/chromiumcodereview.appspot.**
>
com/9447084/diff/21002/base/**file_path.h<https://chromiumcodereview.appspot.com/9447084/diff/21002/base/file_path.h>
> >
>
>> >> File base/file_path.h (right):
>> >>
>> >
>> >
>> >
https://chromiumcodereview.**a**ppspot.com/9447084/diff/**<http://appspot.com...
>> >
>>
>
> 21002/base/file_path.h#****newcode343<https://**
> chromiumcodereview.appspot.**com/9447084/diff/21002/base/**
>
file_path.h#newcode343<https://chromiumcodereview.appspot.com/9447084/diff/21002/base/file_path.h#newcode343>
> >
>
>> >
>> >> base/file_path.h:343: bool ReadFromPickle(PickleReader* iter);
>> >> sorry to nit pick, but it'd be great if you could rename the parameter
>> to
>> >> 'reader' instead of 'iter'... or maybe go with PickleIterator?
>> >>
>> >
>> > PickleIterator sounds good.
>> >
>> >
>> >
>> > 
https://chromiumcodereview.**a**ppspot.com/9447084/diff/**<http://appspot.com...
>> >>
>>
>
> 21002/base/pickle.h<https://**chromiumcodereview.appspot.**
>
com/9447084/diff/21002/base/**pickle.h<https://chromiumcodereview.appspot.com/9447084/diff/21002/base/pickle.h>
> >
>
>> >> File base/pickle.h (right):
>> >>
>> >
>> >
>> >
https://chromiumcodereview.**a**ppspot.com/9447084/diff/**<http://appspot.com...
>> >
>>
>
> 21002/base/pickle.h#**newcode142<https://**chromiumcodereview.appspot.**
>
com/9447084/diff/21002/base/**pickle.h#newcode142<https://chromiumcodereview.appspot.com/9447084/diff/21002/base/pickle.h#newcode142>
> >
>
>> >
>> >> base/pickle.h:142: bool ReadBool(PickleReader* iter, bool* result)
>> const {
>> >> same nit:  rename 'iter' to 'reader'
>> >>
>> >
>> >
>> >
https://chromiumcodereview.**a**ppspot.com/9447084/diff/**<http://appspot.com...
>> >
>>
>
> 21002/chrome/browser/****bookmarks/bookmark_node_data.****cc<https://**
> chromiumcodereview.appspot.**com/9447084/diff/21002/chrome/**
>
browser/bookmarks/bookmark_**node_data.cc<https://chromiumcodereview.appspot.com/9447084/diff/21002/chrome/browser/bookmarks/bookmark_node_data.cc>
> >
>
>> >
>> >> File chrome/browser/bookmarks/****bookmark_node_data.cc (right):
>> >>
>> >
>> >
>> >
https://chromiumcodereview.**a**ppspot.com/9447084/diff/**<http://appspot.com...
>> >
>>
>
> 21002/chrome/browser/****bookmarks/bookmark_node_data.****cc#newcode57<
> https://**chromiumcodereview.appspot.**com/9447084/diff/21002/chrome/**
>
browser/bookmarks/bookmark_**node_data.cc#newcode57<https://chromiumcodereview.appspot.com/9447084/diff/21002/chrome/browser/bookmarks/bookmark_node_data.cc#newcode57>
> >
>
>> >
>> >> chrome/browser/bookmarks/****bookmark_node_data.cc:57: bool
>> >> BookmarkNodeData::Element::****ReadFromPickle(Pickle* pickle,
>>
>> >> so the idea is to do a follow-up patch that will clean code like this
>> up
>> >> to
>> >>
>> > just
>> >
>> >> pass PickleReader?
>> >>
>> >
>> > Exactly, that's the plan.
>> >
>> >
>>
>
> https://chromiumcodereview.**a**ppspot.com/9447084/%3Chttps://**
>
chromiumcodereview.appspot.**com/9447084/<http://appspot.com/9447084/%3Chttps://chromiumcodereview.appspot.com/9447084/>
> >
>
>> >
>>
>
>
>
https://chromiumcodereview.**appspot.com/9447084/<https://chromiumcodereview....
>