Re: [PATCH] low-hanging fruit

[Lars Gullik Bjønnes]

Andre Poenitz [EMAIL PROTECTED] writes:

| I'd actually prefer to put one-liners directly in the class definition in
| my own coding as a matter of convienience and better readability, too. Of
| course, the latter is arguable, but if I see
| 
|size_type size() const
|  { return data_.size(); }

Yes, but too often this is not one-liners, but two-liners on one line:

 size_type size() const { Assert(...); return data_.size(); }

And this is something that I really like.

I also belive that the class definition in itself become a lot more
cleaner when the inlines are put _after_ the definition. _If_ the you
have to scroll "far" to to get from the declaration to the definition
there is probably something bad with the class' structure it should
probably be split (and yes we have this problem in several LyX
classes)
  
| This does not only quadruple the amount of typing (which is a pain for the
| wrists, I am _very_ aware of that) and occupies twice as much space on
| screen, it is also takes more key presses for moving around.

ok... I don't have any wrist problem...

| 
| And it gets even worse with templates... 
| 
| No, I am not advocating "my style",  I am pretty happy with current LyX as
| long as it stays uniform. I am just trying to support Asgers words

I choose this way sine it is often what I see suggested by really good
C++ programmers.
 
| " The code is often easier to understand when the definition is close to the
|   declaration for these bits of code that are really simple. "
| 
| , in "real code" we prefer 'int i = 5;'  over 'int i; [...] i = 5;' too,
| don't we?

Yes, but that is not only a "keep-declaration-and-definition" close
rule, but also a "dont-initilize-twice" rule.

Lgb

Re: [PATCH] low-hanging fruit

[Andre Poenitz]

 Yes, but too often this is not one-liners, but two-liners on one line:
 
  size_type size() const { Assert(...); return data_.size(); }

Those are gone now, aren't they? ;-)

Ok, I admit, my personal limit in such cases is about four lines, i.e.
usually one or two initializations, and call to an algorithm or a loop and
a return.  But I was pretty sure you wouldn't accept any example with
more than one statement anyway...

 I also belive that the class definition in itself become a lot more
 cleaner when the inlines are put _after_ the definition.

It is certainly a religious matter. If I see only the declaration, I have
to guess what it does, or to read the describing comment (if available) and
hope that it is not outdated, or to scroll down to the definition.

There is no need to guess or to hope or to scroll if the code follows
immediately. It is simply there. And the chances that obviously
misleading comments pass review are pretty slim...

 _If_ the you have to scroll "far" to to get from the declaration to the
 definition there is probably something bad with the class' structure it
 should probably be split (and yes we have this problem in several LyX
 classes)

That's certainly true. And current LyX classes are too fat in most cases...

 ok... I don't have any wrist problem...

I have from time to time. No fun.

 I choose this way sine it is often what I see suggested by really good
 C++ programmers.

I accept all of your points, they have influence me as well. It is just
that I give some points another weight so the outcome is different. And as
I said, I could live with LyX's style _very_ well, too.

Andre'

-- 
Andr Pnitz  [EMAIL PROTECTED]

Re: [PATCH] low-hanging fruit

[Lars Gullik Bjønnes]

Andre Poenitz <[EMAIL PROTECTED]> writes:

| I'd actually prefer to put one-liners directly in the class definition in
| my own coding as a matter of convienience and better readability, too. Of
| course, the latter is arguable, but if I see
| 
|size_type size() const
|  { return data_.size(); }

Yes, but too often this is not one-liners, but two-liners on one line:

 size_type size() const { Assert(...); return data_.size(); }

And this is something that I really like.

I also belive that the class definition in itself become a lot more
cleaner when the inlines are put _after_ the definition. _If_ the you
have to scroll "far" to to get from the declaration to the definition
there is probably something bad with the class' structure it should
probably be split (and yes we have this problem in several LyX
classes)
  
| This does not only quadruple the amount of typing (which is a pain for the
| wrists, I am _very_ aware of that) and occupies twice as much space on
| screen, it is also takes more key presses for moving around.

ok... I don't have any wrist problem...

| 
| And it gets even worse with templates... 
| 
| No, I am not advocating "my style",  I am pretty happy with current LyX as
| long as it stays uniform. I am just trying to support Asgers words

I choose this way sine it is often what I see suggested by really good
C++ programmers.
 
| " The code is often easier to understand when the definition is close to the
|   declaration for these bits of code that are really simple. "
| 
| , in "real code" we prefer 'int i = 5;'  over 'int i; [...] i = 5;' too,
| don't we?

Yes, but that is not only a "keep-declaration-and-definition" close
rule, but also a "dont-initilize-twice" rule.

Lgb

Re: [PATCH] low-hanging fruit

[Andre Poenitz]

> Yes, but too often this is not one-liners, but two-liners on one line:
> 
>  size_type size() const { Assert(...); return data_.size(); }

Those are gone now, aren't they? ;-)

Ok, I admit, my personal limit in such cases is about four lines, i.e.
usually one or two initializations, and call to an algorithm or a loop and
a return.  But I was pretty sure you wouldn't accept any example with
more than one statement anyway...

> I also belive that the class definition in itself become a lot more
> cleaner when the inlines are put _after_ the definition.

It is certainly a religious matter. If I see only the declaration, I have
to guess what it does, or to read the describing comment (if available) and
hope that it is not outdated, or to scroll down to the definition.

There is no need to guess or to hope or to scroll if the code follows
immediately. It is simply there. And the chances that obviously
misleading comments pass review are pretty slim...

> _If_ the you have to scroll "far" to to get from the declaration to the
> definition there is probably something bad with the class' structure it
> should probably be split (and yes we have this problem in several LyX
> classes)

That's certainly true. And current LyX classes are too fat in most cases...

> ok... I don't have any wrist problem...

I have from time to time. No fun.

> I choose this way sine it is often what I see suggested by really good
> C++ programmers.

I accept all of your points, they have influence me as well. It is just
that I give some points another weight so the outcome is different. And as
I said, I could live with LyX's style _very_ well, too.

