#15 Fix leaky memory usage after some specific text operations

for 4.3.x
closed-accepted
None
5
2010-04-03
2010-03-19
No

r17499
http://jedit.svn.sourceforge.net/jedit/?view=rev&rev=17499

The leaky memory usage can be noticeable when it happens for very large
buffer, and remains after closing the buffer, until one of those
specific operations on textArea get invoked.

This is partial revertion of r8287.
http://jedit.svn.sourceforge.net/jedit/?view=rev&rev=8287
I believe this can't cause performance loss. The fix can be also good
for threading issues.
http://www.ibm.com/developerworks/java/library/j-jtp01274.html#3.1
The fix had been tested in my local for some months.

Discussion

  • Björn Kautler

    Björn Kautler - 2010-04-02

    Do you really think there is a threading issue? Aren't those methods always called from the same thread?

    I would like to make a counter proposal that doesn't eliminate the shared object approach as e. g. the doWordWrap() method is called on every character the user types if hard-wrap is enabled. You say you tested your version for months, but do you use hard-wrapping where this probably is a performance leak?

    I attached two patches against current trunk@HEAD. One is a thread-unsafe version, one is a thread-safe version.
    What do you think?

     
  • Kazutoshi Satoda

    > Do you really think there is a threading issue? ...

    I don't want to examine that and also don't want to require such
    implicit condition for future changes to achieve doubtful performance
    gain.

    > I would like to make a counter proposal that doesn't eliminate the shared
    > object ...
    (snip)
    > ... do you use hard-wrapping where this probably is a
    > performance leak?

    I don't use hard-wrapping, but I can still believe shared object is not
    worth having for that. Allocation time should be in far smaller order
    (a few micro-seconds badly estimated) than the order of time for user
    input (a few milli-seconds for super typists).

    Here is a quote from the link I put on the initial comment.
    "Sun estimates allocation costs at approximately ten machine
    instructions."
    And the article is quite old, at 2004. We have more improved JVM now.

    > I attached two patches ...

    Explicit nulling is another crime on the article. And I agree on that.
    Thus, I don't want to get these patches.

     
  • Björn Kautler

    Björn Kautler - 2010-04-03

    You don't have to consider only the allocation time. There is one object created for each character entered into the buffer. All these object also have to be garbage collected later, ...
    My point is that we are not sure whether this shared object maybe was introduced to fix a performance or memory problem or if it was just a bad pattern used. If it really was just a bad patern used, I would be happy to use your patch.

    I didn't commit the explicit nulling crime in my patches. The crime is about unnecessary explicit nulling. In my patch it is necessary explicit nulling to prevent the memory leak, so this is not a commiting of the there-described crime.

     
  • Björn Kautler

    Björn Kautler - 2010-04-03
    • assigned_to: nobody --> vampire0
    • status: open --> closed-accepted
     
  • Kazutoshi Satoda

    > There is one object created for each character entered into the buffer.

    There are more objects created for the event; key event object,
    editbus message, all required to repaint, ... etc. One another object
    must not cause a problem.

    > ... we are not sure whether this shared object maybe was introduced to
    > fix a performance or memory problem ...

    Sorry, I failed to show this in the initial comment. r8287 is not the
    revision which introduced this shared object.

    Now I re-examined it and found the shared object at r3791 (2001/09/02),
    which is the initial revision of JEditTextArea.
    http://jedit.svn.sourceforge.net/viewvc/jedit/jEdit/trunk/org/gjt/sp/jedit/textarea/JEditTextArea.java?revision=3791&view=markup

    The object was later moved into TextArea at r7156 literally. r8287 was
    just a minor change to make it final and initialize at the declaration,
    which I wrongly find that was a introduction of the object with
    svn blame.

    > I didn't commit the explicit nulling crime in my patches.

    Hmm, you are right. "Explicit nulling" in the article is a different
    story. Sorry, I didn't read it closely. The explicit assignment in the
    patch is just a part of drawbacks of object pooling.

     
  • Björn Kautler

    Björn Kautler - 2010-04-06

    > There are more objects created for the event; key event object,
    > editbus message, all required to repaint, ... etc. One another object
    > must not cause a problem.

    Of course there are other objects created, but never know whether one mor could cause a problem or not. You can at most say "should not" but not "must not". Besides that, in german I would say "flock also produce droppings" but I don't know how to translate this phrase to good english.

    > Now I re-examined it and found the shared object at r3791 (2001/09/02),
    > which is the initial revision of JEditTextArea.
    > http://jedit.svn.sourceforge.net/viewvc/jedit/jEdit/trunk/org/gjt/sp/jedit/textarea/JEditTextArea.java?revision=3791&view=markup

    This is only partly true. This is the initial revision of JEditTextArea in SVN as the former history was not imported to SVN but only a snapshot when Slava moved from GJT to SF. This shared object was introduced in 1.11 in GJT CVS at 1999-07-08 06:06:04 if you want to know it exactly. :-) But also there it doesn't seem from first look that it was introduced to improve performance but there were some structural changes whereby this was introduced.

    > The object was later moved into TextArea at r7156 literally.

    Yup, this was part of the effort to make the independent TextArea package to be embedded in other Java apps.

    However, those findings I did caused me to accept your patch. It is already in the release branch since I closed this artifact. ;-)

     

Log in to post a comment.