You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@shindig.apache.org by li...@apache.org on 2008/12/05 11:54:30 UTC
svn commit: r723705 -
/incubator/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/util/BeanJsonConverter.java
Author: lindner
Date: Fri Dec 5 02:54:27 2008
New Revision: 723705
URL: http://svn.apache.org/viewvc?rev=723705&view=rev
Log:
Performance Patch from hi5
- errorMessage was being created for every getter json serialization
- on a heavily loaded server GC overhead went from 10% to 2%
Modified:
incubator/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/util/BeanJsonConverter.java
Modified: incubator/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/util/BeanJsonConverter.java
URL: http://svn.apache.org/viewvc/incubator/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/util/BeanJsonConverter.java?rev=723705&r1=723704&r2=723705&view=diff
==============================================================================
--- incubator/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/util/BeanJsonConverter.java (original)
+++ incubator/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/util/BeanJsonConverter.java Fri Dec 5 02:54:27 2008
@@ -78,7 +78,7 @@
* @param pojo The object to convert
* @return An object whos toString method will return json
*/
- public String convertToString(Object pojo) {
+ public String convertToString(final Object pojo) {
return convertToJson(pojo).toString();
}
@@ -88,7 +88,7 @@
* @param pojo The object to convert
* @return An object whos toString method will return json
*/
- public Object convertToJson(Object pojo) {
+ public Object convertToJson(final Object pojo) {
try {
return translateObjectToJson(pojo);
} catch (JSONException e) {
@@ -96,7 +96,7 @@
}
}
- private Object translateObjectToJson(Object val) throws JSONException {
+ private Object translateObjectToJson(final Object val) throws JSONException {
if (val instanceof Object[]) {
JSONArray array = new JSONArray();
for (Object asd : (Object[]) val) {
@@ -143,7 +143,7 @@
* @param pojo The object to convert
* @return A JSONObject representing this pojo
*/
- private JSONObject convertMethodsToJson(Object pojo) {
+ private JSONObject convertMethodsToJson(final Object pojo) {
List<MethodPair> availableGetters;
availableGetters = GETTER_METHODS.get(pojo.getClass());
@@ -154,31 +154,34 @@
JSONObject toReturn = new JSONObject();
for (MethodPair getter : availableGetters) {
- String errorMessage = "Could not encode the " + getter.method + " method on "
- + pojo.getClass().getName();
try {
Object val = getter.method.invoke(pojo, EMPTY_OBJECT);
if (val != null) {
toReturn.put(getter.fieldName, translateObjectToJson(val));
}
} catch (JSONException e) {
- throw new RuntimeException(errorMessage, e);
+ throw new RuntimeException(errorMessage(pojo, getter), e);
} catch (IllegalAccessException e) {
- throw new RuntimeException(errorMessage, e);
+ throw new RuntimeException(errorMessage(pojo, getter), e);
} catch (InvocationTargetException e) {
- throw new RuntimeException(errorMessage, e);
+ throw new RuntimeException(errorMessage(pojo, getter), e);
} catch (IllegalArgumentException e) {
- throw new RuntimeException(errorMessage, e);
+ throw new RuntimeException(errorMessage(pojo, getter), e);
}
}
return toReturn;
}
- private static class MethodPair {
+ private static String errorMessage(Object pojo, MethodPair getter) {
+ return "Could not encode the " + getter.method + " method on "
+ + pojo.getClass().getName();
+ }
+
+ private static final class MethodPair {
public Method method;
public String fieldName;
- private MethodPair(Method method, String fieldName) {
+ private MethodPair(final Method method, final String fieldName) {
this.method = method;
this.fieldName = fieldName;
}
Re: svn commit: r723705 -
Posted by Paul Lindner <pl...@hi5.com>.
Apologies my intentions were not clear.
Final variables == good
Final classes == bad
On Dec 9, 2008, at 1:40 PM, Rodrigo Gallardo wrote:
> On Tue, Dec 09, 2008 at 01:20:27PM -0800, Paul Lindner wrote:
>>
>> On Dec 8, 2008, at 9:36 AM, Henning P. Schmiedehausen wrote:
>>
>>> Paul Lindner <pl...@hi5.com> writes:
>>>
>>>> The error string was generated for every single field when
>>>> serializing
>>>> an object. So if you have the person object with say 10 fields you
>>>> generate 10 error message strings. That's a lot of Strings!
>>>
>>> Yes, that is obvious. I was wondering about the final modifiers
>>> because this is how I tend to write code and a while back, a
>>> number of
>>> my patches were shot down because "this does not adhere to the
>>> rest of
>>> the shindig code". If we start changing this, that is better.
>>>
>>> [...]
>>
>> actually:
>>
>> http://cwiki.apache.org/SHINDIGxSITE/java-style.html
>>
>>
>> final
>> Use final whenever possible on all member variables.
>>
>> Reasoning: Avoids missing initialization.
>>
>> However, since this is a library I would not make anything but static
>> inner classes final.
>
> Sorry, I don't see how that follows. final, when applied to classes
> and methods means 'not overridable by inheritance'. When applied to
> variables means 'can only be assigned once'. Yes, I agree that classes
> and methods in a library should be inheritable from, but I fail to see
> how member variables have much to do with that.
>
> And making variables final also helps (lots) with thread safety, which
> is an importatn concern.
Paul Lindner
plindner@hi5.com
Re: svn commit: r723705 -
Posted by Rodrigo Gallardo <ro...@nul-unu.com>.
On Tue, Dec 09, 2008 at 01:20:27PM -0800, Paul Lindner wrote:
>
> On Dec 8, 2008, at 9:36 AM, Henning P. Schmiedehausen wrote:
>
>> Paul Lindner <pl...@hi5.com> writes:
>>
>>> The error string was generated for every single field when
>>> serializing
>>> an object. So if you have the person object with say 10 fields you
>>> generate 10 error message strings. That's a lot of Strings!
>>
>> Yes, that is obvious. I was wondering about the final modifiers
>> because this is how I tend to write code and a while back, a number of
>> my patches were shot down because "this does not adhere to the rest of
>> the shindig code". If we start changing this, that is better.
>>
>> [...]
>
> actually:
>
> http://cwiki.apache.org/SHINDIGxSITE/java-style.html
>
>
> final
> Use final whenever possible on all member variables.
>
> Reasoning: Avoids missing initialization.
>
> However, since this is a library I would not make anything but static
> inner classes final.
Sorry, I don't see how that follows. final, when applied to classes
and methods means 'not overridable by inheritance'. When applied to
variables means 'can only be assigned once'. Yes, I agree that classes
and methods in a library should be inheritable from, but I fail to see
how member variables have much to do with that.
And making variables final also helps (lots) with thread safety, which
is an importatn concern.
Re: svn commit: r723705 -
Posted by Kevin Brown <et...@google.com>.
On Tue, Dec 9, 2008 at 1:20 PM, Paul Lindner <pl...@hi5.com> wrote:
>
> On Dec 8, 2008, at 9:36 AM, Henning P. Schmiedehausen wrote:
>
> Paul Lindner <pl...@hi5.com> writes:
>>
>> The error string was generated for every single field when serializing
>>> an object. So if you have the person object with say 10 fields you
>>> generate 10 error message strings. That's a lot of Strings!
>>>
>>
>> Yes, that is obvious. I was wondering about the final modifiers
>> because this is how I tend to write code and a while back, a number of
>> my patches were shot down because "this does not adhere to the rest of
>> the shindig code". If we start changing this, that is better.
>>
>> [...]
>>
>
> actually:
>
> http://cwiki.apache.org/SHINDIGxSITE/java-style.html
>
>
> final
> Use final whenever possible on all member variables.
>
> Reasoning: Avoids missing initialization.
>
> However, since this is a library I would not make anything but static inner
> classes final.
I think Henning was referring to final on method arguments, which is
completely useless unless language semantics require them (say, inner
classes). The current style guide only refers to member variables, where
final is encouraged to promote type safety and ensuring explicit
initialization is performed. I haven't seen a good argument for using final
on method parameters, so the only remaining argument is consistency. If
someone wants final on all method parameters, I say go for it, but first you
have to update every method in every class in the entire code base to
conform, and then get someone to review it for you. Good luck with that.
>
>
>
>> Well, then we'd have to duplicate code inside of each catch block.
>>>
>>
>> It does give you the opportunity to customize the error message per
>> catch block and it actually reduces the amount of unneccessarily
>> created strings to 0. I would be interested to see this under YourKit
>> profiler with 5k req/sec; I would guess that you would see another
>> performance increase.
>>
>>
> Actually each catch block calls the private method with the proper args.
> We're good, but yes we could change this to accommodate a custom error
> message.
>
> I'm pretty happy with the implementation as it stands.
>>>
>>
>> That does not mean it is optimal. :-)
>>
>
>
> Indeed!
>
>
>
> Paul Lindner
> plindner@hi5.com
>
>
>
>
Re: svn commit: r723705 -
Posted by Paul Lindner <pl...@hi5.com>.
On Dec 8, 2008, at 9:36 AM, Henning P. Schmiedehausen wrote:
> Paul Lindner <pl...@hi5.com> writes:
>
>> The error string was generated for every single field when
>> serializing
>> an object. So if you have the person object with say 10 fields you
>> generate 10 error message strings. That's a lot of Strings!
>
> Yes, that is obvious. I was wondering about the final modifiers
> because this is how I tend to write code and a while back, a number of
> my patches were shot down because "this does not adhere to the rest of
> the shindig code". If we start changing this, that is better.
>
> [...]
actually:
http://cwiki.apache.org/SHINDIGxSITE/java-style.html
final
Use final whenever possible on all member variables.
Reasoning: Avoids missing initialization.
However, since this is a library I would not make anything but static
inner classes final.
>
>> Well, then we'd have to duplicate code inside of each catch block.
>
> It does give you the opportunity to customize the error message per
> catch block and it actually reduces the amount of unneccessarily
> created strings to 0. I would be interested to see this under YourKit
> profiler with 5k req/sec; I would guess that you would see another
> performance increase.
>
Actually each catch block calls the private method with the proper
args. We're good, but yes we could change this to accommodate a
custom error message.
>> I'm pretty happy with the implementation as it stands.
>
> That does not mean it is optimal. :-)
Indeed!
Paul Lindner
plindner@hi5.com
Re: svn commit: r723705 -
Posted by "Henning P. Schmiedehausen" <he...@schmiedehausen.org>.
Paul Lindner <pl...@hi5.com> writes:
>The error string was generated for every single field when serializing
>an object. So if you have the person object with say 10 fields you
>generate 10 error message strings. That's a lot of Strings!
Yes, that is obvious. I was wondering about the final modifiers
because this is how I tend to write code and a while back, a number of
my patches were shot down because "this does not adhere to the rest of
the shindig code". If we start changing this, that is better.
[...]
>Well, then we'd have to duplicate code inside of each catch block.
It does give you the opportunity to customize the error message per
catch block and it actually reduces the amount of unneccessarily
created strings to 0. I would be interested to see this under YourKit
profiler with 5k req/sec; I would guess that you would see another
performance increase.
>I'm pretty happy with the implementation as it stands.
That does not mean it is optimal. :-)
Ciao
Henning
--
Henning P. Schmiedehausen - Palo Alto, California, U.S.A.
henning@schmiedehausen.org "We're Germans and we use Unix.
henning@apache.org That's a combination of two demographic groups
known to have no sense of humour whatsoever."
-- Hanno Mueller, de.comp.os.unix.programming
Re: svn commit: r723705 -
Posted by Paul Lindner <pl...@hi5.com>.
On Dec 5, 2008, at 10:29 AM, Henning P. Schmiedehausen wrote:
> lindner@apache.org writes:
>
>> Author: lindner
>> Date: Fri Dec 5 02:54:27 2008
>> New Revision: 723705
>
>> URL: http://svn.apache.org/viewvc?rev=723705&view=rev
>> Log:
>> Performance Patch from hi5
>> - errorMessage was being created for every getter json serialization
>> - on a heavily loaded server GC overhead went from 10% to 2%
>
> Where is the JIRA for it?
>
> What exactly made the GC overhead go down? Declaring the parameters
> final or moving the error string generation around?
>
The error string was generated for every single field when serializing
an object. So if you have the person object with say 10 fields you
generate 10 error message strings. That's a lot of Strings!
The final modifiers were added as a matter of course. If that makes
it hard for people to subclass then we can put them back. Modern JVMs
are pretty smart about final vs. non-final these days.
FWIW we ran the old code vs the new code with the YourKit profiler
with about 5k r/s, most of them rpc requests. The difference was huge.
> Wouldn't it make it much more sense to move the string generation into
> the catch blocks and keep the additional method out?
Well, then we'd have to duplicate code inside of each catch block.
I'm pretty happy with the implementation as it stands.
Paul Lindner
plindner@hi5.com
Re: svn commit: r723705 -
Posted by "Henning P. Schmiedehausen" <he...@schmiedehausen.org>.
lindner@apache.org writes:
>Author: lindner
>Date: Fri Dec 5 02:54:27 2008
>New Revision: 723705
>URL: http://svn.apache.org/viewvc?rev=723705&view=rev
>Log:
>Performance Patch from hi5
>- errorMessage was being created for every getter json serialization
>- on a heavily loaded server GC overhead went from 10% to 2%
Where is the JIRA for it?
What exactly made the GC overhead go down? Declaring the parameters
final or moving the error string generation around?
Wouldn't it make it much more sense to move the string generation into
the catch blocks and keep the additional method out?
Ciao
Henning
--
Henning P. Schmiedehausen - Palo Alto, California, U.S.A.
henning@schmiedehausen.org "We're Germans and we use Unix.
henning@apache.org That's a combination of two demographic groups
known to have no sense of humour whatsoever."
-- Hanno Mueller, de.comp.os.unix.programming