Andre'

-- 
André Pönitz  [EMAIL PROTECTED]

[PATCH] low-hanging fruit

[John Levon]

Doing lyx -x lyx-quit big.lyx,  the attached patch
gives the following results (warm cache, error around 4%) :

user (s)| sys (s)   | elapsed (s)   |
--
28.33 (0.00%)   | 0.30 (0.00%)  | 29.61 (0.00%) | newinsetsold
24.29 (-14.27%) | 0.32 (6.61%)  | 25.92 (-12.48%)   | newinsetsnew

and the binary is only 14k bigger (with -g -O). This seems worth it to me.

The top of the profile looks like this with the patch applied :
 
LyXParagraph::InsertChar(int, char, LyXFont const )[0x08146e5c]: 1.0553% (10233 
samples)
LyXParagraph::GetChar(int) const[0x08148210]: 1.1648% (11295 samples)
basic_string...::compare(char const *, unsigned int, unsigned int) 
const[0x08288a38]: 1.3048% (12652 samples)
LyXText::GetFont(Buffer const *, LyXParagraph *, int) const[0x08173358]: 1.4893% 
(14441 samples)
LyXFont::FontBits::eq(LyXFont::FontBits const ) const[0x081110f4]: 1.5215% (14753 
samples)
LyXFont::realize(LyXFont const )[0x08111af0]: 1.5622% (15148 samples)
boost::shared_ptrLyXFont * find_ifboost::shared_ptrLyXFont *, 
ShareContainerLyXFont::isEqual(boost::shared_ptrLyXFont *, 
boost::shared_ptrLyXFont *, ShareContainerLyXFont::isEqual, 
random_access_iterator_tag)[0x082d5dc0]: 1.6546% (16044 samples)
Buffer::parseSingleLyXformat2Token(LyXLex , LyXParagraph *, LyXParagraph *, 
basic_stringchar, string_char_traitschar, __default_alloc_templatetrue, 0  const 
, int , char , LyXFont )[0x080a45d4]: 1.8889% (18316 samples)
__7LyXFont[0x0870]: 2.3485% (22772 samples)
LyXParagraph::GetFontSettings(BufferParams const , int) const[0x0814750c]: 3.3957% 
(32927 samples)
LyXText::GetRow(LyXParagraph *, int, int ) const[0x08172b40]: 63.7895% (618541 
samples)


thanks
john

-- 
"24-hour boredom
I'm convicted instantly"
- Manic Street Preachers


Index: src/ChangeLog
===
RCS file: /usr/local/lyx/cvsroot/lyx-devel/src/ChangeLog,v
retrieving revision 1.95
diff -u -p -r1.95 ChangeLog
--- src/ChangeLog   2001/03/15 10:06:52 1.95
+++ src/ChangeLog   2001/03/15 11:53:15
@@ -1,3 +1,10 @@
+2001-03-15  John Levon  [EMAIL PROTECTED]
+
+   * lyxrow.h:
+   * lyxrow.C: inline some get() accessors. This
+   adds about 14k to the binary but speeds up a
+   large file load by about 10%.
+
 2001-03-14  Angus Leeming  [EMAIL PROTECTED]
 
* lyxfunc.C (Dispatch): removed redundant break statement.
Index: src/lyxrow.C
===
RCS file: /usr/local/lyx/cvsroot/lyx-devel/src/lyxrow.C,v
retrieving revision 1.4
diff -u -p -r1.4 lyxrow.C
--- src/lyxrow.C2000/07/24 13:53:17 1.4
+++ src/lyxrow.C2001/03/15 11:53:15
@@ -29,30 +29,12 @@ void Row::par(LyXParagraph * p)
 }
 
 
-LyXParagraph * Row::par()
-{
-   return par_;
-}
-
-
-LyXParagraph * Row::par() const
-{
-   return par_;
-}
-
-
 void Row::pos(LyXParagraph::size_type p)
 {
pos_ = p;
 }
 
 
-LyXParagraph::size_type Row::pos() const
-{
-   return pos_;
-}
-
-
 void Row::fill(int f)
 {
fill_ = f;
@@ -71,24 +53,12 @@ void Row::height(unsigned short h)
 }
 
 
-unsigned short Row::height() const
-{
-   return height_;
-}
-
-
 void Row::width(unsigned int w)
 {
width_ = w;
 }
 
 
-unsigned int Row::width() const
-{
-   return width_;
-}
-
-
 void Row::ascent_of_text(unsigned short a)
 {
ascent_of_text_ = a;
@@ -119,19 +89,7 @@ void Row::next(Row * r)
 }
 
 
-Row * Row::next() const
-{
-   return next_;
-}
-
-
 void Row::previous(Row * r)
 {
previous_ = r;
-}
-
-
-Row * Row::previous() const
-{
-   return previous_;
 }
