You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@openjpa.apache.org by "Michael Dick (JIRA)" <ji...@apache.org> on 2007/02/22 18:49:05 UTC
[jira] Created: (OPENJPA-160) Reuse BrokerImpl objects
Reuse BrokerImpl objects
------------------------
Key: OPENJPA-160
URL: https://issues.apache.org/jira/browse/OPENJPA-160
Project: OpenJPA
Issue Type: Sub-task
Reporter: Michael Dick
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "Patrick Linskey (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12476335 ]
Patrick Linskey commented on OPENJPA-160:
-----------------------------------------
Hmm. I don't get it. The only thing in newInstance() is 'new BrokerImpl()'. It's my understanding that all of the BrokerImpl construction should be reported in the BrokerImpl.<init> method call. I have a hard time believing that memory allocation (the only other thing that should be involved in a 'new BrokerImpl()' call) is taking up 9.1% of the time.
What profiler are you using? Did you run the test without the profiler also, to see if it's inspection overhead?
Am I wrong in thinking that the construction time, including inlined field assignments, will be in BrokerImpl.<init>?
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
> Assigned To: Patrick Linskey
> Attachments: newprofile.jpg, openjpa-160-patch.txt, openjpa-160-patch.txt, perf2.jpg, perf3.jpg
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "Craig Russell (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12476219 ]
Craig Russell commented on OPENJPA-160:
---------------------------------------
Sweet. The ObjectValue.InstanceFactory is the technique I was going to propose. My only question is whether this pattern exists anywhere else or only in ObjectValue. That might argue for making InstanceFactory a more general interface instead of being a member of ObjectValue.
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
> Attachments: openjpa-160-patch.txt, perf2.jpg, perf3.jpg
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "Patrick Linskey (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12476026 ]
Patrick Linskey commented on OPENJPA-160:
-----------------------------------------
So maybe one alternate would be to just make BrokerImpl cloneable, and put an optimization into BrokerValue (or maybe even ObjectValue!) that can hang onto a template instance for later construction. Or, we could bytecode-generate a class that does a 'new BrokerImpl()', again as an across-the-board optimization in ObjectValue. That would let us get away from any pooling complexity.
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
> Attachments: perf2.jpg, perf3.jpg
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "Michael Dick (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Michael Dick updated OPENJPA-160:
---------------------------------
Attachment: perf3.jpg
Here's a jpg with the call to Class.newInstance() expanded. The data is from a different execution though so the numbers are a little different but they show the same problem.
It looks like the time is spent in reflection accessing the constructor. I don't know what data to gather next though.
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
> Attachments: perf2.jpg, perf3.jpg
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "Patrick Linskey (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12476038 ]
Patrick Linskey commented on OPENJPA-160:
-----------------------------------------
Also, I noticed that your traces show AbstractBrokerFactory.lock() and unlock() in them. I don't think that newBroker() invokes these methods anymore; it might be worthwhile to re-run the tests with the latest code drop.
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
> Attachments: perf2.jpg, perf3.jpg
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "Craig Russell (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12476470 ]
Craig Russell commented on OPENJPA-160:
---------------------------------------
A few questions and a comment.
1. Is the profiling capturing CPU time or wait time?
2. Is the VM synchronizing on the constructor, possibly because of the finalize() method that needs to register with a collection? The VM might just synchronize before construction to avoid synchronizing later.
3. Do we really need the finalizer?
4. What about Patrick's suggestion to use clone() instead of constructing a new instance?
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
> Assigned To: Patrick Linskey
> Attachments: newprofile.jpg, openjpa-160-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, perf2.jpg, perf3.jpg, profile_explicitclass.jpg
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "Patrick Linskey (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12476472 ]
Patrick Linskey commented on OPENJPA-160:
-----------------------------------------
We can easily get rid of the finalizer for the purposes of exploration -- it's just there to help out developers who don't call close() on their own.
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
> Assigned To: Patrick Linskey
> Attachments: newprofile.jpg, openjpa-160-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, perf2.jpg, perf3.jpg, profile_explicitclass.jpg
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "Patrick Linskey (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Patrick Linskey updated OPENJPA-160:
------------------------------------
Attachment: openjpa-160-clone-patch.txt
This patch will clone BrokerImpls from a template instance held in the new BrokerValue class. All initialization has been moved into the initialize() call.
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
> Assigned To: Patrick Linskey
> Attachments: newprofile.jpg, openjpa-160-clone-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, perf2.jpg, perf3.jpg, profile_explicitclass.jpg
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "Patrick Linskey (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12475167 ]
Patrick Linskey commented on OPENJPA-160:
-----------------------------------------
Note that AbstractBrokerFactory already has a data structure for holding weak refs to open brokers. We'd presumably want to incorporate that data structure into whatever we did for pooling.
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "Michael Dick (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12475455 ]
Michael Dick commented on OPENJPA-160:
--------------------------------------
We ran some more performance tests with the latest OpenJPA code and the issue appears to be with creating an instance of the BrokerImpl (when Configurations calls Class.newInstance).
I was surprised that creating a new instance turned out to take so much time and I don't know what we'd could (or would want to) tinker with to try to make it faster to create. I'm not thrilled about adding the complexity of a reuse pool so I'm open to suggestions.
The pool that we used before was a two level pool thread.toString+user+pass -> collection of brokers. Adding a non static field to AbstractBrokerFactory sounds feasible too (unless there's an alternative to pooling).
Still looking into whether we need a key in BrokerImpl - I'll follow up on that as well.
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "John Stecher (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12476502 ]
John Stecher commented on OPENJPA-160:
--------------------------------------
Craig 1) We are using the CPU sampling feature of the profiler to not slow the runtime down to much. Its basically sampling the processor every n microseconds and seeing whats active on the processor when the sample occurs. I have found this shows a pretty good representation of wall clock time for the runtime. 2) and 3) I think Patrick has answered and I look forward to testing the patch. 4) I am open to trying clone as it would show a non-synchronized mechanism for setting up the new object versus the possibly synchronzied newInstance call. Looking at the code we should be able to make clone work in this case. I know from performance testing I did a long time back in the J2EE 1.2 days that clone was something like 4x faster than actually calling newInstance but it was a nightmare to get the clone to actually work in most cases and setup the object correctly.
I look forward to the new patch Patrick.
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
> Assigned To: Patrick Linskey
> Attachments: newprofile.jpg, openjpa-160-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, perf2.jpg, perf3.jpg, profile_explicitclass.jpg
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "Patrick Linskey (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12476629 ]
Patrick Linskey commented on OPENJPA-160:
-----------------------------------------
K... IMO, we should get rid of the finalizer in the default config, and create a new FinalizingBrokerImpl that has a finalizer, that can optionally be used by developers. I think that we should make that the default, and then let appserver providers (who are the most likely to definitely control resources correctly) turn the finalization off.
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
> Assigned To: Patrick Linskey
> Attachments: newprofile.jpg, openjpa-160-clone-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, perf2.jpg, perf3.jpg, profile_clonepatch.jpg, profile_explicitclass.jpg
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "Rob Wisniewski (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12476431 ]
Rob Wisniewski commented on OPENJPA-160:
----------------------------------------
Craig -- I coded up pretty much exactly what you have and it didn't have an impact. You can see the screenshot as profile_explicitclass.
Patrick -- Mike and I tried that yesterday to no avail. It doesn't seem to be that. All of the other instance variables are null except for the JCAHelper object _jca, and creating that class is extremely simple as well. (in fact the constructor isn't even explicit)
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
> Assigned To: Patrick Linskey
> Attachments: newprofile.jpg, openjpa-160-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, perf2.jpg, perf3.jpg, profile_explicitclass.jpg
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "John Stecher (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12476324 ]
John Stecher commented on OPENJPA-160:
--------------------------------------
Our profile is with the first patch.
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
> Assigned To: Patrick Linskey
> Attachments: newprofile.jpg, openjpa-160-patch.txt, openjpa-160-patch.txt, perf2.jpg, perf3.jpg
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "Abe White (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12475363 ]
Abe White commented on OPENJPA-160:
-----------------------------------
+1 to Craig's comments.
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "Rob Wisniewski (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Rob Wisniewski updated OPENJPA-160:
-----------------------------------
Attachment: profile_explicitclass.jpg
I went ahead and made the change that Craig suggested but as you can see it really just changed the label. I don't think the compiler or runtime are doing anything fishy with loading or identifying the class.
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
> Assigned To: Patrick Linskey
> Attachments: newprofile.jpg, openjpa-160-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, perf2.jpg, perf3.jpg, profile_explicitclass.jpg
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "Patrick Linskey (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12475108 ]
Patrick Linskey commented on OPENJPA-160:
-----------------------------------------
> 1. Is there a reason why we can't reuse the BrokerImpl objects?
Architecturally, none that I can think of. But we should take care to ensure that BrokerImpl.free() really does enough work to close up resources appropriately. Also, I'd love to take a look at some profiling data to see if we can just optimize creation of brokers instead of adding the complexity of a pool.
> 2. Assuming we can reuse the objects, where should we put the reuse pool? The
> original implementation created a static map in AbstractBrokerFactory. I'm not sure
> that's the best place for it though. BrokerImpl isn't a final class it's possible that
> different configurations could use different broker implementations (through the
> broker plugin). Maybe we need a new plugin which indicates that class to use as
> a Broker pool?
The two options that I see are a Configuration option and a non-static field in AbstractBrokerFactory. I think that I prefer making it an OpenJPAConfiguration option, so that it's more easily configurable. Configuration would look like so:
<property name="openjpa.BrokerPool" value="Size=50"/>
If BrokerImpl.free() purges the data passed in to the newBroker() call, then we should be able to just use a set. In this scenario, the newBroker() code would then grab something from the set, populate the obtained broker with the data passed into the newBroker() call, and return it.
If BrokerImpl.free() leaves the broker in a state where the data passed into newBroker() is relevant, then we should create a key (probably a private inner class) that includes that data in it, and maintain a map of sets, keyed off of that data.
All things equal, I'd prefer if we could use a Set (the first case).
> 3. Should we pool the broker instances by default? I think we'll want this to be
> configurable, but I'm not sure it needs to be on by default.
We should use the pooling logic, but allow the user to control the pool size. If this is a performance benefit, then we should choose some reasonable initial pool size. I have no idea what 'reasonable' is.
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "Kevin Sutter (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12476744 ]
Kevin Sutter commented on OPENJPA-160:
--------------------------------------
> IIRC, we're operating in a commit-then-review mode in OpenJPA. This issue has been very experimental by nature up earlier today, so I was providing patches to find something that worked. Once we got there, I committed, Abe reviewed, we changed it. Seems pretty much exactly like how we've handled other issues, except that we did a bunch of code-collaboration along the way.
I can see both sides of the argument. If you look at the history on this Issue, there has been lots of discussion, several alternative proposals, several patches considered and tested, and an eventual fix that is acceptable to most everybody. Maybe waiting one day after posting the "final" patch would have been a good compromise before committing the code. Not that we have to do this with every JIRA Issue, but ones like this that attract so much attention and discussion may be good candidates. The extra day would have allowed Abe to get his comments recognized and Craig would have been able to voice his "default action" concern. And, anybody that couldn't wait for the commit to happen could always apply the patch.
> Personally, I prefer more forgiving defaults when possible, so that people don't get bitten when they're just playing around with things. Also, if we decide to change our defaults, I think that we should include openjpa.DataCache, openjpa.QueryCache, and possibly other things listed in the optimization guide in such a change.
Here again, I can see both sides (in a wishy-washy mood). Safe defaults are good to protect the naive users. But, having good performance out of the box is a benefit -- not only for the customer, but also for all of us so that we don't have to explain why we're "protecting" the customer from him/herself. Patrick has a good point. He basically is following suit with the existing OpenJPA behavior in regards to existing optimizing configuration parameters. Maybe we should open a JIRA issue to track this concern. We could discuss all of these optimization parameters and which should be defaulted to which values.
Kevin
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
> Assigned To: Patrick Linskey
> Attachments: newprofile.jpg, openjpa-160-clone-patch.txt, openjpa-160-finalization-and-cloning-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, perf2.jpg, perf3.jpg, profile_clonepatch.jpg, profile_explicitclass.jpg
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "Patrick Linskey (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Patrick Linskey updated OPENJPA-160:
------------------------------------
Attachment: openjpa-160-finalization-and-cloning-patch.txt
- moved BrokerImpl.finalize() to new FinalizingBrokerImpl subtype
- retained clone() logic
- made FinalizingBrokerImpl the default
- added alias for BrokerImpl called 'non-finalizing'
Frameworks that guarantee that EM.close() will be invoked should set the openjpa.BrokerImpl property to 'non-finalizing'.
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
> Assigned To: Patrick Linskey
> Attachments: newprofile.jpg, openjpa-160-clone-patch.txt, openjpa-160-finalization-and-cloning-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, perf2.jpg, perf3.jpg, profile_clonepatch.jpg, profile_explicitclass.jpg
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "Craig Russell (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12476353 ]
Craig Russell commented on OPENJPA-160:
---------------------------------------
The real cost according to the trace is in the dynamically created class, not in BrokerImpl<init>.
I don't get it either, except to think that the implementation of the dynamically created class uses reflection itself, which is slow.
To see if this is the case, we'll need to create a real compiler-generated factory, like this:
openjpa-kernel/src/main/java/org/apache/openjpa/conf/OpenJPAConfigurationImpl.java
...
brokerPlugin.registerInstanceFactory(BrokerImpl.class.getName(),
new BrokerInstanceFactory();
...
public class BrokerInstanceFactory implements ObjectValue.InstanceFactory {
public Object newInstance() {
return new BrokerImpl();
}
...
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
> Assigned To: Patrick Linskey
> Attachments: newprofile.jpg, openjpa-160-patch.txt, openjpa-160-patch.txt, perf2.jpg, perf3.jpg
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "Craig Russell (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12475507 ]
Craig Russell commented on OPENJPA-160:
---------------------------------------
>We ran some more performance tests with the latest OpenJPA code and the issue appears to be with creating an instance of the BrokerImpl (when Configurations calls Class.newInstance).
I'm surprised that the time is being taken in the constructor. The BrokerImpl is actually initialized for real during the initialize method, not the constructor, and that's where I'd expect to find the initialization cost.
There is no constructor implemented for BrokerImpl, so the only initialization done during the compiler-generated constructor is the initialization of the fields. Most of the fields are initialized to null, which takes no time (the initial memory allocated is cleared via a system clear memory call). The only things I see in the field initialization during newInstance are:
private final JCAHelper _jca = new JCAHelper();
This is a stateless instance for which the constructor should be "free".
private ClassLoader _loader = Thread.currentThread().
getContextClassLoader();
Ah, perhaps this is the culprit?
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Assigned: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "Craig Russell (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Craig Russell reassigned OPENJPA-160:
-------------------------------------
Assignee: Patrick Linskey
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
> Assigned To: Patrick Linskey
> Attachments: openjpa-160-patch.txt, openjpa-160-patch.txt, perf2.jpg, perf3.jpg
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "Abe White (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12476359 ]
Abe White commented on OPENJPA-160:
-----------------------------------
FYI, at the bytecode level all field initializations take place in <init>. A profiler would have to jump through a lot of hoops to *exclude* field initializations from <init> overhead. This may have no bearing on the issue depending on where the bottleneck is actually occurring, but I thought I'd point it out.
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
> Assigned To: Patrick Linskey
> Attachments: newprofile.jpg, openjpa-160-patch.txt, openjpa-160-patch.txt, perf2.jpg, perf3.jpg
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "Craig Russell (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12476731 ]
Craig Russell commented on OPENJPA-160:
---------------------------------------
>K... IMO, we should get rid of the finalizer in the default config, and create a new FinalizingBrokerImpl that has a finalizer, that can optionally be used by developers. I think that we should make that the default, and then let appserver providers (who are the most likely to definitely control resources correctly) turn the finalization off.
Two comments, one procedural.
1. I really think we're going just a bit too fast here. I wasn't able to comment on the discussion because I've been in meetings all morning, and it's really tough to find the issue resolved, a patch committed, and I never had a chance. I also notice that just minutes after the commit, Abe had a comment that resulted in another change. For issues that attract this kind of attention, I think a little more time is probably needed to reach closure.
2. I'd like to reopen the discussion of which BrokerImpl should be the default. In general, I like performance options to be the default. It makes the "out of the box" experience better because users don't need to find, much less read, the relevant part of the documentation. Well-behaved applications don't need the finalizer. Small applications running in short-lived vms don't need it.
So it seems the finalizer is only really needed in long-lived vms (application servers, web servers) that have poorly designed applications. Why is this the default? Applications that don't close their ems should be slapped about the upper body. Seems that the finalize method should do some strenuous logging (WARNING or SEVERE) to point out to the developer that they need to change their ways.
Bottom line, I guess I'd prefer that the finalizer version be documented in the troubleshooting section of the doc.
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
> Assigned To: Patrick Linskey
> Attachments: newprofile.jpg, openjpa-160-clone-patch.txt, openjpa-160-finalization-and-cloning-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, perf2.jpg, perf3.jpg, profile_clonepatch.jpg, profile_explicitclass.jpg
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "John Stecher (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12476391 ]
John Stecher commented on OPENJPA-160:
--------------------------------------
Patrick what is in the new version? Not sure what Sutter's conflict is.
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
> Assigned To: Patrick Linskey
> Attachments: newprofile.jpg, openjpa-160-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, perf2.jpg, perf3.jpg
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "Patrick Linskey (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12476393 ]
Patrick Linskey commented on OPENJPA-160:
-----------------------------------------
> Patrick what is in the new version? Not sure what Sutter's conflict is.
It's basically the same. Kevin submitted something that sets the system classloader differently; it so happens that that change is in a line that I indented into an if block in my patch.
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
> Assigned To: Patrick Linskey
> Attachments: newprofile.jpg, openjpa-160-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, perf2.jpg, perf3.jpg
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "John Stecher (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12476333 ]
John Stecher commented on OPENJPA-160:
--------------------------------------
Tested the second patch but basically the same thing WRT performance.
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
> Assigned To: Patrick Linskey
> Attachments: newprofile.jpg, openjpa-160-patch.txt, openjpa-160-patch.txt, perf2.jpg, perf3.jpg
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "Patrick Linskey (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12476039 ]
Patrick Linskey commented on OPENJPA-160:
-----------------------------------------
The profiling data seems to point to reflection as the culprit; the init() call is very very fast.
Could someone with access to the benchmark try out just calling 'new BrokerImpl() from JDBCConfigurationImpl or something? That should tell us a lot about where the cost is coming from.
WRT per-thread pooling -- it sounds like you're proposing that we would actually share brokers between multiple EMs in the same thread. I think that that would have a lot of undesired consequences, would certainly violate a bunch of the intent of the JPA spec, and would probably fail in the CTS. In OpenJPA, each logical EM definitely needs to have access to a unique Broker. Pooling could help us reduce the cost of obtaining such a Broker, but sharing would be a pretty significant semantic change.
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
> Attachments: perf2.jpg, perf3.jpg
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "John Stecher (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12476351 ]
John Stecher commented on OPENJPA-160:
--------------------------------------
All performance throughput numbers are run without the profiler on a 2-way Linux/Intel machine. 1410 tps at 99% CPU without the patch, 1413 tps at 99% CPU with both patches. Those numbers are average throughput over a 3 minute run with another 3 minute run as a warmup.
Flipping the profiler on only after gathering those numbers is what shows the above graphs.
Using Jprofiler as well as IBM internal tools.
I am in agreement with you that I am a little taken aback by the overhead of the newinstance call but I can tell you that the <init> you see in the profile is the constructor and only the constructor my knowledge of the profiling tools, everything else under newinstance is the class being setup by the VM and the class members being initialized.
Pooling the objects and thus eliminating the object creation completely leads to a tps rate of 1506 using the same methodology talked about above.
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
> Assigned To: Patrick Linskey
> Attachments: newprofile.jpg, openjpa-160-patch.txt, openjpa-160-patch.txt, perf2.jpg, perf3.jpg
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "Craig Russell (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12476321 ]
Craig Russell commented on OPENJPA-160:
---------------------------------------
For the record, the new patch changes a couple of things. The factory method now does not take an argument, but what is registered is the full class name of the implementing class along with the factory for the class. This allows, for example, multiple factories to be registered for the same instance of ObjectValue, one of which is selected at configuration time.
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
> Assigned To: Patrick Linskey
> Attachments: newprofile.jpg, openjpa-160-patch.txt, openjpa-160-patch.txt, perf2.jpg, perf3.jpg
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Resolved: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "Patrick Linskey (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Patrick Linskey resolved OPENJPA-160.
-------------------------------------
Resolution: Fixed
Hopefully, we can put this to rest now.
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
> Assigned To: Patrick Linskey
> Attachments: newprofile.jpg, openjpa-160-clone-patch.txt, openjpa-160-finalization-and-cloning-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, perf2.jpg, perf3.jpg, profile_clonepatch.jpg, profile_explicitclass.jpg
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "Craig Russell (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Craig Russell updated OPENJPA-160:
----------------------------------
Attachment: openjpa-160-patch.txt
This patch uses a regular class instead of an anonymous inner class to create the BrokerImplFactory. It is supposed to include everything in Patrick's patch.
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
> Assigned To: Patrick Linskey
> Attachments: newprofile.jpg, openjpa-160-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, perf2.jpg, perf3.jpg, profile_explicitclass.jpg
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "Patrick Linskey (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12476749 ]
Patrick Linskey commented on OPENJPA-160:
-----------------------------------------
> The extra day would have allowed Abe to get his comments recognized
> and Craig would have been able to voice his "default action" concern.
I guess I fail to see the problem. Craig has voiced his "default action" concern, and Abe did get his comments recognized. IMO, the real issue here is whether we want to do review-then-commit or commit-then-review. Unless there is some known way to say "this issue needs to be review-then-commit", this problem will just keep on repeating itself.
Personally, I like to get changes off of my local machine and into svn as soon as I can, as I've found that letting changes linger is problematic.
Remember that (modulo svn's issues with David's checkin) we have history here. Just checking something in doesn't mean that it's necessarily done.
> But, having good performance out of the box is a benefit -- not only for the
> customer, but also for all of us so that we don't have to explain why we're
> "protecting" the customer from him/herself.
It's worth noting that in this situation, it's not a performance issue per se, but rather a scalability issue, since it only crops up under heavily-concurrent usage patterns. I'd expect that anyone doing that type of coding and not using an appserver would be reading through the optimization guide in detail.
Which brings up an interesting possibility: we could set the value differently if the entry point is from PersistenceProvider.createContainerEntityManagerFactory(), since an appserver really really should be managing resources correctly.
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
> Assigned To: Patrick Linskey
> Attachments: newprofile.jpg, openjpa-160-clone-patch.txt, openjpa-160-finalization-and-cloning-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, perf2.jpg, perf3.jpg, profile_clonepatch.jpg, profile_explicitclass.jpg
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "Patrick Linskey (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Patrick Linskey updated OPENJPA-160:
------------------------------------
Attachment: openjpa-160-patch.txt
This patch should eliminate any reflection overhead for the BrokerImpl call. While implementing it, I noticed a couple of other plugins that are instantiated via reflection during BrokerImpl instantiation, so I used the new optimization for them as well.
Please run this against the benchmarks that demonstrated the problem and report back as to whether or not this resolves the issue.
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
> Attachments: openjpa-160-patch.txt, perf2.jpg, perf3.jpg
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "Patrick Linskey (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Patrick Linskey updated OPENJPA-160:
------------------------------------
Attachment: openjpa-160-patch.txt
New version to resolve conflict with Kevin Sutter's change.
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
> Assigned To: Patrick Linskey
> Attachments: newprofile.jpg, openjpa-160-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, perf2.jpg, perf3.jpg
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "Craig Russell (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12475511 ]
Craig Russell commented on OPENJPA-160:
---------------------------------------
Great screen shot, but when I click on the frypan next to newInstance, nothing happens. ;-)
Could you go a few levels down into newInstance and see what the heck is taking so long? My money is on getContextClassLoader or registerFinalizer.
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
> Attachments: perf2.jpg
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "Rob Wisniewski (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12476626 ]
Rob Wisniewski commented on OPENJPA-160:
----------------------------------------
Okay.. looks like the finalizer was the problem. I took it out and we're right up where the pooling prototype puts us with respect to performance. So the question is are we willing to take away the developer's safety net and get rid of the finalizer? Our calculations are showing about 8% improvement for this fix alone in our particular scenario. I like those numbers. Is there a way we can make this work?
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
> Assigned To: Patrick Linskey
> Attachments: newprofile.jpg, openjpa-160-clone-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, perf2.jpg, perf3.jpg, profile_clonepatch.jpg, profile_explicitclass.jpg
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "Patrick Linskey (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12476423 ]
Patrick Linskey commented on OPENJPA-160:
-----------------------------------------
Did you guys get a chance to try setting the ClassLoader field to null to see if that's the culprit somehow? I'd be surprised, but then again, I'm surprised that the contents of the InstanceFactory method could possibly be accounting for 9% of anything.
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
> Assigned To: Patrick Linskey
> Attachments: newprofile.jpg, openjpa-160-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, perf2.jpg, perf3.jpg, profile_explicitclass.jpg
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "John Stecher (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
John Stecher updated OPENJPA-160:
---------------------------------
Attachment: newprofile.jpg
Profile with ObjectValue factory.
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
> Assigned To: Patrick Linskey
> Attachments: newprofile.jpg, openjpa-160-patch.txt, openjpa-160-patch.txt, perf2.jpg, perf3.jpg
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "Patrick Linskey (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12476234 ]
Patrick Linskey commented on OPENJPA-160:
-----------------------------------------
> Patrick - I'll pull your patch and we'll test it out here today on the benchmark.
Cool -- I'm looking forward to seeing the results.
> My only question is whether this pattern exists anywhere else or only in
> ObjectValue. That might argue for making InstanceFactory a more general
> interface instead of being a member of ObjectValue.
I thought about that, and decided to just keep it in ObjectValue until we discover the need for it elsewhere.
> As you've coded it, each instance of ObjectValue gets its own Map. Does a
> given instance need more than one factory? It could just be stored as a
> member instead of a Map.
>
> On the other hand, if the Map is static, it should probably be a Concurrent Map
> and account for garbage collecting undeployed classes.
I put a Map in place to get around any ClassLoader issues. In the common case, it shouldn't be an issue, since all the built-in OpenJPA classes will presumably be in the same classloader as the ObjectValue class. But the existing code handled more complex ClassLoader situations, so I figured I'd preserve that feature.
The set-up happens during BrokerFactory initialization, so there is no real need to make it static. Making it static might speed things up between multiple BrokerFactories, but I like having different BrokerFactories be independent.
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
> Attachments: openjpa-160-patch.txt, perf2.jpg, perf3.jpg
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "Craig Russell (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12475238 ]
Craig Russell commented on OPENJPA-160:
---------------------------------------
I think it's great that profiling EntityManager creation resulted in this analysis.
I'd like to know why Broker creation is so expensive. Most of the static information should be kept in the Broker factory so configuring and looking up stuff should be "free". The only thing that is needed for a new Broker should be the various identity maps and such that bind the EM to the persistence context. These artifacts should be just as cheap to create as to clear.
Until we understand why it's so expensive to create Brokers, I'd like to stay away from pools. From my experience, trying to dynamically manage pools is extremely difficult, and making the user responsible for tuning the pools is a disaster waiting to happen.
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "John Stecher (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12476438 ]
John Stecher commented on OPENJPA-160:
--------------------------------------
So here is a thought. Basically this benchmark is performing 5 interactions with the database doing a simple FBPK in this scenario. Thus the tps numbers above really need to be multiplied by 5 to get the total number of times we pass into the JPA code base. So that really means that we are doing 7000 invocations per second spread across 50 runtime threads in the appserver. This could lead to a heck of a lot of contention on any single sync point which is what we saw in the earlier improvements we worked on with Kevin and company.
Now someone can correct me if I am wrong here but isnt the JVM specification clear that when initializing a class the class object must be synchronized on. Thus possibly making this the single most contended point in the codebase now that we have removed all the other sync points in the earlier performance work?
We are doing CPU sampling based profiling to cut overhead to a minimum so that would show something like this as a hotspot I would think as the actually sync point is in C code and happens under the newintsance method.
Anyway just throwing that out there for a reason this could be happening. I am still baffled. Need to think about a test case to prove this as well.
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
> Assigned To: Patrick Linskey
> Attachments: newprofile.jpg, openjpa-160-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, perf2.jpg, perf3.jpg, profile_explicitclass.jpg
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
Re: [jira] Commented: (OPENJPA-160) Reuse BrokerImpl objects
Posted by Craig L Russell <Cr...@Sun.COM>.
I've got it coded up and am running sanity tests. I'll post the new
patch as soon as I verify I didn't break anything obvious.
Craig
On Feb 27, 2007, at 1:11 PM, John Stecher (JIRA) wrote:
>
> [ https://issues.apache.org/jira/browse/OPENJPA-160?
> page=com.atlassian.jira.plugin.system.issuetabpanels:comment-
> tabpanel#action_12476390 ]
>
> John Stecher commented on OPENJPA-160:
> --------------------------------------
>
> Craig - I will try to get it coded up this afternoon. Got side
> tracked on other issues here. :( If you can provide me the code I
> would be grateful. If not hopefully we (Me and another IBMer are
> working on this) can get it coded up later this afternoon.
>
> Abe - The last time I looked at bytecode in depth (it's been a
> while) the field declarations and method declarations were separate
> in both where they were located in the class file and when they
> were executed. I thought static and field identifiers were
> executed first in alphabetical order and then the init method was
> called. Then I remember there being a <clinit> and <init> method
> as well but cant recall what they do off the top of my head.
>
>> Reuse BrokerImpl objects
>> ------------------------
>>
>> Key: OPENJPA-160
>> URL: https://issues.apache.org/jira/browse/
>> OPENJPA-160
>> Project: OpenJPA
>> Issue Type: Sub-task
>> Reporter: Michael Dick
>> Assigned To: Patrick Linskey
>> Attachments: newprofile.jpg, openjpa-160-patch.txt,
>> openjpa-160-patch.txt, openjpa-160-patch.txt, perf2.jpg, perf3.jpg
>>
>>
>
>
> --
> This message is automatically generated by JIRA.
> -
> You can reply to this email to add a comment to the issue online.
>
Craig Russell
Architect, Sun Java Enterprise System http://java.sun.com/products/jdo
408 276-5638 mailto:Craig.Russell@sun.com
P.S. A good JDO? O, Gasp!
[jira] Commented: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "John Stecher (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12476390 ]
John Stecher commented on OPENJPA-160:
--------------------------------------
Craig - I will try to get it coded up this afternoon. Got side tracked on other issues here. :( If you can provide me the code I would be grateful. If not hopefully we (Me and another IBMer are working on this) can get it coded up later this afternoon.
Abe - The last time I looked at bytecode in depth (it's been a while) the field declarations and method declarations were separate in both where they were located in the class file and when they were executed. I thought static and field identifiers were executed first in alphabetical order and then the init method was called. Then I remember there being a <clinit> and <init> method as well but cant recall what they do off the top of my head.
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
> Assigned To: Patrick Linskey
> Attachments: newprofile.jpg, openjpa-160-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, perf2.jpg, perf3.jpg
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "Patrick Linskey (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12476343 ]
Patrick Linskey commented on OPENJPA-160:
-----------------------------------------
Assuming that field initializations aren't being counted in <init>, the only thing that could be taking time is the Thread.currentThread().getContextClassLoader() call. Can you run a test with _loader set to null to see if that's the culprit?
Based on quick examination, it looks like if it's set to null, the code should still work, but it's possible that having a null loader returned by BrokerImpl.getClassLoader() could cause problems.
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
> Assigned To: Patrick Linskey
> Attachments: newprofile.jpg, openjpa-160-patch.txt, openjpa-160-patch.txt, perf2.jpg, perf3.jpg
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "Patrick Linskey (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12476471 ]
Patrick Linskey commented on OPENJPA-160:
-----------------------------------------
So the only thing that I think is left to try to control is the field declarations in BrokerImpl. I'm going to put together a patch that moves all fields in BrokerImpl into an inner class, sets the inner class instance variable to null in the constructor phase, and then instantiates the inner class (and therefore the extra memory of the variables) during initialize(), which is called after construction.
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
> Assigned To: Patrick Linskey
> Attachments: newprofile.jpg, openjpa-160-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, perf2.jpg, perf3.jpg, profile_explicitclass.jpg
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "Patrick Linskey (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12476520 ]
Patrick Linskey commented on OPENJPA-160:
-----------------------------------------
In our situation, cloning might not be a nightmare, as almost all initialization already happens in initialize().
Patches coming later this evening.
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
> Assigned To: Patrick Linskey
> Attachments: newprofile.jpg, openjpa-160-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, perf2.jpg, perf3.jpg, profile_explicitclass.jpg
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "Michael Dick (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Michael Dick updated OPENJPA-160:
---------------------------------
Attachment: perf2.jpg
I have a screenshot from the performance tool. I don't have access to the performance test environment right now but I'll see what else I can share on Monday.
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
> Attachments: perf2.jpg
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "John Stecher (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12476228 ]
John Stecher commented on OPENJPA-160:
--------------------------------------
Patrick - I'll pull your patch and we'll test it out here today on the benchmark. I agree with Craig that it seems like its the reflection thats kicking us in the butt the most so we will see here.
WTR the per-thread pooling I was thinking of something a little more intelligent with the key being the thread-id and the EM id thus making it only usable in a single EM and on a single thread. If you had multiple EM's you would have multiple BrokerImpl's and thus not share amongst the EM's. In any regard I'm just throwing it out here to get kicked around and hardened. If your current patch works it won't be necessary. :)
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
> Attachments: openjpa-160-patch.txt, perf2.jpg, perf3.jpg
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "Abe White (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12476396 ]
Abe White commented on OPENJPA-160:
-----------------------------------
> The last time I looked at bytecode in depth (it's been a while) the field declarations and method declarations were separate in both where they were located in the class file and when they were executed.
The names and types of your fields are defined in their own area of the class file. But code used to initialize instance field values inline is dumped into your constructor bytecode (<init>) before any constructor code you write. And code used to initialize static field values inline is dumped into your static initializer bytecode (<clinit>) before any static block code you write.
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
> Assigned To: Patrick Linskey
> Attachments: newprofile.jpg, openjpa-160-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, perf2.jpg, perf3.jpg
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "Craig Russell (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12476433 ]
Craig Russell commented on OPENJPA-160:
---------------------------------------
Rob -- thanks, our coding passed in the mail.
All -- WTF?
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
> Assigned To: Patrick Linskey
> Attachments: newprofile.jpg, openjpa-160-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, perf2.jpg, perf3.jpg, profile_explicitclass.jpg
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "John Stecher (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12476320 ]
John Stecher commented on OPENJPA-160:
--------------------------------------
Patrick - Tested the patch and it has the same performance as the original implementation. Basically the cost is still in the BrokerImpl newinstance call which is setting up the class. I have attached a profile showing the runtime with your patch in it. If you look closely all we really did was move the cost of the BrokerImpl create into the inner class that you created. The profile shows that the <init> method is costing next to nothing compared to the actual setup of the class and class variables. From the best of my knowledge the <init> method in the profile is the constructor while the rest of the time is class setup overhead. So given this I'd say we still dont have a fix that addresses the issue yet.
John
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
> Assigned To: Patrick Linskey
> Attachments: newprofile.jpg, openjpa-160-patch.txt, openjpa-160-patch.txt, perf2.jpg, perf3.jpg
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "Michael Dick (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12475102 ]
Michael Dick commented on OPENJPA-160:
--------------------------------------
Another performance issue we've run into is the overhead of creating a new BrokerImpl object when the application calls createEntityManager. The JPA spec clearly states that the provider needs to return a new EntityManager instance, and we're not trying to circumvent that requirement. However it seems plausible that we could reuse the underlying BrokerImpl object, once all the persistence data has been cleared (ie after BrokerImpl.free has been called). Implementing a fairly simple object reuse pool resulted in a significant performance improvement in our testing. I don't see this as being a violation of the intent of the spec, but I'd rather get a sense of consensus before I/we go any further.
Questions :
1. Is there a reason why we can't reuse the BrokerImpl objects?
2. Assuming we can reuse the objects, where should we put the reuse pool? The original implementation created a static map in AbstractBrokerFactory. I'm not sure that's the best place for it though. BrokerImpl isn't a final class it's possible that different configurations could use different broker implementations (through the broker plugin). Maybe we need a new plugin which indicates that class to use as a Broker pool?
3. Should we pool the broker instances by default? I think we'll want this to be configurable, but I'm not sure it needs to be on by default.
Justification :
We've been running tests with the Sun Application Server and adding in a BrokerImpl reuse pool brings the performance on par with Hibernate.
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "Rob Wisniewski (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12476627 ]
Rob Wisniewski commented on OPENJPA-160:
----------------------------------------
Also, to be clear, the cloning seems to give us a little boost too so I'd love to integrate that as well. I tried both ways. Clone w/ finalizer removed is 1522 req/sec and NewInstance w/ finalizer removed is 1504. Considering the clone change is fairly inoccuous I say let's go with that.
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
> Assigned To: Patrick Linskey
> Attachments: newprofile.jpg, openjpa-160-clone-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, perf2.jpg, perf3.jpg, profile_clonepatch.jpg, profile_explicitclass.jpg
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "John Stecher (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12476032 ]
John Stecher commented on OPENJPA-160:
--------------------------------------
>From the testing that we have done fooling with different prototyped versions and making different fields in BrokerImpl static to avoid recreation I am pretty (almost 100%) sure the cost we are looking at here is just that of creating this class in general being expensive. I would love to have someone else profile the code with different tools than I have at my disposal and see if they find a different culprit.
WRT pooling I think a reasonable solution would not be to create a massive pool of objects but just one per thread-id to optimize for the general case. I am assuming that one Broker per thread is common. I am with everyone else in that I would love to keep configuration to a minimum overall. I am not a big fan of exposing pool settings to a user as if we decide to change it later on you might have to support the setting beyond when you really want too. :-) Any thoughts on why this would not work or can someone enlighten me on what the general use case is?
Patrick - I would be worried about the Clone being almost as heavy weight as what we are doing now but need to implement and test it.
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
> Attachments: perf2.jpg, perf3.jpg
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "Patrick Linskey (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12475465 ]
Patrick Linskey commented on OPENJPA-160:
-----------------------------------------
Do you have any stack traces or anything that you can share to help debug the issue?
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "Craig Russell (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12476295 ]
Craig Russell commented on OPENJPA-160:
---------------------------------------
>I put a Map in place to get around any ClassLoader issues. In the common case, it shouldn't be an issue, since all the built-in OpenJPA classes will presumably be in the same classloader as the ObjectValue class. But the existing code handled more complex ClassLoader situations, so I figured I'd preserve that feature.
Once you have a Class for the type, there are no classloader issues any more. The code only looks at the classloader of the type. So I don't see the need for generalization.
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
> Attachments: openjpa-160-patch.txt, perf2.jpg, perf3.jpg
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "Rob Wisniewski (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Rob Wisniewski updated OPENJPA-160:
-----------------------------------
Attachment: profile_clonepatch.jpg
Guys... sorry to keep bearing bad news, but we seem to just be shifting this stuff around constantly.. Now the clone() call takes up the time as you can see in profile_clonepatch.
John suggested that I take out the finalizer as was suggested early and give it a shot. I like that idea and I'll update after I try it.
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
> Assigned To: Patrick Linskey
> Attachments: newprofile.jpg, openjpa-160-clone-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, perf2.jpg, perf3.jpg, profile_clonepatch.jpg, profile_explicitclass.jpg
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "Patrick Linskey (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Patrick Linskey updated OPENJPA-160:
------------------------------------
Attachment: openjpa-160-patch.txt
Craig identified a problem with the patch; after talking with him on the phone, I've updated it to address his concern. Thanks, Craig!
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
> Attachments: openjpa-160-patch.txt, openjpa-160-patch.txt, perf2.jpg, perf3.jpg
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "Patrick Linskey (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12476736 ]
Patrick Linskey commented on OPENJPA-160:
-----------------------------------------
> 1. I really think we're going just a bit too fast here. I wasn't able to comment
> on the discussion because I've been in meetings all morning, and it's really
> tough to find the issue resolved, a patch committed, and I never had a chance.
> I also notice that just minutes after the commit, Abe had a comment that
> resulted in another change. For issues that attract this kind of attention, I
> think a little more time is probably needed to reach closure.
IIRC, we're operating in a commit-then-review mode in OpenJPA. This issue has been very experimental by nature up earlier today, so I was providing patches to find something that worked. Once we got there, I committed, Abe reviewed, we changed it. Seems pretty much exactly like how we've handled other issues, except that we did a bunch of code-collaboration along the way.
> 2. I'd like to reopen the discussion of which BrokerImpl should be the default.
> In general, I like performance options to be the default. It makes the "out of
> the box" experience better because users don't need to find, much less read,
> the relevant part of the documentation. Well-behaved applications don't need
> the finalizer. Small applications running in short-lived vms don't need it.
Personally, I prefer more forgiving defaults when possible, so that people don't get bitten when they're just playing around with things. Also, if we decide to change our defaults, I think that we should include openjpa.DataCache, openjpa.QueryCache, and possibly other things listed in the optimization guide in such a change.
Any other opinions?
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
> Assigned To: Patrick Linskey
> Attachments: newprofile.jpg, openjpa-160-clone-patch.txt, openjpa-160-finalization-and-cloning-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, perf2.jpg, perf3.jpg, profile_clonepatch.jpg, profile_explicitclass.jpg
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "Craig Russell (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12476225 ]
Craig Russell commented on OPENJPA-160:
---------------------------------------
As you've coded it, each instance of ObjectValue gets its own Map. Does a given instance need more than one factory? It could just be stored as a member instead of a Map.
On the other hand, if the Map is static, it should probably be a Concurrent Map and account for garbage collecting undeployed classes.
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
> Attachments: openjpa-160-patch.txt, perf2.jpg, perf3.jpg
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-160) Reuse BrokerImpl objects
Posted by "Rob Wisniewski (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-160?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12476680 ]
Rob Wisniewski commented on OPENJPA-160:
----------------------------------------
Patrick.. good work. The new patch performs as expected. I test with and without the new config property and everything seems to perform as expected.
Everyone agree with integrating this?
> Reuse BrokerImpl objects
> ------------------------
>
> Key: OPENJPA-160
> URL: https://issues.apache.org/jira/browse/OPENJPA-160
> Project: OpenJPA
> Issue Type: Sub-task
> Reporter: Michael Dick
> Assigned To: Patrick Linskey
> Attachments: newprofile.jpg, openjpa-160-clone-patch.txt, openjpa-160-finalization-and-cloning-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, openjpa-160-patch.txt, perf2.jpg, perf3.jpg, profile_clonepatch.jpg, profile_explicitclass.jpg
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.