Menu

#423 UndoManager fix memory usage

closed-rejected
nobody
None
5
2012-08-22
2012-05-19
No

Several fixes:
1.) Introduce CompoundLimit to limit the memory usage of large compound edits, e.g. search-replace-all
2.) contentInserted() + contentRemoved() - Try to use the same string object in all Edit objects. I'm not sure if the change for contentInserted() is necessary. The change for contentRemoved() is needed. The String passed to contentRemoved() is a new String for each call in the search-and-replace-all loop, but in my simple replacement case, has always the same content.
3.) addEdit() - un-reference the old object correctly, so garbage collection can remove it, see also CompoundEdit.add()

Discussion

  • Thomas Meyer

    Thomas Meyer - 2012-05-19
     
  • Jarek Czekalski

    Jarek Czekalski - 2012-05-19

    1. I am not sure, but I guess it's against jedit's desing, so it first needs discussion
    2. Java stores the strings in its internal hashtables. Theoretically 2 equal strings are stored only once. So the only memory waste is the string object, without the characters.
    3. Seems reasonable and needed.

    If you agree and would like to separate the point 3, I would review and apply.

     
  • Alan Ezust

    Alan Ezust - 2012-08-22

    1: hardcoded value for compoundlimit? Why 5000? should it be an option one can disable/change somewhere?
    Is that what jarek meant when he said "against jEdit design?" or just that there is a limit at all?
    3. Should this be a separate patch?

     
  • Alan Ezust

    Alan Ezust - 2012-08-22
    • assigned_to: nobody --> k_satoda
     
  • Thomas Meyer

    Thomas Meyer - 2012-08-22

    The memory usage for the UndoManager is fixed for the normal search case by introduction of a Replace Edit class and the compression of these Edits. There is one case where the memory usage is still high:
    Search and Replace all for a regular expression: In this case the search string can be different for each hit, so the compression wont work.
    I'm not sure how to proceed with the idea (the compound limit) introduced by this patch. Is it still worth to have it?
    Anyway as you said the patch is not finished and still has some rough edges like the hard coded limit.
    Point 2) is already fixed and point 3) is invalid.

    By the way: Jarkek found two bugs in the new implementation that reduce memory usage in the UndoManager and one already existing bug. I did provide patches to fix the newly introduced bugs. See:
    https://sourceforge.net/tracker/?func=detail&aid=3551910&group_id=588&atid=100588
    https://sourceforge.net/tracker/?func=detail&aid=3554476&group_id=588&atid=100588

    I hope somebody will take care to commit these two patches to the trunk!

    Should I open a new bug ticket for the already existing bug described in?
    https://sourceforge.net/tracker/?func=detail&aid=3554476&group_id=588&atid=100588

     
  • Alan Ezust

    Alan Ezust - 2012-08-22

    Those first links you have are to items in the "bugs" tracker that happen to have attachments that are patches. I didn't notice that until now. Which is why I was moving your tickets to patches when I noticed them, but jarek says I shouldn't do that anymore and I agree.

    I ask that for these tickets and future times, please submit patches as separate tickets in the "patches" tracker, so we can handle them in a more timely fashion. Add a comment "fix for tracker #xxxxxx".

     
  • Alan Ezust

    Alan Ezust - 2012-08-22

    let's reject this particular patch and you can re-submit a more polished one when it is ready.

     
  • Alan Ezust

    Alan Ezust - 2012-08-22
    • assigned_to: k_satoda --> nobody
    • status: open --> closed-rejected
     

Log in to post a comment.