Index: src/lyxrow.h
===
RCS file: /usr/local/lyx/cvsroot/lyx-devel/src/lyxrow.h,v
retrieving revision 1.10
diff -u -p -r1.10 lyxrow.h
--- src/lyxrow.h2000/07/24 13:53:17 1.10
+++ src/lyxrow.h2001/03/15 11:53:15
@@ -26,13 +26,13 @@ public:
///
void par(LyXParagraph * p);
///
-   LyXParagraph * par();
+   inline LyXParagraph * par();
///
-   LyXParagraph * par() const;
+   inline LyXParagraph * par() const;
///
void pos(LyXParagraph::size_type p);
///
-   LyXParagraph::size_type pos() const;
+   inline LyXParagraph::size_type pos() const;
///
void fill(int f);
///
@@ -40,11 +40,11 @@ public:
///
void height(unsigned short h);
///
-   unsigned short height() const;
+   inline unsigned short height() const;
///
void width(unsigned int w);
///
-   unsigned int width() const;
+   inline unsigned int width() const;
///
void ascent_of_text(unsigned short 

Re: [PATCH] low-hanging fruit

[Lars Gullik Bjønnes]

John Levon [EMAIL PROTECTED] writes:

| Doing lyx -x lyx-quit big.lyx,  the attached patch
| gives the following results (warm cache, error around 4%) :
| 
| user (s)| sys (s)   | elapsed (s)   |
| --
| 28.33 (0.00%)   | 0.30 (0.00%)  | 29.61 (0.00%) | newinsetsold
| 24.29 (-14.27%) | 0.32 (6.61%)  | 25.92 (-12.48%)   | newinsetsnew
| 
| and the binary is only 14k bigger (with -g -O). This seems worth it
| to me.

Measure size with 'size'.

Lgb

Re: [PATCH] low-hanging fruit

[John Levon]

On 15 Mar 2001, Lars Gullik Bjønnes wrote:

 John Levon [EMAIL PROTECTED] writes:
 
 | Doing lyx -x lyx-quit big.lyx,  the attached patch
 | gives the following results (warm cache, error around 4%) :
 | 
 | user (s)| sys (s)   | elapsed (s)   |
 | --
 | 28.33 (0.00%)   | 0.30 (0.00%)  | 29.61 (0.00%) | newinsetsold
 | 24.29 (-14.27%) | 0.32 (6.61%)  | 25.92 (-12.48%)   | newinsetsnew
 | 
 | and the binary is only 14k bigger (with -g -O). This seems worth it
 | to me.
 
 Measure size with 'size'.
 
 Lgb

ugh, yes, sorry

hold on

john

-- 
"24-hour boredom
I'm convicted instantly"
- Manic Street Preachers

Re: [PATCH] low-hanging fruit

[Lars Gullik Bjønnes]

John Levon [EMAIL PROTECTED] writes:

| Doing lyx -x lyx-quit big.lyx,  the attached patch
| gives the following results (warm cache, error around 4%) :
| 
| user (s)| sys (s)   | elapsed (s)   |
| --
| 28.33 (0.00%)   | 0.30 (0.00%)  | 29.61 (0.00%) | newinsetsold
| 24.29 (-14.27%) | 0.32 (6.61%)  | 25.92 (-12.48%)   | newinsetsnew
| 
| and the binary is only 14k bigger (with -g -O). This seems worth it to me.

I don't want this patch yet.

If you have lines like:

do_something(with_this.size());

and widh_this.size() is inlined and do_something segfaults, the
segfault can look like it happened inside with_this.size(). 

Also when having inlines in the header file, alway put the inlined
method after the class definithin with an inline, in the class inline
should not be used:

class Foo {
publi:
int size();
};

inline
Foo::size() {
return ...;
}


As you can see with this it is _very_ easy to move the inlined method
from the header file to the C file (and back).

I have been thinking of a scheme where we can choose at compile time
if we want to have methods like this inlined or not.

foo.h:
class Foo {
public:
int size();
};

#ifdef INLINE_SIMPLE_METHODS
#define INLINE inline
#include "foo_inlines.h"
#endif

foo_inlines.h:
INLINE
Foo::size() {
return ...;
}

foo.C:
#ifndef INLINE_SIMPLE_METHODS
#define INLINE
#include "foo_inlines.h"
#endif


Or another scheme that gives the same effect.

Lgb

Re: [PATCH] low-hanging fruit

[John Levon]

On Thu, 15 Mar 2001, John Levon wrote:

 On 15 Mar 2001, Lars Gullik Bjønnes wrote:
 
  John Levon [EMAIL PROTECTED] writes:
  
  | Doing lyx -x lyx-quit big.lyx,  the attached patch
  | gives the following results (warm cache, error around 4%) :
  | 
  | user (s)| sys (s)   | elapsed (s)   |
  | --
  | 28.33 (0.00%)   | 0.30 (0.00%)  | 29.61 (0.00%) | newinsetsold
  | 24.29 (-14.27%) | 0.32 (6.61%)  | 25.92 (-12.48%)   | newinsetsnew
  | 
  | and the binary is only 14k bigger (with -g -O). This seems worth it
  | to me.
  
  Measure size with 'size'.
  
  Lgb

hum, now I'm really confused :

pg008:1050 size lyx
   textdata bss dec hex filename
2643515   19332   43100 2705947  294a1b lyx
pg008:1051 size lyxnew
   textdata bss dec hex filename
2638043   19332   43100 2700475  2934bb lyxnew
pg008:1053 expr 2638043 - 2643515
-5472

"lyxnew" is the one with the patch !!!

huh ??

john

-- 
"24-hour boredom
I'm convicted instantly"
- Manic Street Preachers

Re: [PATCH] low-hanging fruit

[John Levon]

On 15 Mar 2001, Lars Gullik Bjønnes wrote:

 John Levon [EMAIL PROTECTED] writes:
 
 | Doing lyx -x lyx-quit big.lyx,  the attached patch
 | gives the following results (warm cache, error around 4%) :
 | 
 | user (s)| sys (s)   | elapsed (s)   |
 | --
 | 28.33 (0.00%)   | 0.30 (0.00%)  | 29.61 (0.00%) | newinsetsold
 | 24.29 (-14.27%) | 0.32 (6.61%)  | 25.92 (-12.48%)   | newinsetsnew
 | 
 | and the binary is only 14k bigger (with -g -O). This seems worth it to me.
 
 I don't want this patch yet.
 
 If you have lines like:
 
 do_something(with_this.size());
 
 and widh_this.size() is inlined and do_something segfaults, the
 segfault can look like it happened inside with_this.size(). 

and this is reason enough to not have this speedup ??

 Also when having inlines in the header file, alway put the inlined
 method after the class definithin with an inline, in the class inline
 should not be used:

ok, sorry

 #ifdef INLINE_SIMPLE_METHODS
 #define INLINE inline
 #include "foo_inlines.h"
 #endif
 
 foo_inlines.h:
 INLINE
 Foo::size() {
 return ...;
 }
 
 foo.C:
 #ifndef INLINE_SIMPLE_METHODS
 #define INLINE
 #include "foo_inlines.h"
 #endif

well as described, if not inlined this will have copies of Foo:size()

Also I don't think IMHO it makes sense to inline hardly-used accessor methods
(or maybe it does). I suppose it depends on architecture what kind of effect
this will have on .text size

thanks
john

-- 
"24-hour boredom
I'm convicted instantly"
- Manic Street Preachers

Re: [PATCH] low-hanging fruit

[Lars Gullik Bjønnes]

John Levon [EMAIL PROTECTED] writes:

| hum, now I'm really confused :
| 
| pg008:1050 size lyx
|textdata bss dec hex filename
| 2643515   19332   43100 2705947  294a1b lyx
| pg008:1051 size lyxnew
|textdata bss dec hex filename
| 2638043   19332   43100 2700475  2934bb lyxnew
| pg008:1053 expr 2638043 - 2643515
| -5472
| 
| "lyxnew" is the one with the patch !!!
| 
| huh ??

So it inlined nicely and the function call is completely removed and no
code generated for it.

Lgb

Re: [PATCH] low-hanging fruit

[John Levon]

On 15 Mar 2001, Lars Gullik Bjønnes wrote:

 John Levon [EMAIL PROTECTED] writes:
 
 | hum, now I'm really confused :
 | 
 | pg008:1050 size lyx
 |textdata bss dec hex filename
 | 2643515   19332   43100 2705947  294a1b lyx
 | pg008:1051 size lyxnew
 |textdata bss dec hex filename
 | 2638043   19332   43100 2700475  2934bb lyxnew
 | pg008:1053 expr 2638043 - 2643515
 | -5472
 | 
 | "lyxnew" is the one with the patch !!!
 | 
 | huh ??
 
 So it inlined nicely and the function call is completely removed and no
 code generated for it.
 
 Lgb

Yes I realised after I sent it it made sense. I'm not sure why "ls" at home
claimed it was larger ... (here ls has the same difference too)

I really do think the speedup is significant enough to live with the debugging 
problem...
(and it will also apply to normal use of lyx rather than just loading as far as I can 
see ?)

thanks
john

-- 
"24-hour boredom
I'm convicted instantly"
- Manic Street Preachers

Re: [PATCH] low-hanging fruit

[Lars Gullik Bjønnes]

John Levon [EMAIL PROTECTED] writes:

|  and widh_this.size() is inlined and do_something segfaults, the
|  segfault can look like it happened inside with_this.size(). 
| 
| and this is reason enough to not have this speedup ??

Yes, I belive a more detrministic development environment is reason
enough.

And as said, I am thinking about schemes to inline importent small
methods again, but not just now.
 
|  Also when having inlines in the header file, alway put the inlined
|  method after the class definithin with an inline, in the class inline
|  should not be used:
| 
| ok, sorry
| 
|  #ifdef INLINE_SIMPLE_METHODS
|  #define INLINE inline
|  #include "foo_inlines.h"
|  #endif
|  
|  foo_inlines.h:
|  INLINE
|  Foo::size() {
|  return ...;
|  }
|  
|  foo.C:
|  #ifndef INLINE_SIMPLE_METHODS
|  #define INLINE
|  #include "foo_inlines.h"
|  #endif
| 
| well as described, if not inlined this will have copies of Foo:size()

??? yes, that was the purpose...
 
| Also I don't think IMHO it makes sense to inline hardly-used accessor methods
| (or maybe it does). I suppose it depends on architecture what kind of effect
| this will have on .text size

One-liner getter methods will almost alway inline perfectly and
generate less code than a "full" method. So I belive that in most
cases getters should be made inline, most of the setters should be
inline (if very simple). Methods that are only wrappers around other
methods can be inlined.

Lgb

Re: [PATCH] low-hanging fruit

[John Levon]

On 15 Mar 2001, Lars Gullik Bjønnes wrote:

 John Levon [EMAIL PROTECTED] writes:
 
 |  and widh_this.size() is inlined and do_something segfaults, the
 |  segfault can look like it happened inside with_this.size(). 
 | 
 | and this is reason enough to not have this speedup ??
 
 Yes, I belive a more detrministic development environment is reason
 enough.

your call

 | well as described, if not inlined this will have copies of Foo:size()
 
 ??? yes, that was the purpose...

I mean there will be one in foo.C AND non-inlined copies in anything including foo.h. 
Or am I talking crap again

john

-- 
"24-hour boredom
I'm convicted instantly"
- Manic Street Preachers

Re: [PATCH] low-hanging fruit

[Lars Gullik Bjønnes]

John Levon [EMAIL PROTECTED] writes:

| I mean there will be one in foo.C AND non-inlined copies in anything including 
|foo.h. 
| Or am I talking crap again

foo_inlines.h should only be included in foo.h if
INLINE_SIMPLE_METHODS is defined.

Lgb

Re: [PATCH] low-hanging fruit

[John Levon]

On 15 Mar 2001, Lars Gullik Bjønnes wrote:

 foo_inlines.h should only be included in foo.h if
 INLINE_SIMPLE_METHODS is defined.
 
 Lgb

*sigh*

I really do have problems with reading :P

john 

-- 
"24-hour boredom
I'm convicted instantly"
- Manic Street Preachers

Re: [PATCH] low-hanging fruit

[Juergen Vigna]

On 15-Mar-2001 John Levon wrote:

 I really do think the speedup is significant enough to live with the debugging
 problem...

What debugging problem? I compile with only -g option and then inlined code
is no problem on debugging!

   Jrgen

--
-._-._-._-._-._-._-._-._-._-._-._-._-._-._-._-._-._-._-._-._-._-._-._
Dr. Jrgen VignaE-Mail:  [EMAIL PROTECTED]
Italienallee 13/N   Tel/Fax: +39-0471-450260 / +39-0471-450253
I-39100 Bozen   Web: http://www.sad.it/~jug
-._-._-._-._-._-._-._-._-._-._-._-._-._-._-._-._-._-._-._-._-._-._-._

Reality is just a convenient measure of complexity.
-- Alvy Ray Smith

Re: [PATCH] low-hanging fruit

[Lars Gullik Bjønnes]

Juergen Vigna [EMAIL PROTECTED] writes:

| On 15-Mar-2001 John Levon wrote:
| 
|  I really do think the speedup is significant enough to live with the debugging
|  problem...
| 
| What debugging problem? I compile with only -g option and then inlined code
| is no problem on debugging!

Well I consider compiling without -O an even bigger problem. Then a lot
of small problems will not be noticed until you compile with
optimization turned on.

Lgb

Re: [PATCH] low-hanging fruit

[Asger K. Alstrup Nielsen]

On 15 Mar 2001, Lars Gullik Bjnnes wrote:

[How to avoid inlining to improve debugging]
 Well I consider compiling without -O an even bigger problem. Then a lot
 of small problems will not be noticed until you compile with
 optimization turned on.

Are you sure that there is not a flag to gcc such that you can compile
with -O, but without inlining?

I don't think it's a good trade-off to clutter all inlinings with
several lines of conditional preprocessor voodoo, just to avoid
a largely hypothetical problem!  Notice that most methods that
you inline are fairly trivial, and it's, in my experience, extremely
rare that you want to single-step through those.

I think this problem should be solved via a compiler switch - not
by cluttering the code.  If the compiler can't do this, fix the compiler
rather than cluttering the code.

Also, I don't think it's not a good trade-off to allow easy moving of
inline methods back and forth between the cpp/cxx/c++ files and the header
files by using the inline keyword.
The code is often easier to understand when the definition is close to the
declaration for these bits of code that are really simple.

It's not important that changing whether a method is inline or not should
be an easy operation. You only do this operation when you optimize the
code, and in this case, you should only do it after profiling. Therefore,
you can afford the extra trouble it is to inline it with a few more
lexical changes.

Greets,

Asger

Re: [PATCH] low-hanging fruit

[Andre Poenitz]

 Are you sure that there is not a flag to gcc such that you can compile
 with -O, but without inlining?

-fno-inline   would sound appropriate, wouldn't it?

 [...]
 The code is often easier to understand when the definition is close to the
 declaration for these bits of code that are really simple.

I'd actually prefer to put one-liners directly in the class definition in
my own coding as a matter of convienience and better readability, too. Of
course, the latter is arguable, but if I see

   size_type size() const
 { return data_.size(); }

I immediately know what's going on and that it is a comparatively "cheap"
operation. And it's 30 key presses.

Current LyX: 

   size_type size() const;

   [... 50 lines later ...]

   MyFooBarClass::size_type MyFooBarClass::size() const
   {
 return data_.size()
   }

This does not only quadruple the amount of typing (which is a pain for the
wrists, I am _very_ aware of that) and occupies twice as much space on
screen, it is also takes more key presses for moving around.

And it gets even worse with templates... 

No, I am not advocating "my style",  I am pretty happy with current LyX as
long as it stays uniform. I am just trying to support Asgers words

" The code is often easier to understand when the definition is close to the
  declaration for these bits of code that are really simple. "

, in "real code" we prefer 'int i = 5;'  over 'int i; [...] i = 5;' too,
don't we?

Andre'

-- 
Andr Pnitz  [EMAIL PROTECTED]

[PATCH] low-hanging fruit

[John Levon]

Doing lyx -x lyx-quit big.lyx,  the attached patch
gives the following results (warm cache, error around 4%) :

user (s)| sys (s)   | elapsed (s)   |
--
28.33 (0.00%)   | 0.30 (0.00%)  | 29.61 (0.00%) | newinsetsold
24.29 (-14.27%) | 0.32 (6.61%)  | 25.92 (-12.48%)   | newinsetsnew

and the binary is only 14k bigger (with -g -O). This seems worth it to me.

The top of the profile looks like this with the patch applied :
 
LyXParagraph::InsertChar(int, char, LyXFont const &)[0x08146e5c]: 1.0553% (10233 
samples)
LyXParagraph::GetChar(int) const[0x08148210]: 1.1648% (11295 samples)
basic_string<...>::compare(char const *, unsigned int, unsigned int) 
const[0x08288a38]: 1.3048% (12652 samples)
LyXText::GetFont(Buffer const *, LyXParagraph *, int) const[0x08173358]: 1.4893% 
(14441 samples)
LyXFont::FontBits::eq(LyXFont::FontBits const &) const[0x081110f4]: 1.5215% (14753 
samples)
LyXFont::realize(LyXFont const &)[0x08111af0]: 1.5622% (15148 samples)
boost::shared_ptr * find_if(boost::shared_ptr *, 
boost::shared_ptr *, ShareContainer::isEqual, 
random_access_iterator_tag)[0x082d5dc0]: 1.6546% (16044 samples)
Buffer::parseSingleLyXformat2Token(LyXLex &, LyXParagraph *&, LyXParagraph *&, 
basic_string > const 
&, int &, char &, LyXFont &)[0x080a45d4]: 1.8889% (18316 samples)
__7LyXFont[0x0870]: 2.3485% (22772 samples)
LyXParagraph::GetFontSettings(BufferParams const &, int) const[0x0814750c]: 3.3957% 
(32927 samples)
LyXText::GetRow(LyXParagraph *, int, int &) const[0x08172b40]: 63.7895% (618541 
samples)


thanks
john

-- 
"24-hour boredom
I'm convicted instantly"
- Manic Street Preachers


Index: src/ChangeLog
===
RCS file: /usr/local/lyx/cvsroot/lyx-devel/src/ChangeLog,v
retrieving revision 1.95
diff -u -p -r1.95 ChangeLog
--- src/ChangeLog   2001/03/15 10:06:52 1.95
+++ src/ChangeLog   2001/03/15 11:53:15
@@ -1,3 +1,10 @@
+2001-03-15  John Levon  <[EMAIL PROTECTED]>
+
+   * lyxrow.h:
+   * lyxrow.C: inline some get() accessors. This
+   adds about 14k to the binary but speeds up a
+   large file load by about 10%.
+
 2001-03-14  Angus Leeming  <[EMAIL PROTECTED]>
 
* lyxfunc.C (Dispatch): removed redundant break statement.
Index: src/lyxrow.C
===
RCS file: /usr/local/lyx/cvsroot/lyx-devel/src/lyxrow.C,v
retrieving revision 1.4
diff -u -p -r1.4 lyxrow.C
--- src/lyxrow.C2000/07/24 13:53:17 1.4
+++ src/lyxrow.C2001/03/15 11:53:15
@@ -29,30 +29,12 @@ void Row::par(LyXParagraph * p)
 }
 
 
