#123 Remove synchronize in SimpleLRUCache

[Jonny Boman]

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++

[Jonny Boman]

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

[momo]

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

[Anonymous]

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/

[Anonymous]

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/

[Jonny Boman]

New try, and perhaps a bit more correct :)

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

[Anonymous]

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