#123 Remove synchronize in SimpleLRUCache

v1.3
open
momo
None
1
2014-10-29
2013-05-03
Jonny Boman
No

Optimize suggestion to remove synchronize for SimpleLRUCache, in order to get better throughput in concurrent environments.
I attach the modified SimpleLRUCache and SQLParser files, but in essence I just removed synchronized in favor of a java.util.concurrent alternative.

/J++

1 Attachments

Related

Patches: #123

Discussion

  • Jonny Boman

    Jonny Boman - 2013-05-03

    I have used this patch a loong time in our own production environment so I am pretty comfortable with it.

     
  • momo

    momo - 2013-05-03
    • status: open --> closed
    • assigned_to: momo
     
  • momo

    momo - 2013-05-03

    Thank you for providing this patch! I just had a look but unfortunately the proposed changes wouldn't be correct.

    1) You removed synchronization from SimpleLRUCache but this way you are accessing the backing HashMap concurrently, which may lead to all kind of misbehavior (and even dead-lock a calling thread).

    2) You implemented a double-check locking approach in SQLParser, which will never be save in Java (just search Google for something like "java double-checked locking broken").

    But you are right, synchronization in both classes may impact performance and should be avoided. I completely removed synchronization from SQLParser (the previous code wasn't correct anyway) and improved and simplified the SimpleLRUCache implementation. Nevertheless, the cache still requires synchronization but I didn't have the time to put together a lock-free cache. Maybe you could try my changes and give me some feedback how it performs in your application/environment, compared to the original jTDS 1.3.0 and compared to your patched version - just to get an idea how urgent it is to get a lock-free LRU cache implementation.

    Cheers,
    momo

     
  • Comment has been marked as spam. 
    Undo

    You can see all pending comments posted by this user  here

    Anonymous - 2013-05-06

    About 1) I made a typo in the attachment because I actually extended ConcurrentHashMap instead of HashMap. Wouldn't that do the trick?

    J++

    Från: momo [mailto:ickzon@users.sf.net]
    Skickat: den 3 maj 2013 15:25
    Till: [jtds:patches]
    Ämne: [jtds:patches] #123 Remove synchronize in SimpleLRUCache

    Thank you for providing this patch! I just had a look but unfortunately the proposed changes wouldn't be correct.

    1) You removed synchronization from SimpleLRUCache but this way you are accessing the backing HashMap concurrently, which may lead to all kind of misbehavior (and even dead-lock a calling thread).

    2) You implemented a double-check locking approach in SQLParser, which will never be save in Java (just search Google for something like "java double-checked locking broken").

    But you are right, synchronization in both classes may impact performance and should be avoided. I completely removed synchronization from SQLParser (the previous code wasn't correct anyway) and improved and simplified the SimpleLRUCache implementation. Nevertheless, the cache still requires synchronization but I didn't have the time to put together a lock-free cache. Maybe you could try my changes and give me some feedback how it performs in your application/environment, compared to the original jTDS 1.3.0 and compared to your patched version - just to get an idea how urgent it is to get a lock-free LRU cache implementation.

    Cheers,
    momo


    [patches:#123]http://sourceforge.net/p/jtds/patches/123/ Remove synchronize in SimpleLRUCache

    Status: closed
    Created: Fri May 03, 2013 06:36 AM UTC by Jonny Boman
    Last Updated: Fri May 03, 2013 06:38 AM UTC
    Owner: momo

    Optimize suggestion to remove synchronize for SimpleLRUCache, in order to get better throughput in concurrent environments.
    I attach the modified SimpleLRUCache and SQLParser files, but in essence I just removed synchronized in favor of a java.util.concurrent alternative.

    /J++


    Sent from sourceforge.net because you indicated interest in https://sourceforge.net/p/jtds/patches/123/

    To unsubscribe from further messages, please visit https://sourceforge.net/auth/subscriptions/

     

    Related

    Patches: #123

  • Comment has been marked as spam. 
    Undo

    You can see all pending comments posted by this user  here

    Anonymous - 2013-05-06

    You're absolutely right! I must have been blind :)
    I will try the latest version and see...

    J++

    Från: momo [mailto:ickzon@users.sf.net]
    Skickat: den 3 maj 2013 15:25
    Till: [jtds:patches]
    Ämne: [jtds:patches] #123 Remove synchronize in SimpleLRUCache

    Thank you for providing this patch! I just had a look but unfortunately the proposed changes wouldn't be correct.

    1) You removed synchronization from SimpleLRUCache but this way you are accessing the backing HashMap concurrently, which may lead to all kind of misbehavior (and even dead-lock a calling thread).

    2) You implemented a double-check locking approach in SQLParser, which will never be save in Java (just search Google for something like "java double-checked locking broken").

    But you are right, synchronization in both classes may impact performance and should be avoided. I completely removed synchronization from SQLParser (the previous code wasn't correct anyway) and improved and simplified the SimpleLRUCache implementation. Nevertheless, the cache still requires synchronization but I didn't have the time to put together a lock-free cache. Maybe you could try my changes and give me some feedback how it performs in your application/environment, compared to the original jTDS 1.3.0 and compared to your patched version - just to get an idea how urgent it is to get a lock-free LRU cache implementation.

    Cheers,
    momo


    [patches:#123]http://sourceforge.net/p/jtds/patches/123/ Remove synchronize in SimpleLRUCache

    Status: closed
    Created: Fri May 03, 2013 06:36 AM UTC by Jonny Boman
    Last Updated: Fri May 03, 2013 06:38 AM UTC
    Owner: momo

    Optimize suggestion to remove synchronize for SimpleLRUCache, in order to get better throughput in concurrent environments.
    I attach the modified SimpleLRUCache and SQLParser files, but in essence I just removed synchronized in favor of a java.util.concurrent alternative.

    /J++


    Sent from sourceforge.net because you indicated interest in https://sourceforge.net/p/jtds/patches/123/

    To unsubscribe from further messages, please visit https://sourceforge.net/auth/subscriptions/

     

    Related

    Patches: #123

  • Jonny Boman

    Jonny Boman - 2013-05-07

    New try, and perhaps a bit more correct :)

    SimpleLRUCache could occasionally get instantiated unnecessarily but that should not be a problem I think...

     
    Last edit: Jonny Boman 2013-05-07
  • momo

    momo - 2013-05-07
    • status: closed --> open
     
  • Comment has been marked as spam. 
    Undo

    You can see all pending comments posted by this user  here

    Anonymous - 2014-10-29

    Is this fixed in JTDS 1.3.1? I still see the same issue. Please advise.

     


Anonymous

Cancel  Add attachments





Get latest updates about Open Source Projects, Conferences and News.

Sign up for the SourceForge newsletter:





No, thanks