Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1) - LyXscrollpatch20130427.diff (1/1)

[Jean-Marc Lasgouttes]

Le 09/05/2013 09:43, pdv a écrit :

Yes, I'm available.
I've posted another version as you might already have noticed.


Unfortunately it seems that we will not have time to look at it 
seriously :( This stuff is complicated to get right...


JMarc

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1) - LyXscrollpatch20130427.diff (1/1)

[Jean-Marc Lasgouttes]

Le 09/05/2013 09:43, pdv a écrit :

Yes, I'm available.
I've posted another version as you might already have noticed.


Unfortunately it seems that we will not have time to look at it 
seriously :( This stuff is complicated to get right...


JMarc

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1) - LyXscrollpatch20130427.diff (1/1) - LyXscrollpatch20130509.diff (1/1)

[pdv]

In article 517ee12b.4040...@lyx.org,
 Jean-Marc Lasgouttes lasgout...@lyx.org wrote:

 Le 28/04/2013 13:06, pdv a écrit :
  OK, here is a new version. Let me know if you experience anymore
  problems.
  For the time being I've left the clean-up step of the map as it was,
  although I realize it's of limited value; when entering the same word
  multiple times, the partial words get included anyway;
 
  The map itself is still defined in the BufferView class, but as a shared
  static map. The reason for this is that when the user makes changes to
  the screen fonts, the map becomes useless and I think the best option is
  to clear it at that point.
  I've added the case LFUN_SCREEN_FONT_UPDATE: to BufferView::dispatch()
  and added a corresponding request in GuiApplication::dispatch();
 
 You should move the code that computes text width to 
 GuiFontMetrics::width(docstring). This is the right place for storing a 
 map, that could be static if you want to keep your current solution or 
 just a member of the GuiFontMetrics object so that you do not have to 
 play tricks with font attributes.
 
 Moreover, if your mùap is in GuiFontMetrics it will be reset 
 automatically by the code in GuiApplication.cpp.
 
 This will be much easier in my opinion.
 
  I noticed then that the map was cleared and rebuild twice ...
  ... because in GuiPreferences::dispatchParams() there is first a call
  dispatch(FuncRequest(LFUN_LYXRC_APPLY, ss.str()))
  later followed by
  dispatch(FuncRequest(LFUN_SCREEN_FONT_UPDATE))
  but the first dispatch runs GuiReset() which also sends a
  LFUN_SCREEN_FONT_UPDATE.
  Therefore I've commented out the 2nd explicit LFUN_SCREEN_FONT_UPDATE
  request.
 
 Not sure about that, but things should become clearer once code is at 
 the right place.
 
 JMarc

I've moved the code to GuiFontMetrics which is indeed much better.
Solved also a remaining issue with smallcaps.

