New adjustment method for Courgette....

New adjustment method for Courgette.
Slower, but better.

1.0.154.59 to 1.0.154.65
Old: 1m19s,  354,997 bytes
New: 4m01s,  279,798 bytes

Timings on Lenovo T61 (T7700 cpu)

BUG=none
TEST=none (existing tests)


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=17566

[sra, 2009-05-29 22:17:29]

The 120 line comment is the place to start.

[cpu_(ooo_6.6-7.5), 2009-06-01 19:37:11]

This is really cool. I wish I understood it. Some stylistic comments. I would
like a second pass at it in case I can actually add some real value.

The comments are awesome.

http://codereview.chromium.org/118031/diff/1/3
File courgette/adjustment_method_2.cc (right):

http://codereview.chromium.org/118031/diff/1/3#newcode221
Line 221: candidates_(NULL)
move candidates to previous line or use single row initializers.

http://codereview.chromium.org/118031/diff/1/3#newcode231
Line 231: // inside a std::map.
lol, we neeed ALLOW_DEFAULT_COPY_CTOR macro  :)

http://codereview.chromium.org/118031/diff/1/3#newcode236
Line 236: static std::string ToString(const LabelInfo* info) {
remind me .. what static gives you here?

http://codereview.chromium.org/118031/diff/1/3#newcode254
Line 254: 
please use the style guide format for classes including order of public,
protected, private. This comment applies to several classes in this file.

http://codereview.chromium.org/118031/diff/1/3#newcode319
Line 319: ++adjs_;
adjs_ neeeds a better name.

http://codereview.chromium.org/118031/diff/1/3#newcode349
Line 349: // private:
// private ?

http://codereview.chromium.org/118031/diff/1/3#newcode392
Line 392: ShinglePattern* pattern_;
if we are going to have exposed members and no inherintance I think we rather be
a struct.

http://codereview.chromium.org/118031/diff/1/3#newcode398
Line 398: };
looks like we don't want the default ctors in this class

http://codereview.chromium.org/118031/diff/1/3#newcode438
Line 438: enum { OFFSET_MASK = 7,
I think that enum is not style compliant :)

http://codereview.chromium.org/118031/diff/1/3#newcode459
Line 459: std::set<Shingle*, Shingle::PointerLess> model_instances_;    // ??
order.
remove "??"

http://codereview.chromium.org/118031/diff/1/3#newcode461
Line 461: 
I would be good to put some quick description of FreqView like in lies 705-706

http://codereview.chromium.org/118031/diff/1/3#newcode478
Line 478: typedef std::set<FreqView, FreqView::Greater> Histogram;
ditto here, if the members are exposed, why not being a struct?

http://codereview.chromium.org/118031/diff/1/3#newcode847
Line 847: #if 0
if ALOG itself is under #if do we need this blocks under #if ?

http://codereview.chromium.org/118031/diff/1/3#newcode1173
Line 1173: #if 1
remove #if 1 ?

[sra, 2009-06-02 17:02:12]

http://codereview.chromium.org/118031/diff/1/3
File courgette/adjustment_method_2.cc (right):

http://codereview.chromium.org/118031/diff/1/3#newcode236
Line 236: static std::string ToString(const LabelInfo* info) {
On 2009/06/01 19:37:11, cpu wrote:
> remind me .. what static gives you here?

I'll remove it.  (It did avoid a name clash before this code was all in its own
namespace.)

http://codereview.chromium.org/118031/diff/1/3#newcode254
Line 254: 
On 2009/06/01 19:37:11, cpu wrote:
> please use the style guide format for classes including order of public,
> protected, private. This comment applies to several classes in this file.

Done.

http://codereview.chromium.org/118031/diff/1/3#newcode319
Line 319: ++adjs_;
On 2009/06/01 19:37:11, cpu wrote:
> adjs_ neeeds a better name.

Or better yet, complete elimination!

http://codereview.chromium.org/118031/diff/1/3#newcode349
Line 349: // private:
On 2009/06/01 19:37:11, cpu wrote:
> // private ?

Done.

http://codereview.chromium.org/118031/diff/1/3#newcode392
Line 392: ShinglePattern* pattern_;
On 2009/06/01 19:37:11, cpu wrote:
> if we are going to have exposed members and no inherintance I think we rather
be
> a struct.

Or hide the members under accessors, which is what I have done.

http://codereview.chromium.org/118031/diff/1/3#newcode398
Line 398: };
On 2009/06/01 19:37:11, cpu wrote:
> looks like we don't want the default ctors in this class

One would think so, but apparently we need one.

http://codereview.chromium.org/118031/diff/1/3#newcode438
Line 438: enum { OFFSET_MASK = 7,
On 2009/06/01 19:37:11, cpu wrote:
> I think that enum is not style compliant :)

Done.

http://codereview.chromium.org/118031/diff/1/3#newcode459
Line 459: std::set<Shingle*, Shingle::PointerLess> model_instances_;    // ??
order.
On 2009/06/01 19:37:11, cpu wrote:
> remove "??"

Done.

http://codereview.chromium.org/118031/diff/1/3#newcode461
Line 461: 
On 2009/06/01 19:37:11, cpu wrote:
> I would be good to put some quick description of FreqView like in lies 705-706

Done.

http://codereview.chromium.org/118031/diff/1/3#newcode478
Line 478: typedef std::set<FreqView, FreqView::Greater> Histogram;
On 2009/06/01 19:37:11, cpu wrote:
> ditto here, if the members are exposed, why not being a struct?

Made private, with accessors where necessary

http://codereview.chromium.org/118031/diff/1/3#newcode847
Line 847: #if 0
On 2009/06/01 19:37:11, cpu wrote:
> if ALOG itself is under #if do we need this blocks under #if ?

This printing is very verbose so not all that helpful unless you break a certain
part of the program.
I've split it into separate functions to make commenting-out/in easier.

http://codereview.chromium.org/118031/diff/1/3#newcode1173
Line 1173: #if 1
On 2009/06/01 19:37:11, cpu wrote:
> remove #if 1 ?

Done.

[cpu_(ooo_6.6-7.5), 2009-06-03 00:12:36]

LGTM

http://codereview.chromium.org/118031/diff/2001/2003
File courgette/adjustment_method_2.cc (right):

http://codereview.chromium.org/118031/diff/2001/2003#newcode537
Line 537: // std::set<Shingle*, Shingle::PointerLess> program_instances_;
Looks like these two are not used anymore.

[sra, 2009-06-03 02:40:53]

http://codereview.chromium.org/118031/diff/2001/2003
File courgette/adjustment_method_2.cc (right):

http://codereview.chromium.org/118031/diff/2001/2003#newcode537
Line 537: // std::set<Shingle*, Shingle::PointerLess> program_instances_;
On 2009/06/03 00:12:37, cpu wrote:
> Looks like these two are not used anymore.

Removed.