You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@openjpa.apache.org by "Fernando (JIRA)" <ji...@apache.org> on 2008/12/13 02:10:44 UTC
[jira] Created: (OPENJPA-826) moving some code outside of lock
boundaries, that don't need to be locked
moving some code outside of lock boundaries, that don't need to be locked
-------------------------------------------------------------------------
Key: OPENJPA-826
URL: https://issues.apache.org/jira/browse/OPENJPA-826
Project: OpenJPA
Issue Type: Improvement
Components: kernel
Affects Versions: 2.0.0
Reporter: Fernando
I am reviewing code because of slices concurrency/multithreaded issues, and I see that some code in QueryImpl was too aggressive in their locks. So I reviewed QueryImpl and BrokerImpl...
Mainly: assertMethods do not need to be within the lock.
Then I found a few methods that were doing precondition/shortcut checking, but all within locks, so that seemed like a waste:
1) QueryImpl.setCandiateExtent; checks to see if value setting is same as current value
2) these places check to see if it needs to calculate the value of not:
QueryImpl.isUnique, QueryImpl.setCandiateExtent, QueryImpl.getCandidateType, QueryImpl.getResultType, QueryImpl.getParameterDeclaration
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-826) moving some code outside of lock
boundaries, that don't need to be locked
Posted by "David Jencks (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-826?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12656263#action_12656263 ]
David Jencks commented on OPENJPA-826:
--------------------------------------
These changes look to me like they introduce serious thread safety issues. For instance with this snippet:
public Class getResultType() {
+ assertOpen();
+ if (_resultClass != null || _compiled != null || _query == null
+ || _broker == null)
+ return _resultClass;
+
lock();
try {
- assertOpen();
- if (_resultClass != null || _compiled != null || _query == null
- || _broker == null)
- return _resultClass;
-
// check again after compilation; maybe encoded in string
compileForCompilation();
return _resultClass;
_resultClass must be volatile in order to assure that it is a completely constructed object, and an arbitrarily large number of threads can get into the locked block and call compileForCompilation which I assume sets _resultClass.
You might be able to fix it with code something like this, assuming the assertions are thread safe:
private volatile Class _resultClass;
...
public Class getResultType() {
assertOpen();
if (_resultClass == null) {
lock();
if (_resultClass == null) {
compileForCompilation();
}
unlock();
}
return _resultClass;
}
(exception handling and some of the conditions left out). This will not work unless the varlable is volatile, as unlocked threads could then read a partially initialized object. Google for double-checked locking for more details.
> moving some code outside of lock boundaries, that don't need to be locked
> -------------------------------------------------------------------------
>
> Key: OPENJPA-826
> URL: https://issues.apache.org/jira/browse/OPENJPA-826
> Project: OpenJPA
> Issue Type: Improvement
> Components: kernel
> Affects Versions: 2.0.0
> Reporter: Fernando
> Attachments: locks-asserts.diff
>
>
> I am reviewing code because of slices concurrency/multithreaded issues, and I see that some code in QueryImpl was too aggressive in their locks. So I reviewed QueryImpl and BrokerImpl...
> Mainly: assertMethods do not need to be within the lock.
> Then I found a few methods that were doing precondition/shortcut checking, but all within locks, so that seemed like a waste:
> 1) QueryImpl.setCandiateExtent; checks to see if value setting is same as current value
> 2) these places check to see if it needs to calculate the value of not:
> QueryImpl.isUnique, QueryImpl.setCandiateExtent, QueryImpl.getCandidateType, QueryImpl.getResultType, QueryImpl.getParameterDeclaration
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (OPENJPA-826) moving some code outside of lock
boundaries, that don't need to be locked
Posted by "Fernando (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-826?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Fernando updated OPENJPA-826:
-----------------------------
Attachment: locks-asserts.diff
patch to move around some code outside of locks, to make multithreaded support more efficient.
> moving some code outside of lock boundaries, that don't need to be locked
> -------------------------------------------------------------------------
>
> Key: OPENJPA-826
> URL: https://issues.apache.org/jira/browse/OPENJPA-826
> Project: OpenJPA
> Issue Type: Improvement
> Components: kernel
> Affects Versions: 2.0.0
> Reporter: Fernando
> Attachments: locks-asserts.diff
>
>
> I am reviewing code because of slices concurrency/multithreaded issues, and I see that some code in QueryImpl was too aggressive in their locks. So I reviewed QueryImpl and BrokerImpl...
> Mainly: assertMethods do not need to be within the lock.
> Then I found a few methods that were doing precondition/shortcut checking, but all within locks, so that seemed like a waste:
> 1) QueryImpl.setCandiateExtent; checks to see if value setting is same as current value
> 2) these places check to see if it needs to calculate the value of not:
> QueryImpl.isUnique, QueryImpl.setCandiateExtent, QueryImpl.getCandidateType, QueryImpl.getResultType, QueryImpl.getParameterDeclaration
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (OPENJPA-826) moving some code outside of lock
boundaries, that don't need to be locked
Posted by "Pinaki Poddar (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-826?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Pinaki Poddar updated OPENJPA-826:
----------------------------------
Component/s: (was: kernel)
slice
> moving some code outside of lock boundaries, that don't need to be locked
> -------------------------------------------------------------------------
>
> Key: OPENJPA-826
> URL: https://issues.apache.org/jira/browse/OPENJPA-826
> Project: OpenJPA
> Issue Type: Improvement
> Components: slice
> Affects Versions: 2.0.0
> Reporter: Fernando
> Attachments: locks-asserts-2.diff, locks-asserts.diff
>
>
> I am reviewing code because of slices concurrency/multithreaded issues, and I see that some code in QueryImpl was too aggressive in their locks. So I reviewed QueryImpl and BrokerImpl...
> Mainly: assertMethods do not need to be within the lock.
> Then I found a few methods that were doing precondition/shortcut checking, but all within locks, so that seemed like a waste:
> 1) QueryImpl.setCandiateExtent; checks to see if value setting is same as current value
> 2) these places check to see if it needs to calculate the value of not:
> QueryImpl.isUnique, QueryImpl.setCandiateExtent, QueryImpl.getCandidateType, QueryImpl.getResultType, QueryImpl.getParameterDeclaration
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (OPENJPA-826) moving some code outside of lock
boundaries, that don't need to be locked
Posted by "Fernando (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-826?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Fernando updated OPENJPA-826:
-----------------------------
Attachment: locks-asserts-2.diff
this is a second patch, which only moves the asserts out of the lock.. and puts back some of the more convoluted null checks..
this one should be more palatable, and should be included.
> moving some code outside of lock boundaries, that don't need to be locked
> -------------------------------------------------------------------------
>
> Key: OPENJPA-826
> URL: https://issues.apache.org/jira/browse/OPENJPA-826
> Project: OpenJPA
> Issue Type: Improvement
> Components: kernel
> Affects Versions: 2.0.0
> Reporter: Fernando
> Attachments: locks-asserts-2.diff, locks-asserts.diff
>
>
> I am reviewing code because of slices concurrency/multithreaded issues, and I see that some code in QueryImpl was too aggressive in their locks. So I reviewed QueryImpl and BrokerImpl...
> Mainly: assertMethods do not need to be within the lock.
> Then I found a few methods that were doing precondition/shortcut checking, but all within locks, so that seemed like a waste:
> 1) QueryImpl.setCandiateExtent; checks to see if value setting is same as current value
> 2) these places check to see if it needs to calculate the value of not:
> QueryImpl.isUnique, QueryImpl.setCandiateExtent, QueryImpl.getCandidateType, QueryImpl.getResultType, QueryImpl.getParameterDeclaration
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-826) moving some code outside of lock
boundaries, that don't need to be locked
Posted by "Fernando (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-826?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12656693#action_12656693 ]
Fernando commented on OPENJPA-826:
----------------------------------
OK. So maybe I was being a little overzealous. but I'm pretty sure, all of the assertOpen/NotReadOnly/NotSerialized methods can go outside the lock.
for the few places where I also moved out the null checks, I guess we should double them up to be extra sure..
Would you like me to submit a different patch that only moves out the asserts?
Another patch I would definitely add is to add the locks inside of the compilation methods, instead of putting locks every the compilation methods are called.. then within the compilation method I would add a short-circuit, that would simply exit if its already been compiled.. Then a lot of the boilerplate code would go away..
> moving some code outside of lock boundaries, that don't need to be locked
> -------------------------------------------------------------------------
>
> Key: OPENJPA-826
> URL: https://issues.apache.org/jira/browse/OPENJPA-826
> Project: OpenJPA
> Issue Type: Improvement
> Components: kernel
> Affects Versions: 2.0.0
> Reporter: Fernando
> Attachments: locks-asserts.diff
>
>
> I am reviewing code because of slices concurrency/multithreaded issues, and I see that some code in QueryImpl was too aggressive in their locks. So I reviewed QueryImpl and BrokerImpl...
> Mainly: assertMethods do not need to be within the lock.
> Then I found a few methods that were doing precondition/shortcut checking, but all within locks, so that seemed like a waste:
> 1) QueryImpl.setCandiateExtent; checks to see if value setting is same as current value
> 2) these places check to see if it needs to calculate the value of not:
> QueryImpl.isUnique, QueryImpl.setCandiateExtent, QueryImpl.getCandidateType, QueryImpl.getResultType, QueryImpl.getParameterDeclaration
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.