begin 644 LyXscrollpatch20130509.diff
M1G)O;2`W9#)C.3(W,S`W,CW93DX,#(T96,X,C0X83X8F4U,#,Q,5F,68V
M($UO;B!397`@,3@,#`Z,#`Z,#`@,C`P,0IF]M.B!0871R:6-K($1E(%9I
MW-C:5R92`\1V:7-S8VAEF5`961P;F5T+F)E/@I$871E.B!7960L(#$@
M36%Y(#(P,3,@,C(Z-38Z,3$@*S`R,#`*4W5B:F5C=#H@6U!!5$-(72!-;W9E
M9!T:4@=5X=%=I9'1H*D@9G)O;2!497AT365TFECR!T;R!'=6E;VYT
M365TFECRP*(')E;F%M960@:70@86YD(9I5D('-M86QL8V%PRX*BTM
M+0H@W)C+T)U9F9EE9I97N8W!P(`@(`@(`@(`@(`@(`@('P@(`T
M(LMB!SF,O0G5F9F5R5FEE=RYH(`@(`@(`@(`@(`@(`@(`@?`@
M(#,@*RT*('-R8R]497AT365TFECRYC'`@(`@(`@(`@(`@(`@(!\
M(#(Y,R`K*RLK*RLK*RLK*RLK*RLK*RLK*RLK*RLK*RLK*RLK*RTM+0H@W)C
M+U1E'1-971R:6-S+F@@(`@(`@(`@(`@(`@(`@('P@(#$X(LK+0H@
MW)C+V9R;VYT96YDR];VYT365TFECRYH(`@(`@(`@('P@(`R(L*
M('-R8R]FF]N=5N9',O70T+T=U:49O;G1-971R:6-S+F-P!\(`T.`K
M*RLK*RL*('-R8R]FF]N=5N9',O70T+T=U:49O;G1-971R:6-S+F@@(!\
M(`@-B`KB!SF,O9G)O;G1E;F1S+W%T-]'=6E086EN=5R+F-P`@(`@
M?`@(#(@*PH@W)C+V9R;VYT96YDR]Q=#0O1W5I4')E9G,N8W!P(`@(`@
M('P@(`S(LMB!SF,OF]W%I;G1EBYC'`@(`@(`@(`@(`@(`@
M(`@?`@,S@*RLK*RL*(#$P(9I;5S(-H86YG960L(#,X-2!I;G-EG1I
M;VYS*LI+`S,2!D96QE=EO;G,H+2D*F1I9F8@+2UG:70@82]SF,O0G5F
M9F5R5FEE=RYC'`@8B]SF,O0G5F9F5R5FEE=RYC'`*:6YD97@@9%A,#5A
M-BXN-S4Y-S(V.2`Q,#`V-#0*+2TM($OW)C+T)U9F9EE9I97N8W!PBLK
M*R!B+W-R8R]=69F97)6:65W+F-P`I`0`M.38U+#@*SDV-2PW($!`('9O
M:60@0G5F9F5R5FEE=SHZ=7!D871E1]C=6UE;G1#;%SRA$;V-U;65N=$-L
M87-S0V]NW10='(@;VQD9,IB`*(`EB=69F97)?+F5RF]RR@B0VQAW,@
M4W=I=-H(BD[B!]BT**PD*(`H@+RHJ(%)E='5R;B!T:4@8VAA;F=E('-T
M871UR!A=!C=7)S;W(@]S:71I;VXL('1A:VEN9R!I;B!A8V-O=6YT('1H
M90H@(H@W1A='5S(%T(5A8V@@;5V96P@;V8@=AE(1O8W5M96YT(ET
M97)A=]R(AA('1A8FQE(EN($@95L971E9`I`0`M,38P,PW(LQ-C`P
M+#@0$`@=F]I9!=69F97)6:65W.CID:7-P871C:A=6YC4F5Q=65S=!C
M;VYS=`F(-M9P@1ES%T8VA297-U;'0@)B!DBD*(`EC87-E($Q54Y?
M4T-2145.7U)%0T5.5$52.@H@0ER96-E;G1Eb...@i.ph@0EBF5A:SL*+0HK
M0D)B`)8V%S92!,1E5.7T))0E1%6%]$051!0D%315]!1$0Z('L*(`D)0W5R
MV]R('1M-UB`](-UCL*(`D)9FEN9$ENV5T*'1M-UBP@0DE5$58
M7T-/1$4L(9A;'-E*3L*9EF9B`M+6=I=!A+W-R8R]=69F97)6:65W+F@@
M8B]SF,O0G5F9F5R5FEE=RYHFEN95X(,W9F(T8S$N+C$X9#-A,6,@,3`P
M-C0TBTM+2!A+W-R8R]=69F97)6:65W+F@**RLK((OW)C+T)U9F9EE9I
M97N:`I`0`M,S(P+#@*S,R,PW($!`('!U8FQI8SH*(`EB;V]L(-L:6-K
M86)L94ENV5T*D@8V]NW0[B`)+R\OB`)=F]I9!M86ME1]C=6UE;G1#
M;%Sr...@i.phmBL)B!PFEV871E.@H@2\O+R!N;VYC;W!Y86)L90H@4)U
M9F9EE9I97H0G5F9F5R5FEE=R!C;VYS=`F*3L*0$`@+3,V,2PV(LS-C$L
M-R!`0!PFEV871E.@H@B`)W1R=6-T(%!R:79A=4[B`)4')I=F%T92`J
M(-O;G-T(0[BL)B!].PH@B`O+R\@V]M92!S%C92!F;W(@9')A=VEN
M9R!T:4@)VYEW1E9@;6%R:V5RR`H:6X@EX96PIF1I9F8@+2UG:70@
M82]SF,O55X=$UE=')I8W,N8W!P((OW)C+U1E'1-971R:6-S+F-P`II
M;F1E`T93P9,P+BYD-S0Y-V(Y(#$P,#8T-`HM+2T@82]SF,O55X=$UE
M=')I8W,N8W!PBLK*R!B+W-R8R]497AT365TFECRYC'`*0$`@+30Y+#$P
M(LT.2PQ-B!`0`H@(VEN8VQU94@(F9R;VYT96YDR];VYT365TFECRYH
M(@H@(VEN8VQU94@(F9R;VYT96YDR]086EN=5R+F@BB`**R-I;F-L=61E
M()FF]N=5N9',O70T+T=U:49O;G1,;V%D97(N:(**PH@(VEN8VQU94@
M(G-U'!OG0O95B=6N:(*(-I;F-L=61E()S=7!P;W)T+V1O8W-TFEN
M9U]L:7-T+F@BB`C:6YC;'5D92`BW5P]R=]G971T97AT+F@BB`C:6YC
M;'5D92`BW5P]R=]L87-S97)T+F@BBLC:6YC;'5D92`BW5P]R=]Q
MW1R:6YG7VAE;'!EG,N:(**R-I;F-L=61E()S=7!P;W)T+W1E'1U=EL
MRYH(@HKBLC:6YC;'5D92`\471'=6DO449O;G1-971R:6-S/@H@B`C:6YC

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1) - LyXscrollpatch20130427.diff (1/1)

[pdv]

In article 5186ae01.4080...@lyx.org,
 Jean-Marc Lasgouttes lasgout...@lyx.org wrote:

 Le 05/05/13 20:36, pdv a écrit :
  What exactly takes 17.4s?
 
  I suppose that's the time taken by GuiPainter::text(), but I don't know
  enough of the Instruments app and it's modules to give any more details.
 
  I've now monitored both functions with pmprof:
  ( I scroll through a document of mine starting from the top down to the
  same location with the down arrow key (line per line not page per page) )
 
 A related question: will you be somewhat available during the coming 
 week? I'd like at the developers meeting (from Thursday to Sunday) to 
 start from your patch and apply it to master branch. We will probably 
 diverge significantly from what you propose but nevertheless it would be 
 useful to know whether there are times when you are available. The plan 
 is probably to set up a branch on git to work on.
 
 JMarc

Yes, I'm available.
I've posted another version as you might already have noticed.

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1) - LyXscrollpatch20130427.diff (1/1) - LyXscrollpatch20130509.diff (1/1)

[Jean-Marc Lasgouttes]

Le 09/05/2013 09:37, pdv a écrit :

I've moved the code to GuiFontMetrics which is indeed much better.
Solved also a remaining issue with smallcaps.


Very good. Thanks.

JMarc

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1) - LyXscrollpatch20130427.diff (1/1) - LyXscrollpatch20130509.diff (1/1)

[pdv]

In article <517ee12b.4040...@lyx.org>,
 Jean-Marc Lasgouttes  wrote:

> Le 28/04/2013 13:06, pdv a écrit :
> > OK, here is a new version. Let me know if you experience anymore
> > problems.
> > For the time being I've left the clean-up step of the map as it was,
> > although I realize it's of limited value; when entering the same word
> > multiple times, the partial words get included anyway;
> >
> > The map itself is still defined in the BufferView class, but as a shared
> > static map. The reason for this is that when the user makes changes to
> > the screen fonts, the map becomes useless and I think the best option is
> > to clear it at that point.
> > I've added the case LFUN_SCREEN_FONT_UPDATE: to BufferView::dispatch()
> > and added a corresponding request in GuiApplication::dispatch();
> 
> You should move the code that computes text width to 
> GuiFontMetrics::width(docstring). This is the right place for storing a 
> map, that could be static if you want to keep your current solution or 
> just a member of the GuiFontMetrics object so that you do not have to 
> play tricks with font attributes.
> 
> Moreover, if your mùap is in GuiFontMetrics it will be reset 
> automatically by the code in GuiApplication.cpp.
> 
> This will be much easier in my opinion.
> 
> > I noticed then that the map was cleared and rebuild twice ...
> > ... because in GuiPreferences::dispatchParams() there is first a call
> > dispatch(FuncRequest(LFUN_LYXRC_APPLY, ss.str()))
> > later followed by
> > dispatch(FuncRequest(LFUN_SCREEN_FONT_UPDATE))
> > but the first dispatch runs GuiReset() which also sends a
> > LFUN_SCREEN_FONT_UPDATE.
> > Therefore I've commented out the 2nd explicit LFUN_SCREEN_FONT_UPDATE
> > request.
> 
> Not sure about that, but things should become clearer once code is at 
> the right place.
> 
> JMarc

I've moved the code to GuiFontMetrics which is indeed much better.
Solved also a remaining issue with smallcaps.

begin 644 LyXscrollpatch20130509.diff
M1G)O;2`W9#)C.3(W,S`W,C&5D('-M86QL8V%P'1-971R:6-S+F@@("`@("`@("`@("`@("`@("`@('P@(#$X("LK+0H@
M"`T93

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1) - LyXscrollpatch20130427.diff (1/1)

[pdv]

In article <5186ae01.4080...@lyx.org>,
 Jean-Marc Lasgouttes  wrote:

> Le 05/05/13 20:36, pdv a écrit :
> >> What exactly takes 17.4s?
> >
> > I suppose that's the time taken by GuiPainter::text(), but I don't know
> > enough of the Instruments app and it's modules to give any more details.
> >
> > I've now monitored both functions with pmprof:
> > ( I scroll through a document of mine starting from the top down to the
> > same location with the down arrow key (line per line not page per page) )
> 
> A related question: will you be somewhat available during the coming 
> week? I'd like at the developers meeting (from Thursday to Sunday) to 
> start from your patch and apply it to master branch. We will probably 
> diverge significantly from what you propose but nevertheless it would be 
> useful to know whether there are times when you are available. The plan 
> is probably to set up a branch on git to work on.
> 
> JMarc

Yes, I'm available.
I've posted another version as you might already have noticed.

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1) - LyXscrollpatch20130427.diff (1/1) - LyXscrollpatch20130509.diff (1/1)

[Jean-Marc Lasgouttes]

Le 09/05/2013 09:37, pdv a écrit :

I've moved the code to GuiFontMetrics which is indeed much better.
Solved also a remaining issue with smallcaps.


Very good. Thanks.

JMarc

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1) - LyXscrollpatch20130427.diff (1/1)

[pdv]

In article 518166ab.5070...@lyx.org,
 Jean-Marc Lasgouttes lasgout...@lyx.org wrote:

 Le 01/05/2013 20:55, pdv a écrit :
  There are 2 occurences of calculate_qt_char_width, in TextMetrics and in
  RowPainter.
 
 Yes, I toggled both.
 
  and there is at least one change which is not enclosed by these
  conditionals: In TextMetrics I changed rowBreakPoint() to return the
  breakpoint as well as the width, so that the subsequent call to
  rowWidth() is not needed anymore.
 
  I'm still many  revisions behind master;
  I'll catch-up and see what I obtain with pmprof.
 
 It would be surprising that playing catch-up is enough.
 
  I've done some timing with Instruments and a testdocument;
  with calculate_qt_char_width=0 GuiPainter::text takes 17.4s
  with calculate_qt_char_width=1 GuiPainter::text takes 4.5s
 
 What exactly takes 17.4s?

I suppose that's the time taken by GuiPainter::text(), but I don't know 
enough of the Instruments app and it's modules to give any more details.

I've now monitored both functions with pmprof:
( I scroll through a document of mine starting from the top down to the 
same location with the down arrow key (line per line not page per page) )


calculate_qt_char_widths = 0
#pmprof# text: 0.04msec, count=1044047, total=45155.01msec

calculate_qt_char_widths = 1
#pmprof# text: 0.05msec, count=427662, total=21348.83msec
#pmprof# textmetrics_textwidth: 0.00msec, count=285813, total=563.34msec

text is from GuiPainter::text()
in both cases I inserted the macro at the start of the function.
Qualitatively this agrees with the Instruments timings, although the 
gain in text() is smaller and the time in TextMetrics::textWidth() is 
larger but still negligible compared to the time in GuiPainter::text().


 
  the time taken by TextMetrics::textMetrics is then 137ms
 
  Was this scrolling problem not an OS X issue?
 
 Yes, but I do not have OS X at hand. It is nevertheless important to 
 test all platforms.
 
 JMarc

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1) - LyXscrollpatch20130427.diff (1/1)

[Jean-Marc Lasgouttes]

Le 05/05/13 20:36, pdv a écrit :

What exactly takes 17.4s?


I suppose that's the time taken by GuiPainter::text(), but I don't know
enough of the Instruments app and it's modules to give any more details.

I've now monitored both functions with pmprof:
( I scroll through a document of mine starting from the top down to the
same location with the down arrow key (line per line not page per page) )


A related question: will you be somewhat available during the coming 
week? I'd like at the developers meeting (from Thursday to Sunday) to 
start from your patch and apply it to master branch. We will probably 
diverge significantly from what you propose but nevertheless it would be 
useful to know whether there are times when you are available. The plan 
is probably to set up a branch on git to work on.


JMarc

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1) - LyXscrollpatch20130427.diff (1/1)

[pdv]

In article <518166ab.5070...@lyx.org>,
 Jean-Marc Lasgouttes  wrote:

> Le 01/05/2013 20:55, pdv a écrit :
> > There are 2 occurences of calculate_qt_char_width, in TextMetrics and in
> > RowPainter.
> 
> Yes, I toggled both.
> 
> > and there is at least one change which is not enclosed by these
> > conditionals: In TextMetrics I changed rowBreakPoint() to return the
> > breakpoint as well as the width, so that the subsequent call to
> > rowWidth() is not needed anymore.
> >
> > I'm still many  revisions behind master;
> > I'll catch-up and see what I obtain with pmprof.
> 
> It would be surprising that playing catch-up is enough.
> 
> > I've done some timing with Instruments and a testdocument;
> > with calculate_qt_char_width=0 GuiPainter::text takes 17.4s
> > with calculate_qt_char_width=1 GuiPainter::text takes 4.5s
> 
> What exactly takes 17.4s?

I suppose that's the time taken by GuiPainter::text(), but I don't know 
enough of the Instruments app and it's modules to give any more details.

I've now monitored both functions with pmprof:
( I scroll through a document of mine starting from the top down to the 
same location with the down arrow key (line per line not page per page) )


calculate_qt_char_widths = 0
#pmprof# text: 0.04msec, count=1044047, total=45155.01msec

calculate_qt_char_widths = 1
#pmprof# text: 0.05msec, count=427662, total=21348.83msec
#pmprof# textmetrics_textwidth: 0.00msec, count=285813, total=563.34msec

"text" is from GuiPainter::text()
in both cases I inserted the macro at the start of the function.
Qualitatively this agrees with the Instruments timings, although the 
gain in text() is smaller and the time in TextMetrics::textWidth() is 
larger but still negligible compared to the time in GuiPainter::text().


> 
> > the time taken by TextMetrics::textMetrics is then 137ms
> >
> > Was this scrolling problem not an "OS X" issue?
> 
> Yes, but I do not have OS X at hand. It is nevertheless important to 
> test all platforms.
> 
> JMarc

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1) - LyXscrollpatch20130427.diff (1/1)

[Jean-Marc Lasgouttes]

Le 05/05/13 20:36, pdv a écrit :

What exactly takes 17.4s?


I suppose that's the time taken by GuiPainter::text(), but I don't know
enough of the Instruments app and it's modules to give any more details.

I've now monitored both functions with pmprof:
( I scroll through a document of mine starting from the top down to the
same location with the down arrow key (line per line not page per page) )


A related question: will you be somewhat available during the coming 
week? I'd like at the developers meeting (from Thursday to Sunday) to 
start from your patch and apply it to master branch. We will probably 
diverge significantly from what you propose but nevertheless it would be 
useful to know whether there are times when you are available. The plan 
is probably to set up a branch on git to work on.


JMarc

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1) - LyXscrollpatch20130427.diff (1/1)

[pdv]

In article 517fe428.8070...@lyx.org,
 Jean-Marc Lasgouttes lasgout...@lyx.org wrote:

 Le 28/04/2013 13:06, pdv a écrit :
  OK, here is a new version. Let me know if you experience anymore
  problems.
  For the time being I've left the clean-up step of the map as it was,
  although I realize it's of limited value; when entering the same word
  multiple times, the partial words get included anyway;
 
 For now I only tried performance, and did not notice any problem. With 
 the attached patch I get the following numbers.
 
 The profiler only computes the time needed to recomputes the metrics. I 
 do not know what other parts should be intrumented, probably around 
 rowpainter.
 
 Experiment is to load the user's guide, go to top and press PageDown 40 
 times. The numbers are in milliseconds, 3 runs for each case.
 
 My numbers are quite noisy actually, but it seems that the cost of 
 calculate_qt_char_width is non-negligible. OTOH, I do not understand why 
 calculate_qt_char_widths=0 does not give the same numbers as master? 
 Didn't you say that it should be equivalent?

There are 2 occurences of calculate_qt_char_width, in TextMetrics and in 
RowPainter.
and there is at least one change which is not enclosed by these 
conditionals: In TextMetrics I changed rowBreakPoint() to return the 
breakpoint as well as the width, so that the subsequent call to 
rowWidth() is not needed anymore.

I'm still many  revisions behind master;
I'll catch-up and see what I obtain with pmprof.

I've done some timing with Instruments and a testdocument;
with calculate_qt_char_width=0 GuiPainter::text takes 17.4s
with calculate_qt_char_width=1 GuiPainter::text takes 4.5s
the time taken by TextMetrics::textMetrics is then 137ms

Was this scrolling problem not an OS X issue?

 
 JMarc
 
 
 Your patch with #define calculate_qt_char_widths 1
 
 #pmprof# metrics: 4.43msec, count=49, total=216.90msec
 
 #pmprof# metrics: 4.23msec, count=49, total=207.36msec
 
 #pmprof# metrics: 4.49msec, count=49, total=219.83msec
 
 
 Your patch with #define calculate_qt_char_widths 0
 
 #pmprof# metrics: 3.05msec, count=49, total=149.61msec
 
 #pmprof# metrics: 2.81msec, count=50, total=140.57msec
 
 #pmprof# metrics: 2.74msec, count=50, total=136.93msec
 
 
 Current master:
 
 #pmprof# metrics: 3.18msec, count=50, total=158.78msec
 
 #pmprof# metrics: 3.95msec, count=50, total=197.63msec
 
 #pmprof# metrics: 3.97msec, count=50, total=198.61msec
 
 
 -
 diff --git a/src/BufferView.cpp b/src/BufferView.cpp
 index 10b5263..4af1b14 100644
 --- a/src/BufferView.cpp
 +++ b/src/BufferView.cpp
 @@ -78,6 +78,7 @@
  #include support/lstrings.h
  #include support/Package.h
  #include support/types.h
 +#include support/pmprof.h
  
  #include cerrno
  #include fstream
 @@ -2574,6 +2575,7 @@ bool BufferView::singleParUpdate()
  
  void BufferView::updateMetrics()
  {
 + PROFILE_THIS_BLOCK(metrics);
   if (height_ == 0 || width_ == 0)
   return;

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1) - LyXscrollpatch20130427.diff (1/1)

[Jean-Marc Lasgouttes]

Le 01/05/2013 20:55, pdv a écrit :

There are 2 occurences of calculate_qt_char_width, in TextMetrics and in
RowPainter.


Yes, I toggled both.


and there is at least one change which is not enclosed by these
conditionals: In TextMetrics I changed rowBreakPoint() to return the
breakpoint as well as the width, so that the subsequent call to
rowWidth() is not needed anymore.

I'm still many  revisions behind master;
I'll catch-up and see what I obtain with pmprof.


It would be surprising that playing catch-up is enough.


I've done some timing with Instruments and a testdocument;
with calculate_qt_char_width=0 GuiPainter::text takes 17.4s
with calculate_qt_char_width=1 GuiPainter::text takes 4.5s


What exactly takes 17.4s?


the time taken by TextMetrics::textMetrics is then 137ms

Was this scrolling problem not an OS X issue?


Yes, but I do not have OS X at hand. It is nevertheless important to 
test all platforms.


JMarc

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1) - LyXscrollpatch20130427.diff (1/1)

[pdv]

In article <517fe428.8070...@lyx.org>,
 Jean-Marc Lasgouttes  wrote:

> Le 28/04/2013 13:06, pdv a écrit :
> > OK, here is a new version. Let me know if you experience anymore
> > problems.
> > For the time being I've left the clean-up step of the map as it was,
> > although I realize it's of limited value; when entering the same word
> > multiple times, the partial words get included anyway;
> 
> For now I only tried performance, and did not notice any problem. With 
> the attached patch I get the following numbers.
> 
> The profiler only computes the time needed to recomputes the metrics. I 
> do not know what other parts should be intrumented, probably around 
> rowpainter.
> 
> Experiment is to load the user's guide, go to top and press PageDown 40 
> times. The numbers are in milliseconds, 3 runs for each case.
> 
> My numbers are quite noisy actually, but it seems that the cost of 
> calculate_qt_char_width is non-negligible. OTOH, I do not understand why 
> calculate_qt_char_widths=0 does not give the same numbers as master? 
> Didn't you say that it should be equivalent?

There are 2 occurences of calculate_qt_char_width, in TextMetrics and in 
RowPainter.
and there is at least one change which is not enclosed by these 
conditionals: In TextMetrics I changed rowBreakPoint() to return the 
breakpoint as well as the width, so that the subsequent call to 
rowWidth() is not needed anymore.

I'm still many  revisions behind master;
I'll catch-up and see what I obtain with pmprof.

I've done some timing with Instruments and a testdocument;
with calculate_qt_char_width=0 GuiPainter::text takes 17.4s
with calculate_qt_char_width=1 GuiPainter::text takes 4.5s
the time taken by TextMetrics::textMetrics is then 137ms

Was this scrolling problem not an "OS X" issue?

> 
> JMarc
> 
> 
> Your patch with #define calculate_qt_char_widths 1
> 
> #pmprof# metrics: 4.43msec, count=49, total=216.90msec
> 
> #pmprof# metrics: 4.23msec, count=49, total=207.36msec
> 
> #pmprof# metrics: 4.49msec, count=49, total=219.83msec
> 
> 
> Your patch with #define calculate_qt_char_widths 0
> 
> #pmprof# metrics: 3.05msec, count=49, total=149.61msec
> 
> #pmprof# metrics: 2.81msec, count=50, total=140.57msec
> 
> #pmprof# metrics: 2.74msec, count=50, total=136.93msec
> 
> 
> Current master:
> 
> #pmprof# metrics: 3.18msec, count=50, total=158.78msec
> 
> #pmprof# metrics: 3.95msec, count=50, total=197.63msec
> 
> #pmprof# metrics: 3.97msec, count=50, total=198.61msec
> 
> 
> -
> diff --git a/src/BufferView.cpp b/src/BufferView.cpp
> index 10b5263..4af1b14 100644
> --- a/src/BufferView.cpp
> +++ b/src/BufferView.cpp
> @@ -78,6 +78,7 @@
>  #include "support/lstrings.h"
>  #include "support/Package.h"
>  #include "support/types.h"
> +#include "support/pmprof.h"
>  
>  #include 
>  #include 
> @@ -2574,6 +2575,7 @@ bool BufferView::singleParUpdate()
>  
>  void BufferView::updateMetrics()
>  {
> + PROFILE_THIS_BLOCK(metrics);
>   if (height_ == 0 || width_ == 0)
>   return;
>

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1) - LyXscrollpatch20130427.diff (1/1)

[Jean-Marc Lasgouttes]

Le 01/05/2013 20:55, pdv a écrit :

There are 2 occurences of calculate_qt_char_width, in TextMetrics and in
RowPainter.


Yes, I toggled both.


and there is at least one change which is not enclosed by these
conditionals: In TextMetrics I changed rowBreakPoint() to return the
breakpoint as well as the width, so that the subsequent call to
rowWidth() is not needed anymore.

I'm still many  revisions behind master;
I'll catch-up and see what I obtain with pmprof.


It would be surprising that playing catch-up is enough.


I've done some timing with Instruments and a testdocument;
with calculate_qt_char_width=0 GuiPainter::text takes 17.4s
with calculate_qt_char_width=1 GuiPainter::text takes 4.5s


What exactly takes 17.4s?


the time taken by TextMetrics::textMetrics is then 137ms

Was this scrolling problem not an "OS X" issue?


Yes, but I do not have OS X at hand. It is nevertheless important to 
test all platforms.


JMarc

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1) - LyXscrollpatch20130427.diff (1/1)

[Jean-Marc Lasgouttes]

Le 28/04/2013 13:06, pdv a écrit :

OK, here is a new version. Let me know if you experience anymore
problems.
For the time being I've left the clean-up step of the map as it was,
although I realize it's of limited value; when entering the same word
multiple times, the partial words get included anyway;


For now I only tried performance, and did not notice any problem. With 
the attached patch I get the following numbers.


The profiler only computes the time needed to recomputes the metrics. I 
do not know what other parts should be intrumented, probably around 
rowpainter.


Experiment is to load the user's guide, go to top and press PageDown 40 
times. The numbers are in milliseconds, 3 runs for each case.


My numbers are quite noisy actually, but it seems that the cost of 
calculate_qt_char_width is non-negligible. OTOH, I do not understand why 
calculate_qt_char_widths=0 does not give the same numbers as master? 
Didn't you say that it should be equivalent?


JMarc


Your patch with #define calculate_qt_char_widths 1

#pmprof# metrics: 4.43msec, count=49, total=216.90msec

#pmprof# metrics: 4.23msec, count=49, total=207.36msec

#pmprof# metrics: 4.49msec, count=49, total=219.83msec


Your patch with #define calculate_qt_char_widths 0

#pmprof# metrics: 3.05msec, count=49, total=149.61msec

#pmprof# metrics: 2.81msec, count=50, total=140.57msec

#pmprof# metrics: 2.74msec, count=50, total=136.93msec


Current master:

#pmprof# metrics: 3.18msec, count=50, total=158.78msec

#pmprof# metrics: 3.95msec, count=50, total=197.63msec

#pmprof# metrics: 3.97msec, count=50, total=198.61msec


diff --git a/src/BufferView.cpp b/src/BufferView.cpp
index 10b5263..4af1b14 100644
--- a/src/BufferView.cpp
+++ b/src/BufferView.cpp
@@ -78,6 +78,7 @@
 #include support/lstrings.h
 #include support/Package.h
 #include support/types.h
+#include support/pmprof.h
 
 #include cerrno
 #include fstream
@@ -2574,6 +2575,7 @@ bool BufferView::singleParUpdate()
 
 void BufferView::updateMetrics()
 {
+	PROFILE_THIS_BLOCK(metrics);
 	if (height_ == 0 || width_ == 0)
 		return;
 

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1) - LyXscrollpatch20130427.diff (1/1)

[Jean-Marc Lasgouttes]

Le 28/04/2013 13:06, pdv a écrit :

OK, here is a new version. Let me know if you experience anymore
problems.
For the time being I've left the clean-up step of the map as it was,
although I realize it's of limited value; when entering the same word
multiple times, the partial words get included anyway;


For now I only tried performance, and did not notice any problem. With 
the attached patch I get the following numbers.


The profiler only computes the time needed to recomputes the metrics. I 
do not know what other parts should be intrumented, probably around 
rowpainter.


Experiment is to load the user's guide, go to top and press PageDown 40 
times. The numbers are in milliseconds, 3 runs for each case.


My numbers are quite noisy actually, but it seems that the cost of 
calculate_qt_char_width is non-negligible. OTOH, I do not understand why 
calculate_qt_char_widths=0 does not give the same numbers as master? 
Didn't you say that it should be equivalent?


JMarc


Your patch with #define calculate_qt_char_widths 1

#pmprof# metrics: 4.43msec, count=49, total=216.90msec

#pmprof# metrics: 4.23msec, count=49, total=207.36msec

#pmprof# metrics: 4.49msec, count=49, total=219.83msec


Your patch with #define calculate_qt_char_widths 0

#pmprof# metrics: 3.05msec, count=49, total=149.61msec

#pmprof# metrics: 2.81msec, count=50, total=140.57msec

#pmprof# metrics: 2.74msec, count=50, total=136.93msec


Current master:

#pmprof# metrics: 3.18msec, count=50, total=158.78msec

#pmprof# metrics: 3.95msec, count=50, total=197.63msec

#pmprof# metrics: 3.97msec, count=50, total=198.61msec


diff --git a/src/BufferView.cpp b/src/BufferView.cpp
index 10b5263..4af1b14 100644
--- a/src/BufferView.cpp
+++ b/src/BufferView.cpp
@@ -78,6 +78,7 @@
 #include "support/lstrings.h"
 #include "support/Package.h"
 #include "support/types.h"
+#include "support/pmprof.h"
 
 #include 
 #include 
@@ -2574,6 +2575,7 @@ bool BufferView::singleParUpdate()
 
 void BufferView::updateMetrics()
 {
+	PROFILE_THIS_BLOCK(metrics);
 	if (height_ == 0 || width_ == 0)
 		return;
 

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1) - LyXscrollpatch20130427.diff (1/1)

[Jean-Marc Lasgouttes]

Le 28/04/2013 13:06, pdv a écrit :

OK, here is a new version. Let me know if you experience anymore
problems.
For the time being I've left the clean-up step of the map as it was,
although I realize it's of limited value; when entering the same word
multiple times, the partial words get included anyway;

The map itself is still defined in the BufferView class, but as a shared
static map. The reason for this is that when the user makes changes to
the screen fonts, the map becomes useless and I think the best option is
to clear it at that point.
I've added the case LFUN_SCREEN_FONT_UPDATE: to BufferView::dispatch()
and added a corresponding request in GuiApplication::dispatch();


You should move the code that computes text width to 
GuiFontMetrics::width(docstring). This is the right place for storing a 
map, that could be static if you want to keep your current solution or 
just a member of the GuiFontMetrics object so that you do not have to 
play tricks with font attributes.


Moreover, if your mùap is in GuiFontMetrics it will be reset 
automatically by the code in GuiApplication.cpp.


This will be much easier in my opinion.


I noticed then that the map was cleared and rebuild twice ...
... because in GuiPreferences::dispatchParams() there is first a call
dispatch(FuncRequest(LFUN_LYXRC_APPLY, ss.str()))
later followed by
dispatch(FuncRequest(LFUN_SCREEN_FONT_UPDATE))
but the first dispatch runs GuiReset() which also sends a
LFUN_SCREEN_FONT_UPDATE.
Therefore I've commented out the 2nd explicit LFUN_SCREEN_FONT_UPDATE
request.


Not sure about that, but things should become clearer once code is at 
the right place.


JMarc

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1) - LyXscrollpatch20130427.diff (1/1)

[Jean-Marc Lasgouttes]

Le 28/04/2013 13:06, pdv a écrit :

OK, here is a new version. Let me know if you experience anymore
problems.
For the time being I've left the clean-up step of the map as it was,
although I realize it's of limited value; when entering the same word
multiple times, the partial words get included anyway;

The map itself is still defined in the BufferView class, but as a shared
static map. The reason for this is that when the user makes changes to
the screen fonts, the map becomes useless and I think the best option is
to clear it at that point.
I've added the case LFUN_SCREEN_FONT_UPDATE: to BufferView::dispatch()
and added a corresponding request in GuiApplication::dispatch();


You should move the code that computes text width to 
GuiFontMetrics::width(docstring). This is the right place for storing a 
map, that could be static if you want to keep your current solution or 
just a member of the GuiFontMetrics object so that you do not have to 
play tricks with font attributes.


Moreover, if your mùap is in GuiFontMetrics it will be reset 
automatically by the code in GuiApplication.cpp.


This will be much easier in my opinion.


I noticed then that the map was cleared and rebuild twice ...
... because in GuiPreferences::dispatchParams() there is first a call
dispatch(FuncRequest(LFUN_LYXRC_APPLY, ss.str()))
later followed by
dispatch(FuncRequest(LFUN_SCREEN_FONT_UPDATE))
but the first dispatch runs GuiReset() which also sends a
LFUN_SCREEN_FONT_UPDATE.
Therefore I've commented out the 2nd explicit LFUN_SCREEN_FONT_UPDATE
request.


Not sure about that, but things should become clearer once code is at 
the right place.


JMarc

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1) - LyXscrollpatch20130427.diff (1/1)

[pdv]

In article 5178e9fc.6040...@lyx.org,
 Jean-Marc Lasgouttes lasgout...@lyx.org wrote:

 24/04/2013 21:41, pdv:
  Why do you add 0x61 to the values?
 
  That's just for easy reading when looking at what exactly gets written
  to the map; In this way the codes are a, b, c ...
 
 That makes sense.
 
  I do not think there is a need to have a map per document. A shared map
  stored in TextLetrics should be OK (like singleWidth currently).
 
  I was worried that the map might grow out of proportion, e.g. when
  leaving the application open for a long time and since there are more
  words than characters this will be worse than for singleWidth.
 
 I would start with the simple solution and then worry about a more 
 complicated one. A shared map will take less total memory. If acces time 
 is bad, then it may be worth implementing qHash(docstring) (by aping the 
 QString version) and use a QHashdoctring, int.
 
  When typing it's unavoidable to generate all partials of a word; these
  are removed again from the map so that only the final word remains;
  However nothing is done to remedy the reverse: when deleting a word
  character by character all partials will end-up in the map;
 
  Did you do some measurement to ensure that there is a gain of doing that?
 
  I don't think it makes so much difference in time. It was merely to
  avoid filling the map with useless entries, especially if a single map
  is used.
 
 My policy in these kind of situation is to implement the simple solution 
 and do optimization only when some measurement (profiling) proved that 
 there is a problem to solve. Remember Knuth quote: Premature 
 optimization is the root of all evil (or at least most of it) in 
 programming.
 
 Do not hesitate to post patches as often as needed. While I do not have 
 time to look at the hard parts of the patch right now, I think we move 
 in the right direction.
 
 JMarc

OK, here is a new version. Let me know if you experience anymore 
problems.
For the time being I've left the clean-up step of the map as it was, 
although I realize it's of limited value; when entering the same word 
multiple times, the partial words get included anyway;

The map itself is still defined in the BufferView class, but as a shared 
static map. The reason for this is that when the user makes changes to 
the screen fonts, the map becomes useless and I think the best option is 
to clear it at that point.
I've added the case LFUN_SCREEN_FONT_UPDATE: to BufferView::dispatch() 
and added a corresponding request in GuiApplication::dispatch();

I noticed then that the map was cleared and rebuild twice ...
... because in GuiPreferences::dispatchParams() there is first a call
dispatch(FuncRequest(LFUN_LYXRC_APPLY, ss.str()))
later followed by 
dispatch(FuncRequest(LFUN_SCREEN_FONT_UPDATE))
but the first dispatch runs GuiReset() which also sends a 
LFUN_SCREEN_FONT_UPDATE.
Therefore I've commented out the 2nd explicit LFUN_SCREEN_FONT_UPDATE 
request.

Regards,

P. De Visschere

begin 644 LyXscrollpatch20130427.diff
M9EF9B`M+6=I=!A+W-R8R]=69F97)6:65W+F-P!B+W-R8R]=69F97)6
M:65W+F-P`II;F1E`S86$R,#%B+BYD8S,W.6(P(#$P,#8T-`HM+2T@82]S
MF,O0G5F9F5R5FEE=RYC'`**RLK((OW)C+T)U9F9EE9I97N8W!PD!`
M(TY-C4L-R`K.38U+#$X($!`('9O:60@0G5F9F5R5FEE=SHZ=7!D871E1]C
M=6UE;G1#;%SRA$;V-U;65N=$-L87-S0V]NW10='(@;VQD9,IB`*(`EB
M=69F97)?+F5RF]RR@B0VQAW,@4W=I=-H(BD[B!]BT**PD**R\J*B!N
M;W)M86QL2!D969I;F5D(EN;EN90HK(H)=AIR!O;F4@:7,@9F]R(1E
M8G5G9VEN9PHK(HOBL)BMS=0Z.FUA#QD;V-S=')I;FL(EN=#X@0G5F
M9F5R5FEE=SHZ=5X=%]W:61T:%\[BL)BMV;VED($)U9F9EE9I97Z.G-E
M=%1E'17:61T:AD;V-S=')I;F@8V]NW0@)B!K97DL(EN=!W*2![BL)
M=5X=%]W:61T:%];:V5Y72`]('[BL)+R]L7AEG(@/#P@(FUA'-IF4@
M/2`B(#P\('1E'1?=VED=A?+G-IF4H*2`\/`B+!K97D@/2`B(#P\(ME
M2`\/!E;F1L.PHK?0HK(\OB`*(\J*B!2971UFX@=AE(-H86YG92!S
M=%T=7,@870@8W5RV]R('!OVET:6]N+!T86MI;F@:6X@86-C;W5N=!T
M:4*(`J('-T871UR!A=!E86-H(QE=F5L(]F('1H92!D;V-U;65N=!I
M=5R871OB`H82!T86)L92!I;B!A(1E;5T960*0$`@+3$V,38L-B`K,38R
M-RPQ,!`0!V;VED($)U9F9EE9I97Z.F1IW!A=-H*$9U;F-297%U97-T
M(-O;G-T(8@8VUD+!$:7-P871C:%)EW5L=`F(1R*0H@6-AV4@3$95
M3E]30U)%14Y?4D5#14Y415(ZB`)7)E8V5N=5R*D[B`)6)R96%K.PHK
M0D)BL)8V%S92!,1E5.7U-#4D5%3E]3TY47U501$%413H**PD)=5X=%]W
M:61T:%\N8VQE87(H*3L**PD)8G)E86L[B`*(`EC87-E($Q54Y?0DE5$58
M7T1!5$%05-%7T%$1#H@PH@0E#=7)S;W(@=UP8W5R(#t...@8w5r.pid:69F
M(TM9VET($OW)C+T)U9F9EE9I97N:!B+W-R8R]=69F97)6:65W+F@*
M:6YD97@@834X9#(U,2XN-CAD93DQ82`Q,#`V-#0*+2TM($OW)C+T)U9F9E
ME9I97N:`HK*RL@8B]SF,O0G5F9F5R5FEE=RYHD!`(TQ.PQ,`K,3@L
M,3(@0$`*(-I;F-L=61E()$;V-U;65N=$-L87-S4'1R+F@BB`C:6YC;'5D
M92`B=7!D871E7V9L86=S+F@BB`**R-I;F-L=61E()S=7!P;W)T+V1O8W-T
MFEN9RYH(@H@(VEN8VQU94@(G-U'!OG0OVAAF5D7W!TBYH(@HM(VEN
M8VQU94@(G-U'!OG0OW1R9G=D+F@BB`C:6YC;'5D92`BW5P]R=]T
M7!ERYH(@H@BLC:6YC;'5D92`\;6%P/@HKB!N86UEW!A8V4@;'EX('L*
M(`H@;F%M97-P86-E('-U'!OG0@R!C;%SR!:6QE3F%M93L@?0I`0`M
M,S(P+#8@*S,R,BPQ,B!`0!P=6)L:6,ZB`)8F]O;!C;EC:V%B;5);G-E
M=@I(-O;G-T.PH@2\O+PH@79O:60@;6%K941O8W5M96YT0VQAW,H*3L*
M*PD**PDO+R\@9G5N8W1I;VYS('5S960@8GD@55X=$UE=')I8W,Z.G1E'17

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1) - LyXscrollpatch20130427.diff (1/1)

[pdv]

In article <5178e9fc.6040...@lyx.org>,
 Jean-Marc Lasgouttes  wrote:

> 24/04/2013 21:41, pdv:
> >> Why do you add 0x61 to the values?
> >
> > That's just for easy reading when looking at what exactly gets written
> > to the map; In this way the codes are a, b, c ...
> 
> That makes sense.
> 
> >> I do not think there is a need to have a map per document. A shared map
> >> stored in TextLetrics should be OK (like singleWidth currently).
> >
> > I was worried that the map might grow out of proportion, e.g. when
> > leaving the application open for a long time and since there are more
> > words than characters this will be worse than for singleWidth.
> 
> I would start with the simple solution and then worry about a more 
> complicated one. A shared map will take less total memory. If acces time 
> is bad, then it may be worth implementing qHash(docstring) (by aping the 
> QString version) and use a QHash.
> 
> >>> When typing it's unavoidable to generate all partials of a word; these
> >>> are removed again from the map so that only the final word remains;
> >>> However nothing is done to remedy the reverse: when deleting a word
> >>> character by character all partials will end-up in the map;
> >>
> >> Did you do some measurement to ensure that there is a gain of doing that?
> >
> > I don't think it makes so much difference in time. It was merely to
> > avoid filling the map with useless entries, especially if a single map
> > is used.
> 
> My policy in these kind of situation is to implement the simple solution 
> and do optimization only when some measurement (profiling) proved that 
> there is a problem to solve. Remember Knuth quote: "Premature 
> optimization is the root of all evil (or at least most of it) in 
> programming."
> 
> Do not hesitate to post patches as often as needed. While I do not have 
> time to look at the hard parts of the patch right now, I think we move 
> in the right direction.
> 
> JMarc

OK, here is a new version. Let me know if you experience anymore 
problems.
For the time being I've left the clean-up step of the map as it was, 
although I realize it's of limited value; when entering the same word 
multiple times, the partial words get included anyway;

The map itself is still defined in the BufferView class, but as a shared 
static map. The reason for this is that when the user makes changes to 
the screen fonts, the map becomes useless and I think the best option is 
to clear it at that point.
I've added the case LFUN_SCREEN_FONT_UPDATE: to BufferView::dispatch() 
and added a corresponding request in GuiApplication::dispatch();

I noticed then that the map was cleared and rebuild twice ...
... because in GuiPreferences::dispatchParams() there is first a call
dispatch(FuncRequest(LFUN_LYXRC_APPLY, ss.str()))
later followed by 
dispatch(FuncRequest(LFUN_SCREEN_FONT_UPDATE))
but the first dispatch runs GuiReset() which also sends a 
LFUN_SCREEN_FONT_UPDATE.
Therefore I've commented out the 2nd explicit LFUN_SCREEN_FONT_UPDATE 
request.

Regards,

P. De Visschere

begin 644 LyXscrollpatch20130427.diff
M9`M+6=I="!A+W-R8R]"=69F97)6:65W+F-P<"!B+W-R8R]"=69F97)6
M:65W+F-P<`II;F1E>"`S86$R,#%B+BYD8S,W.6(P(#$P,#8T-`HM+2T@82]S
M

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1)

[Jean-Marc Lasgouttes]

24/04/2013 21:41, pdv:

Why do you add 0x61 to the values?


That's just for easy reading when looking at what exactly gets written
to the map; In this way the codes are a, b, c ...


That makes sense.


I do not think there is a need to have a map per document. A shared map
stored in TextLetrics should be OK (like singleWidth currently).


I was worried that the map might grow out of proportion, e.g. when
leaving the application open for a long time and since there are more
words than characters this will be worse than for singleWidth.


I would start with the simple solution and then worry about a more 
complicated one. A shared map will take less total memory. If acces time 
is bad, then it may be worth implementing qHash(docstring) (by aping the 
QString version) and use a QHashdoctring, int.



When typing it's unavoidable to generate all partials of a word; these
are removed again from the map so that only the final word remains;
However nothing is done to remedy the reverse: when deleting a word
character by character all partials will end-up in the map;


Did you do some measurement to ensure that there is a gain of doing that?


I don't think it makes so much difference in time. It was merely to
avoid filling the map with useless entries, especially if a single map
is used.


My policy in these kind of situation is to implement the simple solution 
and do optimization only when some measurement (profiling) proved that 
there is a problem to solve. Remember Knuth quote: Premature 
optimization is the root of all evil (or at least most of it) in 
programming.


Do not hesitate to post patches as often as needed. While I do not have 
time to look at the hard parts of the patch right now, I think we move 
in the right direction.


JMarc

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1)

[Jean-Marc Lasgouttes]

24/04/2013 21:14, pdv:

Sorry, the problem is obvious.
I'm using cmake and although --enable-stdlib-debug is mentioned in the
install notes, I can't set that option.
Once (more or less) ready I build manually with the
LyX-Mac-binary-release.sh script, which by default disables this option.
When I enable it, lyx crashes right away when starting. Don't know yet
whether this signals other problems or if there is a problem with this
option.


Actually the stdlib-debug option does not work on OS X (it may depend on 
gcc and boost versions, I do not know). But I can use it on linux and 
tell you what I find :)


JMarc

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1)

[Jean-Marc Lasgouttes]

24/04/2013 21:41, pdv:

Why do you add 0x61 to the values?


That's just for easy reading when looking at what exactly gets written
to the map; In this way the codes are a, b, c ...


That makes sense.


I do not think there is a need to have a map per document. A shared map
stored in TextLetrics should be OK (like singleWidth currently).


I was worried that the map might grow out of proportion, e.g. when
leaving the application open for a long time and since there are more
words than characters this will be worse than for singleWidth.


I would start with the simple solution and then worry about a more 
complicated one. A shared map will take less total memory. If acces time 
is bad, then it may be worth implementing qHash(docstring) (by aping the 
QString version) and use a QHash.



When typing it's unavoidable to generate all partials of a word; these
are removed again from the map so that only the final word remains;
However nothing is done to remedy the reverse: when deleting a word
character by character all partials will end-up in the map;


Did you do some measurement to ensure that there is a gain of doing that?


I don't think it makes so much difference in time. It was merely to
avoid filling the map with useless entries, especially if a single map
is used.


My policy in these kind of situation is to implement the simple solution 
and do optimization only when some measurement (profiling) proved that 
there is a problem to solve. Remember Knuth quote: "Premature 
optimization is the root of all evil (or at least most of it) in 
programming."


Do not hesitate to post patches as often as needed. While I do not have 
time to look at the hard parts of the patch right now, I think we move 
in the right direction.


JMarc

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1)