-LyXParagraph * Row::par()
-{
-   return par_;
-}
-
-
-LyXParagraph * Row::par() const
-{
-   return par_;
-}
-
-
 void Row::pos(LyXParagraph::size_type p)
 {
pos_ = p;
 }
 
 
-LyXParagraph::size_type Row::pos() const
-{
-   return pos_;
-}
-
-
 void Row::fill(int f)
 {
fill_ = f;
@@ -71,24 +53,12 @@ void Row::height(unsigned short h)
 }
 
 
-unsigned short Row::height() const
-{
-   return height_;
-}
-
-
 void Row::width(unsigned int w)
 {
width_ = w;
 }
 
 
-unsigned int Row::width() const
-{
-   return width_;
-}
-
-
 void Row::ascent_of_text(unsigned short a)
 {
ascent_of_text_ = a;
@@ -119,19 +89,7 @@ void Row::next(Row * r)
 }
 
 
-Row * Row::next() const
-{
-   return next_;
-}
-
-
 void Row::previous(Row * r)
 {
previous_ = r;
-}
-
-
-Row * Row::previous() const
-{
-   return previous_;
 }
Index: src/lyxrow.h
===
RCS file: /usr/local/lyx/cvsroot/lyx-devel/src/lyxrow.h,v
retrieving revision 1.10
diff -u -p -r1.10 lyxrow.h
--- src/lyxrow.h2000/07/24 13:53:17 1.10
+++ src/lyxrow.h2001/03/15 11:53:15
@@ -26,13 +26,13 @@ public:
///
void par(LyXParagraph * p);
///
-   LyXParagraph * par();
+   inline LyXParagraph * par();
///
-   LyXParagraph * par() const;
+   inline LyXParagraph * par() const;
///
void pos(LyXParagraph::size_type p);
///
-   LyXParagraph::size_type pos() const;
+   inline LyXParagraph::size_type pos() const;
///
void fill(int f);
///
@@ -40,11 +40,11 @@ public:
///
void height(unsigned short h);
///
-   unsigned short height() const;
+   inline unsigned short height() const;
///
void width(unsigned int w);
///
-   unsigned int width() const;
+   inline unsigned int width() const;
///
void ascent_of_text(unsigned short a);
///
@@ 

