#425 Correctly unref old objects in UndoManager.addEdit()


While implementing an limit for compundEdit objects I did copy the unref code from UndoManager.addEdit(). But I still got OOM error with the new code inplace. The solution was to fix the existing unref code.


  • Jarek Czekalski

    Jarek Czekalski - 2012-05-21

    Thomas, I think you're wrong here. Please consider a simple example, a list consisting of elements:
    A, B, C
    A is referenced only by B and by HEAD. Original unref code removes both references to A. It becomes free and GC may finalize it.

    The change you did is that after consecutive removal of list elements, each element is dereferenced. Before your change, only one of the removed elements was dereferenced. But after finalizing that one, another one got dereferenced. And so on, chain reaction.

    However I see GC rarely finalizes Edit-s. For example: I do 1000 replaces, run GC (through clicking jedit memory indicator, I believe it does run GC). Then undo. I see no finalizes. Then I do a single edit and undo. GC. Only now it finalizes. It works the same with and without your patch.

    I attach a patch to track edits.

    So I would like to see additional justification for this change. Theoretically it does not do anything meaningful. Please correct me if I'm wrong.

  • Jarek Czekalski

    Jarek Czekalski - 2012-05-21

    -p1 debugging patch

  • Thomas Meyer

    Thomas Meyer - 2012-05-22

    Hi Jarek,

    yes, "A" becomes free, but GC doesn't seem to remove it! "A" still holds a reference to "B" in it's next field. Maybe this is the reason for the GC not to destoy the "A" object? But I'm not sure about the reasons. I theory it should get removed, shouldn't it?

    You can easily reproduce the high memory usage of UndoManager$Insert and UndoManager$Remove objects with attached new patch. Just grab my search-and-replace-all test file and replace "," with ", " (note the extra space) and sample the memory usage with VisualVM. You will see that both classes (Insert&Remove) will hold about 90MB of memory, and won't get GC'ed, i.e. the undo/redo limit is not being obeyed!

    Setting the next field of "A" to null will remove this object from the heap and after that only 100 UndoManager$Insert and UndoManager$Remove objects will remain in the memory sampling as it should be.

    PS1: Calling System.gc() doesn't guarantee anything in regard of object removal.
    PS2: be aware that consecutive replaces (in terms of offset in the JEditBuffer) are collapsed into on one Edit object, in contentRemoved() resp. contentInserted().

  • Jarek Czekalski

    Jarek Czekalski - 2012-05-23

    Hi Thomas

    Why do you expect to remove the objects from memory, when they are needed for the redo?

    1. GC and check jedit memory. Say 15MB.
    2. Do huge s&r, memory usage jumps to say 30MB
    3. Undo. Memory still 30MB. Notice you may do "redo".
    4. Type something. This shoud clean the redo buffer. Memory 15MB.

    This is all without your patch. I see no bug and no need to correct any unreffing. No bug in java also, all works as expected, as in theory :)

    The problem here is that you fix a bug that you have not reported. This is difficult for me. Please submit a bug report first. If there is a memory leak, the additional tools are needed to reproduce. But if it is a functionality issue, we don't need visual vm.

    This patch says that the unref is not done correctly. It looks like it's not true, so I'm closing it. No problem to reopen it after a bug is detected and confirmed.

  • Jarek Czekalski

    Jarek Czekalski - 2012-05-23
    • assigned_to: nobody --> jarekczek
    • status: open --> closed-invalid

Log in to post a comment.