[Jean-Marc Lasgouttes]

24/04/2013 21:14, pdv:

Sorry, the problem is obvious.
I'm using cmake and although --enable-stdlib-debug is mentioned in the
install notes, I can't set that option.
Once (more or less) ready I build manually with the
LyX-Mac-binary-release.sh script, which by default disables this option.
When I enable it, lyx crashes right away when starting. Don't know yet
whether this signals other problems or if there is a problem with this
option.


Actually the stdlib-debug option does not work on OS X (it may depend on 
gcc and boost versions, I do not know). But I can use it on linux and 
tell you what I find :)


JMarc

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1)

[pdv]

In article 5176c559.3030...@lyx.org,
 Jean-Marc Lasgouttes lasgout...@lyx.org wrote:

 Le 21/04/2013 21:24, pdv a écrit :
  Of course, but I wanted a fully functional patch before posting.
  As far as I have tested the functionality should be largely OK now (at
  least the User's Guide and some of my documents are displayed and
  handled correctly).
  There might still be problems with the coding style ...
 
 I have compilation problems with your patch, that are solved by 
 including support/doctring.H instead of strfwd.h in BufferView.h.
 Also, one should use char_type instead of wchar_t in the code.
 
 When running the code with stdlib-debug enables, I get the assertion 
 below on user guide. There is a i + 1  end test missing around line 
 1020 of TextMetrics.cpp.