Re: [PATCH] low-hanging fruit

[Lars Gullik Bjønnes]

John Levon <[EMAIL PROTECTED]> writes:

| Doing lyx -x lyx-quit big.lyx,  the attached patch
| gives the following results (warm cache, error around 4%) :
| 
| user (s)| sys (s)   | elapsed (s)   |
| --
| 28.33 (0.00%)   | 0.30 (0.00%)  | 29.61 (0.00%) | newinsetsold
| 24.29 (-14.27%) | 0.32 (6.61%)  | 25.92 (-12.48%)   | newinsetsnew
| 
| and the binary is only 14k bigger (with -g -O). This seems worth it
| to me.

Measure size with 'size'.

Lgb

Re: [PATCH] low-hanging fruit

[John Levon]

On 15 Mar 2001, Lars Gullik Bjønnes wrote:

> John Levon <[EMAIL PROTECTED]> writes:
> 
> | Doing lyx -x lyx-quit big.lyx,  the attached patch
> | gives the following results (warm cache, error around 4%) :
> | 
> | user (s)| sys (s)   | elapsed (s)   |
> | --
> | 28.33 (0.00%)   | 0.30 (0.00%)  | 29.61 (0.00%) | newinsetsold
> | 24.29 (-14.27%) | 0.32 (6.61%)  | 25.92 (-12.48%)   | newinsetsnew
> | 
> | and the binary is only 14k bigger (with -g -O). This seems worth it
> | to me.
> 
> Measure size with 'size'.
> 
> Lgb

