Mirror instead invoke dynamic, since uses less memory and cpu #367
Conversation
|
everything seems to be ok! tks Otávio! 👍 |
|
I'd create a MirrorMethodExecution instead. And keep an InvokeDynamicMethodExecution. Java 8 is supposed to fix these memory and cpu issues, so we can switch implementations based on JVM version. |
|
I agree with Lucas. I don't see any reason to delete the interface. I wouls
|
|
I have delete the interface because have many locations that we use mirror directly. |
|
The point of MethodExecutor was to replace all there is no problem of using Mirror to lookup fields and methods AND MethodExecutor to execute methods. Nonetheless, it's very easy to create a MirrorMethodExecutor ;) |
|
I'll close without apply this pull request, because I disagree to keep invoke dynamic. There are no guarantees that invoke dynamic will have better performance in Java 8. I did some test and there are no any gain in Java 8: https://gist.github.com/garcia-jj/8998975 |
|
Reopened as @lucascs was suggest. |
|
Closed, due no replies. |
|
Back again. I did some tests with 2 apps (1 large and another small) and the code with Mirror is really a bit faster. My largest application don't get OutOfMemory anymore many days running without restart. And the cost of each request is a bit faster. Memory used by application is less. Why insist with Invoke Dyn? The API is a lot slower, and we need to use a cache. But also using cache is still slow than Mirror. Since cache have memory cost and the performance still poor, why not drop and using Mirror directly without fear? I know that the name of this API is pretty and there is promisses that will be faster in the future. But testing with JDK 8 is not faster than promissed. So I really don't like to keep this API. |
|
You are right. I do not understand why you resist in keep the interface and
|
|
Simplicity. Mirror can be used directly without a managed object. We already use mirror directly on another places like here: https://github.com/caelum/vraptor4/blob/master/vraptor-core/src/main/java/br/com/caelum/vraptor/core/ExceptionRecorder.java#L74 |
|
Huuummm... I disagree.. managed object is not a problem for us. The On Fri, Feb 21, 2014 at 6:45 PM, Otávio Garcia notifications@github.comwrote:
Alberto www.caelum.com.br |
|
It's not only because a managed object more. But too because the interface is not necessary since Mirror can be used without any config, cache and so on. I can't see any advantage to keep the interface. It's a unnecessary complexity since Mirror can be used directly without injection. The |
|
Since there's no drawback, we could keep the interface, allowing another Mário do Amaral Gonçalves 2014-02-21 20:50 GMT-03:00 Otávio Garcia notifications@github.com:
|
|
Invokedyn is not faster in JDK8. So we have hope that in JDK 9, may be faster. So we wont' use another implementation for the next 2 years (or more, since Oracle delayed the last releases) Without interface we keep code clean in easily to maintain (see the amount of lines dropped) |
|
Ok then. :) Mário do Amaral Gonçalves 2014-02-21 23:23 GMT-03:00 Otávio Garcia notifications@github.com:
|
|
I wrote some code with suggestion to keep interface here: https://github.com/garcia-jj/vraptor4/compare/caelum:ot-noinvokedyn...ot-too-boring-interface?expand=1. So we can see the difference. My conclusion is that dropping interface code stay cleaner and easily to keep and understand. To test a code using mirror directly needs no extra code. But using the interface we need to instantiate the implementation and inject the instance. Using mirror we don't need anything. And we have 120 lines less in the code to maintain. Obviously if we want to migrate to method invoke we need to rewrite the code again. But as I said previously: when? May be in 2 or 3 years up method invoke will faster than reflection. So I don't see any cons to drop all by now. |
|
I think that makes sense to keep using the interface, even without the intention to use MethodExecutor some day. Just because it will encapsulate the use of Mirror... and it seems right ;)
+1 to Alberto's comment |
|
Why be complex since we can do better and simple? Makes non sense. 283 lines less. |
Can you explain better? Makes sense for why? To keep a interface for a think that we won't use in next years? Why add complexity for free? |
|
I guess the point is: remove the invoke dynamic method executor, create the mirror method executor and keep the interface. This is not complex... this is a simple abstraction that doesn't add much code. We probably got the invoke dyn logic wrong, so it will be easier to revisit it later. |
+1 |
283 lines less code. Did you see the diff I sent? Tests can be more simple. |
|
For me, in 2014, discuss about to keep an interface or not because of more On Mon, Feb 24, 2014 at 7:04 PM, Otávio Garcia notifications@github.comwrote:
Alberto www.caelum.com.br |
|
Or everyone accepts Otavio's idea and we delete the interface. He has On Mon, Feb 24, 2014 at 7:09 PM, Alberto SOUZA alots.ssa@gmail.com wrote:
Alberto www.caelum.com.br |
|
I like the idea of keeping the interface and revisit the code later. On Monday, February 24, 2014, Alberto Luiz Souza notifications@github.com
Rafael Ponte |
Is not only for line count. If you read my previous post you understand arguments like simplicity, maintenance and so on. |
|
I think most of us prefer to keep the interface, let's keep it that way? |
|
Why not keep to code simple instead of using an unnecessary interface? This interface was introduced only because method executor. So when method executor die, the interface is not necessary. KISS principle. And again: there are some places that mirror executed methods and was not replaced to use the interface. So why not simple to drop the interface, because original code did not use when need? |
|
There are 8 another places that invoke methods using mirror, but was not introduced the interface. And this code works fine without any debit. So if we keep the interface we need to introduce the interface in all places. Mirror can be used in a single line without any injection and without any external dependency. So I don't see advantage to use an interface. The interface was introduced only because method executor needs cache and because we can't write method executor code in one line only (in @lucascs words). Since Mirror don't need cache and can be write in a single line, there is no reason to keep then. |
|
instantiating any class inside your code is against DIP If we were to replace mirror with any other library, we'd have to change all the code. That's why the interface is useful. Besides, we gain another extension point. Too much arguing about this already. |
|
So why the current code is a frankstein, with 1 place only with the interface and many others without? Why all code are not replaced if your theory is too beautiful? And why only at this time you think about DIP? VRaptor 3 code still with mirror directly for 4 years, and nobody think in this? |
|
As I said, too much arguing about this already. Merge it, we have the git history if we change our mind. |
|
And if we keep the interface, you think in replace all place that uses mirror to invoke methods? |
|
🐑 ? (so we can add and test in RC2, probably this friday) |
|
🐑 ⛵ 🚀 🚌 |
Mirror instead invoke dynamic, since uses less memory and cpu
|
\o/ |
Using mirror instead invoke dynamic, I got less memory and CPU usage. The tests was executed in two steps: production and development using Mission Control.
In production env Wildfly runing in domain mode with 4 nodes with 1G max memory (780M + 250 for permgen). All instances runing on same machine with session replication and dual core CPU.
Before I got between 300M and 500M of memory usage average. Runing for 5-6 days I got an out of memory. After I got somethink like 300M and 400M of memory, and runing the app without any restart for 15 days no out of memory happens.
In development I test the app runing with jmeter with 10 simultaneous users with infinite loop count for 3 days. No out of memory happens and memory still between 300M and 400M using the same parameters like production, except that runing in standalone mode on quad-core machine. Jmeter gots an out of memory in many times, but app still running without any problems.
I'm using vraptor with beans discovery = ALL for this test.
Anyone have interest to do more tests or suugest another aproach to me?