Sorry, the problem is obvious.
I'm using cmake and although --enable-stdlib-debug is mentioned in the 
install notes, I can't set that option.
Once (more or less) ready I build manually with the 
LyX-Mac-binary-release.sh script, which by default disables this option.
When I enable it, lyx crashes right away when starting. Don't know yet 
whether this signals other problems or if there is a problem with this 
option.


 
 JMarc
 
 
 /usr/include/c++/4.7/bits/basic_string.h:845: std::basic_stringChar, 
 Traits, Alloc::reference std::basic_stringChar, Traits, 
 Alloc::operator[](std::basic_stringChar, Traits, Alloc::size_type) 
 [with _CharT = wchar_t; _Traits = std::char_traitswchar_t; _Alloc = 
 std::allocatorwchar_t; std::basic_stringChar, Traits, 
 Alloc::reference = wchar_t; std::basic_stringChar, Traits, 
 Alloc::size_type = unsigned int]: Assertion '__pos  size()' failed.
 
 Program received signal SIGABRT, Aborted.
 0xb7fdd424 in __kernel_vsyscall ()
 (gdb) bt
 #0  0xb7fdd424 in __kernel_vsyscall ()
 #1  0xb6f1e1df in __GI_raise (sig=6)
  at ../nptl/sysdeps/unix/sysv/linux/raise.c:64
 #2  0xb6f21825 in __GI_abort () at abort.c:91
 #3  0x08081d1a in std::__replacement_assert (
  __file=0x88a3f34 /usr/include/c++/4.7/bits/basic_string.h, 
 __line=845,
  __function=0x88a4aa0 std::basic_stringwchar_t, 
 std::char_traitswchar_t, std::allocatorwchar_t 
  ::operator[](unsigned int)::__PRETTY_FUNCTION__ 
 std::basic_stringChar, Traits, Alloc::reference 
 std::basic_stringChar, Traits, 
 Alloc::operator[](std::basic_stringChar, Traits, Alloc::size_type) 
 [with _CharT = wchar_t; _Traits = std::char_trai...,
  __condition=0x88a3f5d __pos  size())
  at /usr/include/c++/4.7/i686-linux-gnu/bits/c++config.h:361
 #4  0x080841a2 in std::basic_stringwchar_t, std::char_traitswchar_t, 
 std::allocatorwchar_t ::operator[] (this=0x973861c, __pos=20)
  at /usr/include/c++/4.7/bits/basic_string.h:845
 #5  0x0828d7af in lyx::Paragraph::isLineSeparator (this=0x9756658, pos=20)
  at ../../master/src/Paragraph.cpp:2994
 #6  0x082fc113 in lyx::TextMetrics::rowBreakPoint (this=0xa16b114, 
 width=628,
  pit=0, pos=0) at ../../master/src/TextMetrics.cpp:1020

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1)