ugh, yes, sorry

hold on

john

-- 
"24-hour boredom
I'm convicted instantly"
- Manic Street Preachers

Re: [PATCH] low-hanging fruit

[Lars Gullik Bjønnes]

John Levon <[EMAIL PROTECTED]> writes:

| Doing lyx -x lyx-quit big.lyx,  the attached patch
| gives the following results (warm cache, error around 4%) :
| 
| user (s)| sys (s)   | elapsed (s)   |
| --
| 28.33 (0.00%)   | 0.30 (0.00%)  | 29.61 (0.00%) | newinsetsold
| 24.29 (-14.27%) | 0.32 (6.61%)  | 25.92 (-12.48%)   | newinsetsnew
| 
| and the binary is only 14k bigger (with -g -O). This seems worth it to me.

I don't want this patch yet.

If you have lines like:

do_something(with_this.size());

and widh_this.size() is inlined and do_something segfaults, the
segfault can look like it happened inside with_this.size(). 

Also when having inlines in the header file, alway put the inlined
method after the class definithin with an inline, in the class inline
should not be used:

class Foo {
publi:
int size();
};

inline
Foo::size() {
return ...;
}


As you can see with this it is _very_ easy to move the inlined method
from the header file to the C file (and back).

I have been thinking of a scheme where we can choose at compile time
if we want to have methods like this inlined or not.

foo.h:
class Foo {
public:
int size();
};

#ifdef INLINE_SIMPLE_METHODS
#define INLINE inline
#include "foo_inlines.h"
#endif

foo_inlines.h:
INLINE
Foo::size() {
return ...;
}

foo.C:
#ifndef INLINE_SIMPLE_METHODS
#define INLINE
#include "foo_inlines.h"
#endif


Or another scheme that gives the same effect.

Lgb

Re: [PATCH] low-hanging fruit

[John Levon]

On Thu, 15 Mar 2001, John Levon wrote:

> On 15 Mar 2001, Lars Gullik Bjønnes wrote:
> 
> > John Levon <[EMAIL PROTECTED]> writes:
> > 
> > | Doing lyx -x lyx-quit big.lyx,  the attached patch
> > | gives the following results (warm cache, error around 4%) :
> > | 
> > | user (s)| sys (s)   | elapsed (s)   |
> > | --
> > | 28.33 (0.00%)   | 0.30 (0.00%)  | 29.61 (0.00%) | newinsetsold
> > | 24.29 (-14.27%) | 0.32 (6.61%)  | 25.92 (-12.48%)   | newinsetsnew
> > | 
> > | and the binary is only 14k bigger (with -g -O). This seems worth it
> > | to me.
> > 
> > Measure size with 'size'.
> > 
> > Lgb

hum, now I'm really confused :

pg008:1050 size lyx
   textdata bss dec hex filename
2643515   19332   43100 2705947  294a1b lyx
pg008:1051 size lyxnew
   textdata bss dec hex filename
2638043   19332   43100 2700475  2934bb lyxnew
pg008:1053 expr 2638043 - 2643515
-5472

"lyxnew" is the one with the patch !!!

huh ??

john

-- 
"24-hour boredom
I'm convicted instantly"
- Manic Street Preachers

Re: [PATCH] low-hanging fruit

[John Levon]

On 15 Mar 2001, Lars Gullik Bjønnes wrote:

> John Levon <[EMAIL PROTECTED]> writes:
> 
> | Doing lyx -x lyx-quit big.lyx,  the attached patch
> | gives the following results (warm cache, error around 4%) :
> | 
> | user (s)| sys (s)   | elapsed (s)   |
> | --
> | 28.33 (0.00%)   | 0.30 (0.00%)  | 29.61 (0.00%) | newinsetsold
> | 24.29 (-14.27%) | 0.32 (6.61%)  | 25.92 (-12.48%)   | newinsetsnew
> | 
> | and the binary is only 14k bigger (with -g -O). This seems worth it to me.
> 
> I don't want this patch yet.
> 
> If you have lines like:
> 
> do_something(with_this.size());
> 
> and widh_this.size() is inlined and do_something segfaults, the
> segfault can look like it happened inside with_this.size(). 

and this is reason enough to not have this speedup ??

> Also when having inlines in the header file, alway put the inlined
> method after the class definithin with an inline, in the class inline
> should not be used:

ok, sorry

> #ifdef INLINE_SIMPLE_METHODS
> #define INLINE inline
> #include "foo_inlines.h"
> #endif
> 
> foo_inlines.h:
> INLINE
> Foo::size() {
> return ...;
> }
> 
> foo.C:
> #ifndef INLINE_SIMPLE_METHODS
> #define INLINE
> #include "foo_inlines.h"
> #endif

well as described, if not inlined this will have copies of Foo:size()

Also I don't think IMHO it makes sense to inline hardly-used accessor methods
(or maybe it does). I suppose it depends on architecture what kind of effect
this will have on .text size

thanks
john

-- 
"24-hour boredom
I'm convicted instantly"
- Manic Street Preachers

Re: [PATCH] low-hanging fruit

[Lars Gullik Bjønnes]

John Levon <[EMAIL PROTECTED]> writes:

| hum, now I'm really confused :
| 
| pg008:1050 size lyx
|textdata bss dec hex filename
| 2643515   19332   43100 2705947  294a1b lyx
| pg008:1051 size lyxnew
|textdata bss dec hex filename
| 2638043   19332   43100 2700475  2934bb lyxnew
| pg008:1053 expr 2638043 - 2643515
| -5472
| 
| "lyxnew" is the one with the patch !!!
| 
| huh ??

So it inlined nicely and the function call is completely removed and no
code generated for it.

Lgb

Re: [PATCH] low-hanging fruit

[John Levon]

On 15 Mar 2001, Lars Gullik Bjønnes wrote:

> John Levon <[EMAIL PROTECTED]> writes:
> 
> | hum, now I'm really confused :
> | 
> | pg008:1050 size lyx
> |textdata bss dec hex filename
> | 2643515   19332   43100 2705947  294a1b lyx
> | pg008:1051 size lyxnew
> |textdata bss dec hex filename
> | 2638043   19332   43100 2700475  2934bb lyxnew
> | pg008:1053 expr 2638043 - 2643515
> | -5472
> | 
> | "lyxnew" is the one with the patch !!!
> | 
> | huh ??
> 
> So it inlined nicely and the function call is completely removed and no
> code generated for it.
> 
> Lgb

Yes I realised after I sent it it made sense. I'm not sure why "ls" at home
claimed it was larger ... (here ls has the same difference too)

I really do think the speedup is significant enough to live with the debugging 
problem...
(and it will also apply to normal use of lyx rather than just loading as far as I can 
see ?)

thanks
john

-- 
"24-hour boredom
I'm convicted instantly"
- Manic Street Preachers

Re: [PATCH] low-hanging fruit

[Lars Gullik Bjønnes]

John Levon <[EMAIL PROTECTED]> writes:

| > and widh_this.size() is inlined and do_something segfaults, the
| > segfault can look like it happened inside with_this.size(). 
| 
| and this is reason enough to not have this speedup ??

Yes, I belive a more detrministic development environment is reason
enough.

And as said, I am thinking about schemes to inline importent small
methods again, but not just now.
 
| > Also when having inlines in the header file, alway put the inlined
| > method after the class definithin with an inline, in the class inline
| > should not be used:
| 
| ok, sorry
| 
| > #ifdef INLINE_SIMPLE_METHODS
| > #define INLINE inline
| > #include "foo_inlines.h"
| > #endif
| > 
| > foo_inlines.h:
| > INLINE
| > Foo::size() {
| > return ...;
| > }
| > 
| > foo.C:
| > #ifndef INLINE_SIMPLE_METHODS
| > #define INLINE
| > #include "foo_inlines.h"
| > #endif
| 
| well as described, if not inlined this will have copies of Foo:size()

