#534 Avoid buffer copying where possible

release
closed-fixed
Tony Balinski
Program (402)
9
2006-11-20
2006-07-26
Tony Balinski
No

This improves performance and memory footprint,
particularly when using NEdit to view very large files.
It replaces calls to BufGetAll() with BufAsString().
Where the full buffer text is required in a read-only
context, instead of making a string copy of it, just
use its content directly (having made sure the extra
gap space does not appear inside this content).

See also patch

http://sourceforge.net/tracker/index.php?func=detail&aid=1191453&group_id=11005&atid=311005
1191453 Avoiding buffer copying in macro search

which introduced BufAsString(). It was added to the
NEdit source on 2006-Apr-21.

This patch was created following comments about
suspected malloc errors by Jeno Balasko (discuss list,
2006-Jun-1, see
http://www.nedit.org/pipermail/discuss/2006-June/008385.html
and
http://www.nedit.org/pipermail/develop/2006-June/011958.html
for more).

(I've been using this patch for some time and it
appears pretty solid.)

Discussion

  • Tony Balinski
    Tony Balinski
    2006-07-26

     
    Attachments
  • Logged In: NO

    Well, this is likely to introduce bugs as well as fix the performance bug. I'd suggest that we really review this carefully, to make sure it doesn't have any problems. I don't know the code well enough to say, but I can definitely see possible problems when making replacements from the actual buffer instead of a copy.

    Maybe you know more about this than I do and can vouch for it. I'd just like something stronger than "I've been using it and it seems to work".

     
  • Thorsten Haude
    Thorsten Haude
    2006-10-02

    Logged In: YES
    user_id=119143

    Is this the complete patch or is #1191453 required? If the
    latter, could you create a complete patch?

     
  • Tony Balinski
    Tony Balinski
    2006-10-02

    Logged In: YES
    user_id=618141

    #1191453 was integrated into CVS head on 2006-Apr-21. This
    is an addition to it.

    Essentially, what I did was to examine all calls of
    BufGetAll() to see if they could be replaced with
    BufAsString(). I am personally sure that (a) I have
    converted as many such calls as I can without substantial
    rewriting; (b) I have not introduced a bug whereby the
    result of the changes makes the buffer modifiable in an
    uncontrolled way; (c) I have made the code generally more
    explicit by adding const to various function prototypes
    (which gives me confidence in (b)), and (d) I have not added
    memory leaks.

    However, nobody is immune to error, even me, which is why I
    ask that this patch be reviewed by other eyes. So far I have
    received no complaints regarding the earlier patch, 1191453,
    so I suppose folks are happy with that too.

    I have noted that the patch on SF requires updating for the
    current CVS, so please use the one I am uploading now.

     
  • Tony Balinski
    Tony Balinski
    2006-10-02

    updated for changes in CVS head

     
    Attachments
  • Thorsten Haude
    Thorsten Haude
    2006-10-02

    Logged In: YES
    user_id=119143

    Ok, the mention of the other patch confused me, I wasn't
    aware that it is already committed.

    About that other patch, in the comment for BufAsString():

    >** NB DO NOT ALTER THE TEXT THROUGH THE RETURNED POINTER!
    I think this patch does just that, doesn't it?

    >** This function should really return const char *.
    Why doesn't it?

     
  • Thorsten Haude
    Thorsten Haude
    2006-10-02

    Logged In: YES
    user_id=119143

    One other thing: Adding copious amounts of fprintf()s (to be
    removed before a release) might help to find any bugs. This
    *IS* a development branch.

     
  • Tony Balinski
    Tony Balinski
    2006-10-02

    Logged In: YES
    user_id=618141

    Thanks for looking at the patch, Thorsten. I wish others
    would too! Anybody?

    Indeed, this patch does not (AFAICT) modify the returned
    buffer, except where the null character is substituted for
    something else. This is, I believe, the only place where the
    returned buffer is modified - legitimately - and the only
    real reason for not returning a const. The only other places
    where, maybe, casts should be used are in macro.c
    (searchMS()), where a buffer pointer is copied into a
    DataValue (for a later non-modifying search), and in
    textBuf.c (BufSetTabDistance()) for a redisplay of all text
    to account for changed tab settings, where it's passed as
    "deleted text". I couldn't find a callback which required
    this to be non-const, but I decided this was rather a big
    deal to change. Maybe I should just go ahead anyway.

    As for fprintf()s, I rarely use them. Crashes usually give
    me enough information! But that's just me...

     
  • Thorsten Haude
    Thorsten Haude
    2006-10-03

    Logged In: YES
    user_id=119143

    Nope, no modification I can see, I missed something, sorry.

    As for the printf()s, they could also be assert()s or
    something else you are fond of. The point is that we can use
    more tools in development than in release; NEdit always
    tries to keep clean, even when it needn't.

     
  • Thorsten Haude
    Thorsten Haude
    2006-10-06

    • labels: 376535 --> Program
     
  • Thorsten Haude
    Thorsten Haude
    2006-10-06

    • priority: 5 --> 9
    • milestone: --> release
    • status: open --> open-accepted
     
  • Thorsten Haude
    Thorsten Haude
    2006-10-06

    • assigned_to: nobody --> ajbj
     
  • Tony Balinski
    Tony Balinski
    2006-10-11

    adds extra consts

     
    Attachments
  • Tony Balinski
    Tony Balinski
    2006-10-11

    Logged In: YES
    user_id=618141

    The new patch changes BufAsString() to return const char *,
    and consequently changes result pointers similarly,
    including the prototype for bufModifyCallbackPr, which
    results in many const-adding modifications.

    The constness of BufAsString()'s return value is cast away
    twice: once in textBuf.c: BufSubstituteNullChars(), where
    the buffer text is changed (in a safe way), and once in
    macro.c: searchMS(), where the buffer text pointer is
    temporarily stored in a DataValue for searching in
    searchStringMS() - the original motivation for BufAsString()
    in fact, which does not change the buffer text either.

     
  • Tony Balinski
    Tony Balinski
    2006-10-13

    Logged In: YES
    user_id=618141

    I have just committed the changes in fasterSearch4.diff.
    I'll leave the bug open for now, just in case anyone has issues.

     
  • Tony Balinski
    Tony Balinski
    2006-11-20

    • status: open-accepted --> closed-fixed
     
  • Tony Balinski
    Tony Balinski
    2006-11-20

    Logged In: YES
    user_id=618141
    Originator: YES

    Nobody's been screaming about this, so I'm marking it fixed and closed.