[pdv]

In article 5176ae47.9050...@lyx.org,
 Jean-Marc Lasgouttes lasgout...@lyx.org wrote:

 Le 21/04/2013 21:24, pdv a écrit :
  Hereafter I've listed some comments which might be helpful or answer
  suggestions you've made earlier. (btw I couldn't find support/pmprof.h)
  If you have comments, questions ... I'm looking forward to hearing from
  you.
 
 I need some time to really understand the code. I will probably do that 
 during LDM in Milano at the beginning of May. In  the meantime, here are 
 some remarks.
 
 Some general comments:
 
 - please drop the //pdv comments, since they will eventually not be 
 necessary anymore.

I agree; this was just temporary to easily find what I did modify.

 - please add the #include directives at the right place (separate Qt 
 headers from support and frontend ones).
 
  - there are 4 functions where the textwidth must be calculated: in
  GuiPainter::text() and in 3 functions in the TextMetrics class, in order
  of complexity:
  (1) cursorX(): find the position in pixels, given the cursor position in
  characters,
  (2) rowBreakPoint(): find the next breakpoint of a row, this will always
  break between words,
  (3) getColumnNearX(): actually the reverse operation of (1); more
  complex than (2) since the position can also be located within a word.
 
  For the time being these 3 functions are still independent, although
  they are somewhat similar and maybe some more streamlining is possible
  here. I'm also aware that the code for getColumnNearX() is rather
  complex.
 
 I would think that all computation could be done in rowBreakPoint and 
 that information could be kept in some data structure, so that the other 
 methods can reuse them.
 
  The widths calculated are cached in a std::mapdocstring, int. I've
  also tried QHash but since docstring has no qhash function all strings
  must then be converted to QStrings and there is no speed gain.
 
 OK.
 
  I use only one map, the fonts are coded as a string of 4 characters
  (family, series, shape, size) which is then used as a prefix for the
  key. I have not tried alternatives like using a map for each used font.
 
 Why do you add 0x61 to the values?