??? yes, that was the purpose...
 
| Also I don't think IMHO it makes sense to inline hardly-used accessor methods
| (or maybe it does). I suppose it depends on architecture what kind of effect
| this will have on .text size

One-liner getter methods will almost alway inline perfectly and
generate less code than a "full" method. So I belive that in most
cases getters should be made inline, most of the setters should be
inline (if very simple). Methods that are only wrappers around other
methods can be inlined.

Lgb

Re: [PATCH] low-hanging fruit

[John Levon]

On 15 Mar 2001, Lars Gullik Bjønnes wrote:

> John Levon <[EMAIL PROTECTED]> writes:
> 
> | > and widh_this.size() is inlined and do_something segfaults, the
> | > segfault can look like it happened inside with_this.size(). 
> | 
> | and this is reason enough to not have this speedup ??
> 
> Yes, I belive a more detrministic development environment is reason
> enough.

your call

> | well as described, if not inlined this will have copies of Foo:size()
> 
> ??? yes, that was the purpose...

I mean there will be one in foo.C AND non-inlined copies in anything including foo.h. 
Or am I talking crap again

john

-- 
"24-hour boredom
I'm convicted instantly"
- Manic Street Preachers

Re: [PATCH] low-hanging fruit

[Lars Gullik Bjønnes]

John Levon <[EMAIL PROTECTED]> writes:

| I mean there will be one in foo.C AND non-inlined copies in anything including 
|foo.h. 
| Or am I talking crap again

foo_inlines.h should only be included in foo.h if
INLINE_SIMPLE_METHODS is defined.

Lgb

Re: [PATCH] low-hanging fruit

[John Levon]

On 15 Mar 2001, Lars Gullik Bjønnes wrote:

> foo_inlines.h should only be included in foo.h if
> INLINE_SIMPLE_METHODS is defined.
> 
> Lgb

*sigh*

I really do have problems with reading :P

john 

-- 
"24-hour boredom
I'm convicted instantly"
- Manic Street Preachers

Re: [PATCH] low-hanging fruit

[Juergen Vigna]

On 15-Mar-2001 John Levon wrote:

> I really do think the speedup is significant enough to live with the debugging
> problem...

What debugging problem? I compile with only -g option and then inlined code
is no problem on debugging!

   Jürgen

--
-._-._-._-._-._-._-._-._-._-._-._-._-._-._-._-._-._-._-._-._-._-._-._
Dr. Jürgen VignaE-Mail:  [EMAIL PROTECTED]
Italienallee 13/N   Tel/Fax: +39-0471-450260 / +39-0471-450253
I-39100 Bozen   Web: http://www.sad.it/~jug
-._-._-._-._-._-._-._-._-._-._-._-._-._-._-._-._-._-._-._-._-._-._-._

Reality is just a convenient measure of complexity.
-- Alvy Ray Smith

Re: [PATCH] low-hanging fruit

[Lars Gullik Bjønnes]

Juergen Vigna <[EMAIL PROTECTED]> writes:

| On 15-Mar-2001 John Levon wrote:
| 
| > I really do think the speedup is significant enough to live with the debugging
| > problem...
| 
| What debugging problem? I compile with only -g option and then inlined code
| is no problem on debugging!

Well I consider compiling without -O an even bigger problem. Then a lot
of small problems will not be noticed until you compile with
optimization turned on.

Lgb

Re: [PATCH] low-hanging fruit

[Asger K. Alstrup Nielsen]

On 15 Mar 2001, Lars Gullik Bjønnes wrote:

[How to avoid inlining to improve debugging]
> Well I consider compiling without -O an even bigger problem. Then a lot
> of small problems will not be noticed until you compile with
> optimization turned on.

Are you sure that there is not a flag to gcc such that you can compile
with -O, but without inlining?

I don't think it's a good trade-off to clutter all inlinings with
several lines of conditional preprocessor voodoo, just to avoid
a largely hypothetical problem!  Notice that most methods that
you inline are fairly trivial, and it's, in my experience, extremely
rare that you want to single-step through those.

I think this problem should be solved via a compiler switch - not
by cluttering the code.  If the compiler can't do this, fix the compiler
rather than cluttering the code.

Also, I don't think it's not a good trade-off to allow easy moving of
inline methods back and forth between the cpp/cxx/c++ files and the header
files by using the inline keyword.
The code is often easier to understand when the definition is close to the
declaration for these bits of code that are really simple.

It's not important that changing whether a method is inline or not should
be an easy operation. You only do this operation when you optimize the
code, and in this case, you should only do it after profiling. Therefore,
you can afford the extra trouble it is to inline it with a few more
lexical changes.

Greets,

Asger

Re: [PATCH] low-hanging fruit

[Andre Poenitz]

> Are you sure that there is not a flag to gcc such that you can compile
> with -O, but without inlining?

-fno-inline   would sound appropriate, wouldn't it?

> [...]
> The code is often easier to understand when the definition is close to the
> declaration for these bits of code that are really simple.

I'd actually prefer to put one-liners directly in the class definition in
my own coding as a matter of convienience and better readability, too. Of
course, the latter is arguable, but if I see

   size_type size() const
 { return data_.size(); }

I immediately know what's going on and that it is a comparatively "cheap"
operation. And it's 30 key presses.

Current LyX: 

   size_type size() const;

   [... 50 lines later ...]

   MyFooBarClass::size_type MyFooBarClass::size() const
   {
 return data_.size()
   }

This does not only quadruple the amount of typing (which is a pain for the
wrists, I am _very_ aware of that) and occupies twice as much space on
screen, it is also takes more key presses for moving around.

And it gets even worse with templates... 

No, I am not advocating "my style",  I am pretty happy with current LyX as
long as it stays uniform. I am just trying to support Asgers words

" The code is often easier to understand when the definition is close to the
  declaration for these bits of code that are really simple. "

, in "real code" we prefer 'int i = 5;'  over 'int i; [...] i = 5;' too,
don't we?

Andre'

-- 
André Pönitz  [EMAIL PROTECTED]