[jira] Commented: (OPENJPA-160) Reuse BrokerImpl objects
[ 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
[ 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
[ 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
[ 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.
[jira] Commented: (OPENJPA-160) Reuse BrokerImpl objects
[ 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
[ 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
[ 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] Commented: (OPENJPA-160) Reuse BrokerImpl objects
[ 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] Commented: (OPENJPA-160) Reuse BrokerImpl objects
[ 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
[ 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
[ 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
[ 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] Commented: (OPENJPA-160) Reuse BrokerImpl objects
[ 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
[ 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
[ 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] Commented: (OPENJPA-160) Reuse BrokerImpl objects
[ 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
[ 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
[ 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
[ 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
[ 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.
Re: [jira] Commented: (OPENJPA-160) Reuse BrokerImpl objects
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:[EMAIL PROTECTED] P.S. A good JDO? O, Gasp! smime.p7s Description: S/MIME cryptographic signature
[jira] Commented: (OPENJPA-160) Reuse BrokerImpl objects
[ 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
[ 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
[ 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] Commented: (OPENJPA-160) Reuse BrokerImpl objects
[ 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
[ 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] Commented: (OPENJPA-160) Reuse BrokerImpl objects
[ 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
[ 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] Commented: (OPENJPA-160) Reuse BrokerImpl objects
[ 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
[ 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
[ 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] Commented: (OPENJPA-160) Reuse BrokerImpl objects
[ 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
[ 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
[ 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] Commented: (OPENJPA-160) Reuse BrokerImpl objects
[ 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
[ 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
[ 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.