That's just for easy reading when looking at what exactly gets written 
to the map; In this way the codes are a, b, c ...

 
  The map itself is stored in the BufferView class; In this way there is
  one map for each document; when storing the map in the TextMetrics
  class, multiple maps are generated. I have only tested this with simple
  documents (no child documents S).
 
 I do not think there is a need to have a map per document. A shared map 
 stored in TextLetrics should be OK (like singleWidth currently).

I was worried that the map might grow out of proportion, e.g. when 
leaving the application open for a long time and since there are more 
words than characters this will be worse than for singleWidth.


 
  When typing it's unavoidable to generate all partials of a word; these
  are removed again from the map so that only the final word remains;
  However nothing is done to remedy the reverse: when deleting a word
  character by character all partials will end-up in the map;
 
 Did you do some measurement to ensure that there is a gain of doing that?

I don't think it makes so much difference in time. It was merely to 
avoid filling the map with useless entries, especially if a single map 
is used.

Thanks for the feedback.

 
  The old code is still in place and the old painting character by
  character can be restored by changing 2 appropriate macro definitions.
 
 Thanks,
 JMarc

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1)

[pdv]

In article <5176c559.3030...@lyx.org>,
 Jean-Marc Lasgouttes  wrote:

> Le 21/04/2013 21:24, pdv a écrit :
> > Of course, but I wanted a fully functional patch before posting.
> > As far as I have tested the functionality should be largely OK now (at
> > least the User's Guide and some of my documents are displayed and
> > handled correctly).
> > There might still be problems with the coding style ...
> 
> I have compilation problems with your patch, that are solved by 
> including  instead of strfwd.h in BufferView.h.
> Also, one should use char_type instead of wchar_t in the code.
> 
> When running the code with stdlib-debug enables, I get the assertion 
> below on user guide. There is a "i + 1 < end" test missing around line 
> 1020 of TextMetrics.cpp.

Sorry, the problem is obvious.
I'm using cmake and although --enable-stdlib-debug is mentioned in the 
install notes, I can't set that option.
Once (more or less) ready I build manually with the 
LyX-Mac-binary-release.sh script, which by default disables this option.
When I enable it, lyx crashes right away when starting. Don't know yet 
whether this signals other problems or if there is a problem with this 
option.


> 
> JMarc
> 
> 
> /usr/include/c++/4.7/bits/basic_string.h:845: std::basic_string Traits, Alloc>::reference std::basic_string Alloc>::operator[](std::basic_string::size_type) 
> [with _CharT = wchar_t; _Traits = std::char_traits; _Alloc = 
> std::allocator; std::basic_string Alloc>::reference = wchar_t&; std::basic_string Alloc>::size_type = unsigned int]: Assertion '__pos < size()' failed.
> 
> Program received signal SIGABRT, Aborted.
> 0xb7fdd424 in __kernel_vsyscall ()
> (gdb) bt
> #0  0xb7fdd424 in __kernel_vsyscall ()
> #1  0xb6f1e1df in __GI_raise (sig=6)
>  at ../nptl/sysdeps/unix/sysv/linux/raise.c:64
> #2  0xb6f21825 in __GI_abort () at abort.c:91
> #3  0x08081d1a in std::__replacement_assert (
>  __file=0x88a3f34 "/usr/include/c++/4.7/bits/basic_string.h", 
> __line=845,
>  __function=0x88a4aa0  std::char_traits, std::allocator 
>  >::operator[](unsigned int)::__PRETTY_FUNCTION__> 
> "std::basic_string::reference 
> std::basic_string Alloc>::operator[](std::basic_string::size_type) 
> [with _CharT = wchar_t; _Traits = std::char_trai"...,
>  __condition=0x88a3f5d "__pos < size()")
>  at /usr/include/c++/4.7/i686-linux-gnu/bits/c++config.h:361
> #4  0x080841a2 in std::basic_string std::allocator >::operator[] (this=0x973861c, __pos=20)
>  at /usr/include/c++/4.7/bits/basic_string.h:845
> #5  0x0828d7af in lyx::Paragraph::isLineSeparator (this=0x9756658, pos=20)
>  at ../../master/src/Paragraph.cpp:2994
> #6  0x082fc113 in lyx::TextMetrics::rowBreakPoint (this=0xa16b114, 
> width=628,
>  pit=0, pos=0) at ../../master/src/TextMetrics.cpp:1020

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1)

[pdv]

In article <5176ae47.9050...@lyx.org>,
 Jean-Marc Lasgouttes  wrote:

> Le 21/04/2013 21:24, pdv a écrit :
> > Hereafter I've listed some comments which might be helpful or answer
> > suggestions you've made earlier. (btw I couldn't find support/pmprof.h)
> > If you have comments, questions ... I'm looking forward to hearing from
> > you.
> 
> I need some time to really understand the code. I will probably do that 
> during LDM in Milano at the beginning of May. In  the meantime, here are 
> some remarks.
> 
> Some general comments:
> 
> - please drop the //pdv comments, since they will eventually not be 
> necessary anymore.

I agree; this was just temporary to easily find what I did modify.

> - please add the #include directives at the right place (separate Qt 
> headers from support and frontend ones).
> 
> > - there are 4 functions where the textwidth must be calculated: in
> > GuiPainter::text() and in 3 functions in the TextMetrics class, in order
> > of complexity:
> > (1) cursorX(): find the position in pixels, given the cursor position in
> > characters,
> > (2) rowBreakPoint(): find the next breakpoint of a row, this will always
> > break between words,
> > (3) getColumnNearX(): actually the reverse operation of (1); more
> > complex than (2) since the position can also be located within a word.
> >
> > For the time being these 3 functions are still independent, although
> > they are somewhat similar and maybe some more streamlining is possible
> > here. I'm also aware that the code for getColumnNearX() is rather
> > complex.
> 
> I would think that all computation could be done in rowBreakPoint and 
> that information could be kept in some data structure, so that the other 
> methods can reuse them.
> 
> > The widths calculated are cached in a std::map. I've
> > also tried QHash but since docstring has no qhash function all strings
> > must then be converted to QStrings and there is no speed gain.
> 
> OK.
> 
> > I use only one map, the fonts are coded as a string of 4 characters
> > (family, series, shape, size) which is then used as a prefix for the
> > key. I have not tried alternatives like using a map for each used font.
> 
> Why do you add 0x61 to the values?

