SourceForge has been redesigned. Learn more.
Close

#448 HyperSearchRequest should wait for IO to finish

closed-accepted
None
5
2012-08-25
2012-08-16
No

The current implementation (HyperSearchRequest._run():140) works correctly, because of the optimizations in VFS.load():496 and Buffer.load():302 which let the BufferIORequest for a temporary buffer run on the current thread, which results in a natural serialization of both actions.
This is a rather fragile implementation. Fix it by waiting for the IO to finish.

Other callers of jEdit.openTemporary() already do this.

Discussion

  • Alan Ezust

    Alan Ezust - 2012-08-22
    • assigned_to: nobody --> ezust
    • status: open --> closed-accepted
     
  • Alan Ezust

    Alan Ezust - 2012-08-22

    Committed 22043

     
  • Jarek Czekalski

    Jarek Czekalski - 2012-08-23

    So this is a fix for something that works correctly. I wonder how much slower will hypersearch run after this fix is applied. Hypersearch, opening thousands of files, that should run fastest possible. And this new io synchronization enters EDT thread, which is time expensive.

    >Other callers of jEdit.openTemporary() already do this.

    Maybe they are wrong.

    Alan, did you consider all these factors before committing the patch? Especially the rationale for correcting something that works, what was reported from the very beginning.

     
  • Thomas Meyer

    Thomas Meyer - 2012-08-23

    Hi Jarek,

    as temporary load requests are directly run on the requesting thread, the buffer will be loaded at this point in time and so the call to buffer.isLoaded() will return true, and the call to VFSManager.waitForRequests() will not even happen.

    The patch should have no impact in the runtime of the hypersearch.

    And yes, Alan is very aggressive in commiting my patches. I'm not sure if all of these get the review the definitely need!

     
  • Jarek Czekalski

    Jarek Czekalski - 2012-08-23

    Hi Thomas. Thanks for the explanation. No performance problem indeed, so my greatest concern is void. Sorry guys.

     
  • Jarek Czekalski

    Jarek Czekalski - 2012-08-24

    You didn't say that you want it for the purpose of changing openTemporary behaviour. That was misleading.

    Knowing that I have one more comment:
    There are pieces of code out in the wild that use openTemporary and they don't know it's necessary to do something else, like extra synchronization. So the statement "Other callers of jEdit.openTemporary() already do this" is not true, regarding 3rd party code. And I suggest not to break openTemporary function, because it's jedit fundamental routine.

    The workpool patches are close to being committed and I haven't seen any announcement on jedit-devel like that: "openTemporary changes". Now I'm afraid that more changes are done in the background, without appropriate information to all the developers. Please pay great attention to all api changes and implementation changes that require api changes.

     
  • Jarek Czekalski

    Jarek Czekalski - 2012-08-24

    And one more question. If there is a change done in openTemporary, how about the performance issue I was wrongly afraid of? May I be calm that the performance issue is still void after the change?

     
  • Thomas Meyer

    Thomas Meyer - 2012-08-25

    Hi,

    okay, how about documenting the behaviour of Buffer.load() then more precisely? I attached an patch to do that. what do you think about this?

    and no, I don't plan to change behaviour here, nothing to worry about, albeit the serial loading of all open buffers in PerspectiveManager.PerspectiveHandler.endElement() in the case of an autorestore at startup is a bit annoying...

     
  • Jarek Czekalski

    Jarek Czekalski - 2012-08-25

    It's a good idea to document that. Please post it as a separate ticket and correct 2 things:
    1. isLoaded *is* thread safe - no need to change the javadoc here
    2. whether or not load is thread safe depends on flags or maybe not, I am not sure. Definitely "is not thread safe" cannot be said, because with TEMPORARY flag it actually is thread-safe. It's better to say nothing about it.
    You may assign the ticket to me.

     
  • Thomas Meyer

    Thomas Meyer - 2012-08-25

    "1. isLoaded *is* thread safe - no need to change the javadoc here"

    I think it's not thread-safe: Buffer.isLoaded() calls JEditBuffer.isLoading() which returns the private field "JEditBuffer.loading". The setter method and all readers of this fields are not synchronized. So this is field is not thread-safe.

    Looking at the call path to the field "JEditBuffer.loading" it seems that all access to it will happen in the EDT/AWT thread. So the access is safe because it's confined to the EDT thread.

    So if you would call Buffer.isLoaded() from another thread than the EDT thread, and while the value of the field "JEditBuffer.loading" was just changed in the EDT thread, this change wouldn't be visible to the non-EDT thread as no synchonisation did happen!

    So I'm in favour of highlighting the EDT confinement in the class description:

    Currently:

    " * A <code>JEditBuffer</code> represents the contents of an open text
    * file as it is maintained in the computer's memory (as opposed to
    * how it may be stored on a disk).<p>
    *
    * This class is partially thread-safe, however you must pay attention to two
    * very important guidelines:
    * <ul>
    * <li>Operations such as insert() and remove(),
    * undo(), change Buffer data in a writeLock(), and must
    * be called from the AWT thread.
    * <li>When accessing the buffer from another thread, you must
    * call readLock() before and readUnLock() after, if you plan on performing
    * more than one read, to ensure that the buffer contents are not changed by
    * the AWT thread for the duration of the lock. Only methods whose descriptions
    * specify thread safety can be invoked from other threads."

    My suggestion:

    " * A <code>JEditBuffer</code> represents the contents of an open text
    * file as it is maintained in the computer's memory (as opposed to
    * how it may be stored on a disk).<p>
    *
    * This class is thread-safe, because it's thread confined to
    * the Event Dispatch Thread.
    * These rules apply:
    * <ul>
    * <li>Operations such as insert() and remove(),
    * undo(), etc.(i.e. methods which change Buffer data in a writeLock())
    * and must be called from the EDT.
    * <li>When accessing the buffer from another thread, you must
    * call readLock() before and readUnLock() after the JEditBuffer method call
    * <li>When accessing the buffer from another thread,
    * readLock() and readUnLock() should be used to create compound actions
    * when necessary
    * <li>When accessing the buffer from another thread,
    * only methods whose descriptions specify a call-ability from non-EDT
    * must be called"

    Also I would like to replace the "This method is thread-safe" with "This method can safely be called from a non-EDT thread, with client-side readLock() held."

    What do you think about this idea?

     
  • Alan Ezust

    Alan Ezust - 2012-08-25

    This ticket was already closed and accepted. Please open a new ticket for a new patch.

     
  • Alan Ezust

    Alan Ezust - 2012-08-25
    • assigned_to: ezust --> thomasmey
     
  • Alan Ezust

    Alan Ezust - 2012-08-25

    ignore previous comment, I didn't see what was in the patch. If it's just API docs, you can commit that yourself!

     

Log in to post a comment.