You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@felix.apache.org by "Felix Meschberger (JIRA)" <ji...@apache.org> on 2007/08/15 07:53:30 UTC
[jira] Created: (FELIX-338) Framework FilterImpl is not thread safe
on execution
Framework FilterImpl is not thread safe on execution
----------------------------------------------------
Key: FELIX-338
URL: https://issues.apache.org/jira/browse/FELIX-338
Project: Felix
Issue Type: Bug
Components: Framework
Affects Versions: 0.8.0, 1.0.0
Reporter: Felix Meschberger
Executing a Filter implemented by the Felix Framework FilterImpl class is not thread safe, as the Filter.Implmatch() methods are not synchronized but use a Mapper and Evaluator instance fields which are modified while matching the filter.
Two options seem to exist:
1. Make the match() methods synchronized
2. Create Mapper and Evaluator instances on each match() call
My assumption is that the second method tends to be better because of the synchronization needed with the first approach.
Reported by Tom Remoleur, thanks. The full mail thread leading to this issue may be found at [1].
[1] http://www.mail-archive.com/users@felix.apache.org/msg00145.html
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (FELIX-338) Framework FilterImpl is not thread
safe on execution
Posted by "Karl Pauls (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/FELIX-338?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12520005 ]
Karl Pauls commented on FELIX-338:
----------------------------------
I agree with Richard, we should try to avoid code duplication and I like the fact that the LDAP package is self-contained.
Furthermore, I did re-run my tests and for me your solution doesn't help much. Still 35% overhead for both scenarios. The issue is the simple wrapper instance creations on every call - not the evaluator.
> Framework FilterImpl is not thread safe on execution
> ----------------------------------------------------
>
> Key: FELIX-338
> URL: https://issues.apache.org/jira/browse/FELIX-338
> Project: Felix
> Issue Type: Bug
> Components: Framework
> Affects Versions: 0.8.0, 1.0.0
> Reporter: Felix Meschberger
> Attachments: filter.patch, FilterImpl_rewrite.patch
>
>
> Executing a Filter implemented by the Felix Framework FilterImpl class is not thread safe, as the Filter.Implmatch() methods are not synchronized but use a Mapper and Evaluator instance fields which are modified while matching the filter.
> Two options seem to exist:
> 1. Make the match() methods synchronized
> 2. Create Mapper and Evaluator instances on each match() call
> My assumption is that the second method tends to be better because of the synchronization needed with the first approach.
> Reported by Tom Remoleur, thanks. The full mail thread leading to this issue may be found at [1].
> [1] http://www.mail-archive.com/users@felix.apache.org/msg00145.html
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Resolved: (FELIX-338) Framework FilterImpl is not thread
safe on execution
Posted by "Karl Pauls (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/FELIX-338?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Karl Pauls resolved FELIX-338.
------------------------------
Resolution: Fixed
The patch is in trunk as of r566323. Please try to test whether this makes your issue go away (or not appear in this case). I'll try to run the framework in the project I saw the same issue too. Hopefully, it should be gone now. I'll close this issue if I don't hear anything for some time.
> Framework FilterImpl is not thread safe on execution
> ----------------------------------------------------
>
> Key: FELIX-338
> URL: https://issues.apache.org/jira/browse/FELIX-338
> Project: Felix
> Issue Type: Bug
> Components: Framework
> Affects Versions: 0.8.0, 1.0.0
> Reporter: Felix Meschberger
> Assignee: Karl Pauls
> Attachments: filter.patch, FilterImpl_rewrite.patch
>
>
> Executing a Filter implemented by the Felix Framework FilterImpl class is not thread safe, as the Filter.Implmatch() methods are not synchronized but use a Mapper and Evaluator instance fields which are modified while matching the filter.
> Two options seem to exist:
> 1. Make the match() methods synchronized
> 2. Create Mapper and Evaluator instances on each match() call
> My assumption is that the second method tends to be better because of the synchronization needed with the first approach.
> Reported by Tom Remoleur, thanks. The full mail thread leading to this issue may be found at [1].
> [1] http://www.mail-archive.com/users@felix.apache.org/msg00145.html
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (FELIX-338) Framework FilterImpl is not thread
safe on execution
Posted by "Karl Pauls (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/FELIX-338?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12519960 ]
Karl Pauls commented on FELIX-338:
----------------------------------
Well, in my experience it never pays to jump to conclusions when Java
is involved. I actually wrote a quick test and see the following
(Apple, JVM 1.3 and 1.5).
syncing the match method adds ~ 5% overhead in a single thread scenario.
syncing the match method adds ~ 40% overhead in a multi thread scenario.
creating new instances adds ~ 40 % overhead in a single thread scenario.
creating new instances adds ~ 40 % overhead in a multi thread scenario.
In other words, the new instances approach doesn't seem that good to
me (your mileage may vary :-).
Fortunately, I wrote a solution that uses a ThreadLocal to only create
a new instance per Thread. I keep the instance in a SoftReference and
for my test the memory looks good to. This solution gives me:
ThreadLocal adds ~ 5% overhead in a single thread scenario
ThreadLocal adds ~ 5% overhead in a multi thread scenario
The downside is that it doesn't necessarily makes the code more pretty ;-).
> Framework FilterImpl is not thread safe on execution
> ----------------------------------------------------
>
> Key: FELIX-338
> URL: https://issues.apache.org/jira/browse/FELIX-338
> Project: Felix
> Issue Type: Bug
> Components: Framework
> Affects Versions: 0.8.0, 1.0.0
> Reporter: Felix Meschberger
>
> Executing a Filter implemented by the Felix Framework FilterImpl class is not thread safe, as the Filter.Implmatch() methods are not synchronized but use a Mapper and Evaluator instance fields which are modified while matching the filter.
> Two options seem to exist:
> 1. Make the match() methods synchronized
> 2. Create Mapper and Evaluator instances on each match() call
> My assumption is that the second method tends to be better because of the synchronization needed with the first approach.
> Reported by Tom Remoleur, thanks. The full mail thread leading to this issue may be found at [1].
> [1] http://www.mail-archive.com/users@felix.apache.org/msg00145.html
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (FELIX-338) Framework FilterImpl is not thread
safe on execution
Posted by "Felix Meschberger (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/FELIX-338?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12520069 ]
Felix Meschberger commented on FELIX-338:
-----------------------------------------
In this case, of course I agree with you to stick with your patch (on my Linux with Sun JDK 1.4, the differences are not that big, but then...). Will you apply it, or should I do it ?
> Framework FilterImpl is not thread safe on execution
> ----------------------------------------------------
>
> Key: FELIX-338
> URL: https://issues.apache.org/jira/browse/FELIX-338
> Project: Felix
> Issue Type: Bug
> Components: Framework
> Affects Versions: 0.8.0, 1.0.0
> Reporter: Felix Meschberger
> Attachments: filter.patch, FilterImpl_rewrite.patch
>
>
> Executing a Filter implemented by the Felix Framework FilterImpl class is not thread safe, as the Filter.Implmatch() methods are not synchronized but use a Mapper and Evaluator instance fields which are modified while matching the filter.
> Two options seem to exist:
> 1. Make the match() methods synchronized
> 2. Create Mapper and Evaluator instances on each match() call
> My assumption is that the second method tends to be better because of the synchronization needed with the first approach.
> Reported by Tom Remoleur, thanks. The full mail thread leading to this issue may be found at [1].
> [1] http://www.mail-archive.com/users@felix.apache.org/msg00145.html
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Closed: (FELIX-338) Framework FilterImpl is not thread safe
on execution
Posted by "Karl Pauls (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/FELIX-338?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Karl Pauls closed FELIX-338.
----------------------------
Seems to work now.
> Framework FilterImpl is not thread safe on execution
> ----------------------------------------------------
>
> Key: FELIX-338
> URL: https://issues.apache.org/jira/browse/FELIX-338
> Project: Felix
> Issue Type: Bug
> Components: Framework
> Affects Versions: 0.8.0, 1.0.0
> Reporter: Felix Meschberger
> Assignee: Karl Pauls
> Attachments: filter.patch, FilterImpl_rewrite.patch
>
>
> Executing a Filter implemented by the Felix Framework FilterImpl class is not thread safe, as the Filter.Implmatch() methods are not synchronized but use a Mapper and Evaluator instance fields which are modified while matching the filter.
> Two options seem to exist:
> 1. Make the match() methods synchronized
> 2. Create Mapper and Evaluator instances on each match() call
> My assumption is that the second method tends to be better because of the synchronization needed with the first approach.
> Reported by Tom Remoleur, thanks. The full mail thread leading to this issue may be found at [1].
> [1] http://www.mail-archive.com/users@felix.apache.org/msg00145.html
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Assigned: (FELIX-338) Framework FilterImpl is not thread
safe on execution
Posted by "Karl Pauls (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/FELIX-338?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Karl Pauls reassigned FELIX-338:
--------------------------------
Assignee: Karl Pauls
Yes, I admit that it is always tricky to do performance test. All I can say is that for me the ThreadLocal version runs faster. One issue might be how many native threads your system supports. In case you use much more threads then you really have parallelism must go done. The above, I see on a MacBook Pro with a dual core 2.4 Ghz Intel and under a SunVM under Windows (but that is under VMWare). It is almost the same for 2, 4, and 8 threads but approaches 0 quickly after (at 64 threads I don't see any difference anymore).
I'll go ahead and commit this patch now so that we have a fix in place. However, I agree with your point that we should look at the ldap code again to see whether we can clean it up some more. Let's pick that up again once we have a bug fix release out of the door.
> Framework FilterImpl is not thread safe on execution
> ----------------------------------------------------
>
> Key: FELIX-338
> URL: https://issues.apache.org/jira/browse/FELIX-338
> Project: Felix
> Issue Type: Bug
> Components: Framework
> Affects Versions: 0.8.0, 1.0.0
> Reporter: Felix Meschberger
> Assignee: Karl Pauls
> Attachments: filter.patch, FilterImpl_rewrite.patch
>
>
> Executing a Filter implemented by the Felix Framework FilterImpl class is not thread safe, as the Filter.Implmatch() methods are not synchronized but use a Mapper and Evaluator instance fields which are modified while matching the filter.
> Two options seem to exist:
> 1. Make the match() methods synchronized
> 2. Create Mapper and Evaluator instances on each match() call
> My assumption is that the second method tends to be better because of the synchronization needed with the first approach.
> Reported by Tom Remoleur, thanks. The full mail thread leading to this issue may be found at [1].
> [1] http://www.mail-archive.com/users@felix.apache.org/msg00145.html
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (FELIX-338) Framework FilterImpl is not thread
safe on execution
Posted by "Felix Meschberger (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/FELIX-338?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12519986 ]
Felix Meschberger commented on FELIX-338:
-----------------------------------------
I have to admit, that I never touched ThreadLocals myself upto now. But It seems to be the best of all possibilities to fix the issues.
On the other hand, looking at the code, it seems like the Evaluator basically just ist a single method whose only data modified during evaluation is the operand stack. So, I assume, the Evaluator.evaluate(Mapper) method might just as well be copied to the FilterImpl class and the operand stack a variable local to the method - this stack would be recreated on each call (instead of the Evaluator) and need not be cleared at the end, as this is left to the GC.
The other object - the mapper - is just a facade to the actual data being used for filter matching.
So besides just not reusing the instances, I would also redo the implementation.
> Framework FilterImpl is not thread safe on execution
> ----------------------------------------------------
>
> Key: FELIX-338
> URL: https://issues.apache.org/jira/browse/FELIX-338
> Project: Felix
> Issue Type: Bug
> Components: Framework
> Affects Versions: 0.8.0, 1.0.0
> Reporter: Felix Meschberger
> Attachments: filter.patch
>
>
> Executing a Filter implemented by the Felix Framework FilterImpl class is not thread safe, as the Filter.Implmatch() methods are not synchronized but use a Mapper and Evaluator instance fields which are modified while matching the filter.
> Two options seem to exist:
> 1. Make the match() methods synchronized
> 2. Create Mapper and Evaluator instances on each match() call
> My assumption is that the second method tends to be better because of the synchronization needed with the first approach.
> Reported by Tom Remoleur, thanks. The full mail thread leading to this issue may be found at [1].
> [1] http://www.mail-archive.com/users@felix.apache.org/msg00145.html
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (FELIX-338) Framework FilterImpl is not thread safe
on execution
Posted by "Felix Meschberger (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/FELIX-338?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Felix Meschberger updated FELIX-338:
------------------------------------
Attachment: FilterImpl_rewrite.patch
Here is a patch with what I had in mind. It expectes the following changes in the ldap package:
Parser.getProgram returns Operator[] to not use a cast on each access. I assume the parser creates
a "pure" list anyway, so the getProgram might as well create the proper array
The Unknown class must be public for the FilterImpl.evaluate (copied from Evaluator) to see it
Finally, the evaluate and toStringInfix method are copied from the Evaluator to FilterImpl.
I just saw after finishing the patch, that in fact the FilterImpl class is the only LDAP related class not in the ldap package - so the separation of FilterImpl and Evaluator might make sense.... Don't know for sure.
> Framework FilterImpl is not thread safe on execution
> ----------------------------------------------------
>
> Key: FELIX-338
> URL: https://issues.apache.org/jira/browse/FELIX-338
> Project: Felix
> Issue Type: Bug
> Components: Framework
> Affects Versions: 0.8.0, 1.0.0
> Reporter: Felix Meschberger
> Attachments: filter.patch, FilterImpl_rewrite.patch
>
>
> Executing a Filter implemented by the Felix Framework FilterImpl class is not thread safe, as the Filter.Implmatch() methods are not synchronized but use a Mapper and Evaluator instance fields which are modified while matching the filter.
> Two options seem to exist:
> 1. Make the match() methods synchronized
> 2. Create Mapper and Evaluator instances on each match() call
> My assumption is that the second method tends to be better because of the synchronization needed with the first approach.
> Reported by Tom Remoleur, thanks. The full mail thread leading to this issue may be found at [1].
> [1] http://www.mail-archive.com/users@felix.apache.org/msg00145.html
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (FELIX-338) Framework FilterImpl is not thread safe
on execution
Posted by "Karl Pauls (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/FELIX-338?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Karl Pauls updated FELIX-338:
-----------------------------
Attachment: filter.patch
Using a combination of ThreadLocal and SoftReferences to address the issue.
> Framework FilterImpl is not thread safe on execution
> ----------------------------------------------------
>
> Key: FELIX-338
> URL: https://issues.apache.org/jira/browse/FELIX-338
> Project: Felix
> Issue Type: Bug
> Components: Framework
> Affects Versions: 0.8.0, 1.0.0
> Reporter: Felix Meschberger
> Attachments: filter.patch
>
>
> Executing a Filter implemented by the Felix Framework FilterImpl class is not thread safe, as the Filter.Implmatch() methods are not synchronized but use a Mapper and Evaluator instance fields which are modified while matching the filter.
> Two options seem to exist:
> 1. Make the match() methods synchronized
> 2. Create Mapper and Evaluator instances on each match() call
> My assumption is that the second method tends to be better because of the synchronization needed with the first approach.
> Reported by Tom Remoleur, thanks. The full mail thread leading to this issue may be found at [1].
> [1] http://www.mail-archive.com/users@felix.apache.org/msg00145.html
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (FELIX-338) Framework FilterImpl is not thread
safe on execution
Posted by "Richard S. Hall (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/FELIX-338?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12519995 ]
Richard S. Hall commented on FELIX-338:
---------------------------------------
The LDAP package is self-contained. I don't think it is a good idea to move LDAP code outside the LDAP package, nor is it a good idea to duplicate that code elsewhere, since code duplication is a maintenance headache. Perhaps it would be possible to modify Evaluator.evaluate() method to accept both the program and the mapper so that it can be re-entrant?
> Framework FilterImpl is not thread safe on execution
> ----------------------------------------------------
>
> Key: FELIX-338
> URL: https://issues.apache.org/jira/browse/FELIX-338
> Project: Felix
> Issue Type: Bug
> Components: Framework
> Affects Versions: 0.8.0, 1.0.0
> Reporter: Felix Meschberger
> Attachments: filter.patch, FilterImpl_rewrite.patch
>
>
> Executing a Filter implemented by the Felix Framework FilterImpl class is not thread safe, as the Filter.Implmatch() methods are not synchronized but use a Mapper and Evaluator instance fields which are modified while matching the filter.
> Two options seem to exist:
> 1. Make the match() methods synchronized
> 2. Create Mapper and Evaluator instances on each match() call
> My assumption is that the second method tends to be better because of the synchronization needed with the first approach.
> Reported by Tom Remoleur, thanks. The full mail thread leading to this issue may be found at [1].
> [1] http://www.mail-archive.com/users@felix.apache.org/msg00145.html
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.