That's just for easy reading when looking at what exactly gets written 
to the map; In this way the codes are a, b, c ...

> 
> > The map itself is stored in the BufferView class; In this way there is
> > one map for each document; when storing the map in the TextMetrics
> > class, multiple maps are generated. I have only tested this with simple
> > documents (no child documents S).
> 
> I do not think there is a need to have a map per document. A shared map 
> stored in TextLetrics should be OK (like singleWidth currently).

I was worried that the map might grow out of proportion, e.g. when 
leaving the application open for a long time and since there are more 
words than characters this will be worse than for singleWidth.


> 
> > When typing it's unavoidable to generate all partials of a word; these
> > are removed again from the map so that only the final word remains;
> > However nothing is done to remedy the reverse: when deleting a word
> > character by character all partials will end-up in the map;
> 
> Did you do some measurement to ensure that there is a gain of doing that?

I don't think it makes so much difference in time. It was merely to 
avoid filling the map with useless entries, especially if a single map 
is used.

Thanks for the feedback.

> 
> > The old code is still in place and the old painting character by
> > character can be restored by changing 2 appropriate macro definitions.
> 
> Thanks,
> JMarc

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1)

[Jean-Marc Lasgouttes]

Le 21/04/2013 21:24, pdv a écrit :

Hereafter I've listed some comments which might be helpful or answer
suggestions you've made earlier. (btw I couldn't find support/pmprof.h)
If you have comments, questions ... I'm looking forward to hearing from
you.


I need some time to really understand the code. I will probably do that 
during LDM in Milano at the beginning of May. In  the meantime, here are 
some remarks.


Some general comments:

- please drop the //pdv comments, since they will eventually not be 
necessary anymore.
- please add the #include directives at the right place (separate Qt 
headers from support and frontend ones).



- there are 4 functions where the textwidth must be calculated: in
GuiPainter::text() and in 3 functions in the TextMetrics class, in order
of complexity:
(1) cursorX(): find the position in pixels, given the cursor position in
characters,
(2) rowBreakPoint(): find the next breakpoint of a row, this will always
break between words,
(3) getColumnNearX(): actually the reverse operation of (1); more
complex than (2) since the position can also be located within a word.

For the time being these 3 functions are still independent, although
they are somewhat similar and maybe some more streamlining is possible
here. I'm also aware that the code for getColumnNearX() is rather
complex.


I would think that all computation could be done in rowBreakPoint and 
that information could be kept in some data structure, so that the other 
methods can reuse them.



The widths calculated are cached in a std::mapdocstring, int. I've
also tried QHash but since docstring has no qhash function all strings
must then be converted to QStrings and there is no speed gain.


OK.


I use only one map, the fonts are coded as a string of 4 characters
(family, series, shape, size) which is then used as a prefix for the
key. I have not tried alternatives like using a map for each used font.


Why do you add 0x61 to the values?


The map itself is stored in the BufferView class; In this way there is
one map for each document; when storing the map in the TextMetrics
class, multiple maps are generated. I have only tested this with simple
documents (no child documents Š).


I do not think there is a need to have a map per document. A shared map 
stored in TextLetrics should be OK (like singleWidth currently).



When typing it's unavoidable to generate all partials of a word; these
are removed again from the map so that only the final word remains;
However nothing is done to remedy the reverse: when deleting a word
character by character all partials will end-up in the map;


Did you do some measurement to ensure that there is a gain of doing that?


The old code is still in place and the old painting character by
character can be restored by changing 2 appropriate macro definitions.


Thanks,
JMarc

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1)

[Jean-Marc Lasgouttes]

Le 21/04/2013 21:24, pdv a écrit :

(btw I couldn't find support/pmprof.h)


You should find it if you update to latest master (git update --rebase).

JMarc

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1)

[Jean-Marc Lasgouttes]

Le 21/04/2013 21:24, pdv a écrit :

Of course, but I wanted a fully functional patch before posting.
As far as I have tested the functionality should be largely OK now (at
least the User's Guide and some of my documents are displayed and
handled correctly).
There might still be problems with the coding style ...


I have compilation problems with your patch, that are solved by 
including support/doctring.H instead of strfwd.h in BufferView.h.

Also, one should use char_type instead of wchar_t in the code.

When running the code with stdlib-debug enables, I get the assertion 
below on user guide. There is a i + 1  end test missing around line 
1020 of TextMetrics.cpp.


JMarc


/usr/include/c++/4.7/bits/basic_string.h:845: std::basic_stringChar, 
Traits, Alloc::reference std::basic_stringChar, Traits, 
Alloc::operator[](std::basic_stringChar, Traits, Alloc::size_type) 
[with _CharT = wchar_t; _Traits = std::char_traitswchar_t; _Alloc = 
std::allocatorwchar_t; std::basic_stringChar, Traits, 
Alloc::reference = wchar_t; std::basic_stringChar, Traits, 
Alloc::size_type = unsigned int]: Assertion '__pos  size()' failed.


Program received signal SIGABRT, Aborted.
0xb7fdd424 in __kernel_vsyscall ()
(gdb) bt
#0  0xb7fdd424 in __kernel_vsyscall ()
#1  0xb6f1e1df in __GI_raise (sig=6)
at ../nptl/sysdeps/unix/sysv/linux/raise.c:64
#2  0xb6f21825 in __GI_abort () at abort.c:91
#3  0x08081d1a in std::__replacement_assert (
__file=0x88a3f34 /usr/include/c++/4.7/bits/basic_string.h, 
__line=845,
__function=0x88a4aa0 std::basic_stringwchar_t, 
std::char_traitswchar_t, std::allocatorwchar_t 
::operator[](unsigned int)::__PRETTY_FUNCTION__ 
std::basic_stringChar, Traits, Alloc::reference 
std::basic_stringChar, Traits, 
Alloc::operator[](std::basic_stringChar, Traits, Alloc::size_type) 
[with _CharT = wchar_t; _Traits = std::char_trai...,

__condition=0x88a3f5d __pos  size())
at /usr/include/c++/4.7/i686-linux-gnu/bits/c++config.h:361
#4  0x080841a2 in std::basic_stringwchar_t, std::char_traitswchar_t, 
std::allocatorwchar_t ::operator[] (this=0x973861c, __pos=20)

at /usr/include/c++/4.7/bits/basic_string.h:845
#5  0x0828d7af in lyx::Paragraph::isLineSeparator (this=0x9756658, pos=20)
at ../../master/src/Paragraph.cpp:2994
#6  0x082fc113 in lyx::TextMetrics::rowBreakPoint (this=0xa16b114, 
width=628,

pit=0, pos=0) at ../../master/src/TextMetrics.cpp:1020

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1)

[Jean-Marc Lasgouttes]

Le 21/04/2013 21:24, pdv a écrit :

Hereafter I've listed some comments which might be helpful or answer
suggestions you've made earlier. (btw I couldn't find support/pmprof.h)
If you have comments, questions ... I'm looking forward to hearing from
you.


I need some time to really understand the code. I will probably do that 
during LDM in Milano at the beginning of May. In  the meantime, here are 
some remarks.


Some general comments:

- please drop the //pdv comments, since they will eventually not be 
necessary anymore.
- please add the #include directives at the right place (separate Qt 
headers from support and frontend ones).



- there are 4 functions where the textwidth must be calculated: in
GuiPainter::text() and in 3 functions in the TextMetrics class, in order
of complexity:
(1) cursorX(): find the position in pixels, given the cursor position in
characters,
(2) rowBreakPoint(): find the next breakpoint of a row, this will always
break between words,
(3) getColumnNearX(): actually the reverse operation of (1); more
complex than (2) since the position can also be located within a word.

For the time being these 3 functions are still independent, although
they are somewhat similar and maybe some more streamlining is possible
here. I'm also aware that the code for getColumnNearX() is rather
complex.


I would think that all computation could be done in rowBreakPoint and 
that information could be kept in some data structure, so that the other 
methods can reuse them.



The widths calculated are cached in a std::map. I've
also tried QHash but since docstring has no qhash function all strings
must then be converted to QStrings and there is no speed gain.


OK.


I use only one map, the fonts are coded as a string of 4 characters
(family, series, shape, size) which is then used as a prefix for the
key. I have not tried alternatives like using a map for each used font.


Why do you add 0x61 to the values?


The map itself is stored in the BufferView class; In this way there is
one map for each document; when storing the map in the TextMetrics
class, multiple maps are generated. I have only tested this with simple
documents (no child documents Š).


I do not think there is a need to have a map per document. A shared map 
stored in TextLetrics should be OK (like singleWidth currently).



When typing it's unavoidable to generate all partials of a word; these
are removed again from the map so that only the final word remains;
However nothing is done to remedy the reverse: when deleting a word
character by character all partials will end-up in the map;


Did you do some measurement to ensure that there is a gain of doing that?


The old code is still in place and the old painting character by
character can be restored by changing 2 appropriate macro definitions.


Thanks,
JMarc

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1)

[Jean-Marc Lasgouttes]

Le 21/04/2013 21:24, pdv a écrit :

(btw I couldn't find support/pmprof.h)


You should find it if you update to latest master (git update --rebase).

JMarc

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1)

[Jean-Marc Lasgouttes]

Le 21/04/2013 21:24, pdv a écrit :

Of course, but I wanted a fully functional patch before posting.
As far as I have tested the functionality should be largely OK now (at
least the User's Guide and some of my documents are displayed and
handled correctly).
There might still be problems with the coding style ...


I have compilation problems with your patch, that are solved by 
including  instead of strfwd.h in BufferView.h.

Also, one should use char_type instead of wchar_t in the code.

When running the code with stdlib-debug enables, I get the assertion 
below on user guide. There is a "i + 1 < end" test missing around line 
1020 of TextMetrics.cpp.


JMarc


/usr/include/c++/4.7/bits/basic_string.h:845: std::basic_string::reference std::basic_string::operator[](std::basic_string::size_type) 
[with _CharT = wchar_t; _Traits = std::char_traits; _Alloc = 
std::allocator; std::basic_string::reference = wchar_t&; std::basic_string::size_type = unsigned int]: Assertion '__pos < size()' failed.


Program received signal SIGABRT, Aborted.
0xb7fdd424 in __kernel_vsyscall ()
(gdb) bt
#0  0xb7fdd424 in __kernel_vsyscall ()
#1  0xb6f1e1df in __GI_raise (sig=6)
at ../nptl/sysdeps/unix/sysv/linux/raise.c:64
#2  0xb6f21825 in __GI_abort () at abort.c:91
#3  0x08081d1a in std::__replacement_assert (
__file=0x88a3f34 "/usr/include/c++/4.7/bits/basic_string.h", 
__line=845,
__function=0x88a4aa0 ::operator[](unsigned int)::__PRETTY_FUNCTION__> 
"std::basic_string::reference 
std::basic_string::operator[](std::basic_string::size_type) 
[with _CharT = wchar_t; _Traits = std::char_trai"...,

__condition=0x88a3f5d "__pos < size()")
at /usr/include/c++/4.7/i686-linux-gnu/bits/c++config.h:361
#4  0x080841a2 in std::basic_string::operator[] (this=0x973861c, __pos=20)

at /usr/include/c++/4.7/bits/basic_string.h:845
#5  0x0828d7af in lyx::Paragraph::isLineSeparator (this=0x9756658, pos=20)
at ../../master/src/Paragraph.cpp:2994
#6  0x082fc113 in lyx::TextMetrics::rowBreakPoint (this=0xa16b114, 
width=628,

pit=0, pos=0) at ../../master/src/TextMetrics.cpp:1020

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1)

[pdv]

In article 516eabdf.5090...@lyx.org,
 Jean-Marc Lasgouttes lasgout...@lyx.org wrote:

 17/04/2013 00:05, pdv:
  I do now all the width-calculations in one function.
  In that function the widths are cached in a mapdocstring,int.
  I then obtain again a substantial speed increase.
  For testing I defined a static map, but I assume that eventually the map
  should be stored in a suitable object so that the map doesn't grow out
  of proportion.
 
 You need to keep one map for each FontInfo instance (like what is done 
 for singleWidth). I agree that may use a lot of memory. This is 
 something that can be measured using the massif tool of valgrind.
 
  So far I can handle all basic content properly. I haven't paid much
  attention to inlinecompletion (at one point it posed no problem, but I
  have to check that again), but I just discovered an issue with the
  Labeling style ...
 
 Great. If you want to do basic timing before/after, have a look at 
 support/pmprof.h.
 
 Can we see the patch?
 
 JMarc

Of course, but I wanted a fully functional patch before posting.
As far as I have tested the functionality should be largely OK now (at 
least the User's Guide and some of my documents are displayed and 
handled correctly).
There might still be problems with the coding style ...

Hereafter I've listed some comments which might be helpful or answer 
suggestions you've made earlier. (btw I couldn't find support/pmprof.h)
If you have comments, questions ... I'm looking forward to hearing from 
you.

Regards,

P. De Visschere

- there are 4 functions where the textwidth must be calculated: in 
GuiPainter::text() and in 3 functions in the TextMetrics class, in order 
of complexity:
(1) cursorX(): find the position in pixels, given the cursor position in 
characters,
(2) rowBreakPoint(): find the next breakpoint of a row, this will always 
break between words,
(3) getColumnNearX(): actually the reverse operation of (1); more 
complex than (2) since the position can also be located within a word.

For the time being these 3 functions are still independent, although 
they are somewhat similar and maybe some more streamlining is possible 
here. I'm also aware that the code for getColumnNearX() is rather 
complex.

The widths are calculated in a single TextMetrics::textWidth() function 
which accepts a parameter to cache the result or not (e.g. widths of 
partial words needed when positioning the cursor within a word are not 
cached).

The widths calculated are cached in a std::mapdocstring, int. I've 
also tried QHash but since docstring has no qhash function all strings 
must then be converted to QStrings and there is no speed gain.

I use only one map, the fonts are coded as a string of 4 characters 
(family, series, shape, size) which is then used as a prefix for the 
key. I have not tried alternatives like using a map for each used font.

The map itself is stored in the BufferView class; In this way there is 
one map for each document; when storing the map in the TextMetrics 
class, multiple maps are generated. I have only tested this with simple 
documents (no child documents Š).

When typing it's unavoidable to generate all partials of a word; these 
are removed again from the map so that only the final word remains; 
However nothing is done to remedy the reverse: when deleting a word 
character by character all partials will end-up in the map;

The old code is still in place and the old painting character by 
character can be restored by changing 2 appropriate macro definitions.

begin 644 LyXscrollpatch20130421.diff
M9EF9B`M+6=I=!A+W-R8R]=69F97)6:65W+F-P!B+W-R8R]=69F97)6
M:65W+F-P`II;F1E`S86$R,#%B+BYF,S0X,F%D(#$P,#8T-`HM+2T@82]S
MF,O0G5F9F5R5FEE=RYC'`**RLK((OW)C+T)U9F9EE9I97N8W!PD!`
M(TY-C4L-R`K.38U+#$U($!`('9O:60@0G5F9F5R5FEE=SHZ=7!D871E1]C
M=6UE;G1#;%SRA$;V-U;65N=$-L87-S0V]NW10='(@;VQD9,IB`*(`EB
M=69F97)?+F5RF]RR@B0VQAW,@4W=I=-H(BD[B!]BT**PD**R\J*B!N
M;W)M86QL2!D969I;F5D(EN;EN90HK(H)=AIR!O;F4@:7,@9F]R(1E
M8G5G9VEN9PHK(HOBMV;VED($)U9F9EE9I97Z.G-E=%1E'17:61T:AD
M;V-S=')I;F@8V]NW0@)B!K97DL(EN=!W*2![BL)=5X=%]W:61T:%];
M:V5Y72`]('[BL)+R]L7AEG(@/#P@(FUA'-IF4@/2`B(#P\('1E'1?
M=VED=A?+G-IF4H*2`\/`B+!K97D@/2`B(#P\(ME2`\/!E;F1L.PHK
M?0HK(\OB`*(\J*B!2971UFX@=AE(-H86YG92!S=%T=7,@870@8W5R
MV]R('!OVET:6]N+!T86MI;F@:6X@86-C;W5N=!T:4*(`J('-T871U
MR!A=!E86-H(QE=F5L(]F('1H92!D;V-U;65N=!I=5R871OB`H82!T
M86)L92!I;B!A(1E;5T960*9EF9B`M+6=I=!A+W-R8R]=69F97)6:65W
M+F@@8B]SF,O0G5F9F5R5FEE=RYHFEN95X($U.0R-3$N+C(Y,3(T864@
M,3`P-C0TBTM+2!A+W-R8R]=69F97)6:65W+F@**RLK((OW)C+T)U9F9E
ME9I97N:`I`0`M,C$L-B`K,C$L-R!`0`H@(VEN8VQU94@(G-U'!OG0O
MVAAF5D7W!TBYH(@H@(VEN8VQU94@(G-U'!OG0OW1R9G=D+F@BB`C
M:6YC;'5D92`BW5P]R=]T7!ERYH(@HK(VEN8VQU94@/UA#X@+R\@
M1VB`*(YA;65S%C92!L7@@PH@D!`(TS,C`L-B`K,S(Q+#$R($!`
M('!U8FQI8SH*(`EB;V]L(-L:6-K86)L94ENV5T*D@8V]NW0[B`)+R\O
MB`)=F]I9!M86ME1]C=6UE;G1#;%Sr...@i.phk0HK2\O+R!F=6YC=EO
M;G,@=7-E9!B2!497AT365TFECSHZ=5X=%=I9'1H*D**PDO+R\@9F]R
M(%C8V5SVEN9R!T:4@;6%P('=I=@@=V]R9'=I9'1HPHK6EN=!G9714

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1)

[pdv]

In article <516eabdf.5090...@lyx.org>,
 Jean-Marc Lasgouttes  wrote:

> 17/04/2013 00:05, pdv:
> > I do now all the width-calculations in one function.
> > In that function the widths are cached in a map.
> > I then obtain again a substantial speed increase.
> > For testing I defined a static map, but I assume that eventually the map
> > should be stored in a suitable object so that the map doesn't grow out
> > of proportion.
> 
> You need to keep one map for each FontInfo instance (like what is done 
> for singleWidth). I agree that may use a lot of memory. This is 
> something that can be measured using the massif tool of valgrind.
> 
> > So far I can handle all basic content properly. I haven't paid much
> > attention to inlinecompletion (at one point it posed no problem, but I
> > have to check that again), but I just discovered an issue with the
> > Labeling style ...
> 
> Great. If you want to do basic timing before/after, have a look at 
> support/pmprof.h.
> 
> Can we see the patch?
> 
> JMarc

Of course, but I wanted a fully functional patch before posting.
As far as I have tested the functionality should be largely OK now (at 
least the User's Guide and some of my documents are displayed and 
handled correctly).
There might still be problems with the coding style ...

Hereafter I've listed some comments which might be helpful or answer 
suggestions you've made earlier. (btw I couldn't find support/pmprof.h)
If you have comments, questions ... I'm looking forward to hearing from 
you.

Regards,

P. De Visschere

- there are 4 functions where the textwidth must be calculated: in 
GuiPainter::text() and in 3 functions in the TextMetrics class, in order 
of complexity:
(1) cursorX(): find the position in pixels, given the cursor position in 
characters,
(2) rowBreakPoint(): find the next breakpoint of a row, this will always 
break between words,
(3) getColumnNearX(): actually the reverse operation of (1); more 
complex than (2) since the position can also be located within a word.

For the time being these 3 functions are still independent, although 
they are somewhat similar and maybe some more streamlining is possible 
here. I'm also aware that the code for getColumnNearX() is rather 
complex.

The widths are calculated in a single TextMetrics::textWidth() function 
which accepts a parameter to cache the result or not (e.g. widths of 
partial words needed when positioning the cursor within a word are not 
cached).

The widths calculated are cached in a std::map. I've 
also tried QHash but since docstring has no qhash function all strings 
must then be converted to QStrings and there is no speed gain.

I use only one map, the fonts are coded as a string of 4 characters 
(family, series, shape, size) which is then used as a prefix for the 
key. I have not tried alternatives like using a map for each used font.

The map itself is stored in the BufferView class; In this way there is 
one map for each document; when storing the map in the TextMetrics 
class, multiple maps are generated. I have only tested this with simple 
documents (no child documents Š).

When typing it's unavoidable to generate all partials of a word; these 
are removed again from the map so that only the final word remains; 
However nothing is done to remedy the reverse: when deleting a word 
character by character all partials will end-up in the map;

The old code is still in place and the old painting character by 
character can be restored by changing 2 appropriate macro definitions.

begin 644 LyXscrollpatch20130421.diff
M9`M+6=I="!A+W-R8R]"=69F97)6:65W+F-P<"!B+W-R8R]"=69F97)6
M:65W+F-P<`II;F1E>"`S86$R,#%B+BYF,S0X,F%D(#$P,#8T-`HM+2T@82]S
M