Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Optimizes opt* functions #337

Merged
merged 12 commits into from
May 23, 2017
Merged

Optimizes opt* functions #337

Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
a8d4e47
adjustments to opt methods in reference to https://github.com/stleary…
May 16, 2017
bd4b180
Support for float to BigDecimal in optBigDecimal
May 17, 2017
c46774c
* Update opt* methods for JSONArray
May 17, 2017
fcdb867
grr, forgot to save changes on last commit
May 17, 2017
0c7bd72
fixes for javadoc
May 17, 2017
382f62e
* Prevent exceptions in cases where the value is not a string.
May 18, 2017
c28a2bd
* reverts changes to getDouble and related optDouble and optFloat
May 18, 2017
1ab5260
* Adds methods getNUmber and getFloat to JSONArray and JSONObject
May 18, 2017
a7f8ff2
correct string check for JSONObject optBigDecimal and optBigInteger
May 18, 2017
849b392
updates the getNumber/optNumber to not return invalid Doubles
May 18, 2017
04d6e83
* Missed JSONArray optFloat and optDouble for the revert
May 19, 2017
757b6ed
Merge branch 'master' of github.com:stleary/JSON-java into OptimizeOpt
May 21, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
113 changes: 94 additions & 19 deletions JSONObject.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -540,7 +540,7 @@ public BigInteger getBigInteger(String key) throws JSONException {
return new BigInteger(object.toString());
} catch (Exception e) {
throw new JSONException("JSONObject[" + quote(key)
+ "] could not be converted to BigInteger.");
+ "] could not be converted to BigInteger.", e);
}
}

Expand All @@ -556,11 +556,14 @@ public BigInteger getBigInteger(String key) throws JSONException {
*/
public BigDecimal getBigDecimal(String key) throws JSONException {
Object object = this.get(key);
if (object instanceof BigDecimal) {
return (BigDecimal)object;
}
try {
return new BigDecimal(object.toString());
} catch (Exception e) {
throw new JSONException("JSONObject[" + quote(key)
+ "] could not be converted to BigDecimal.");
+ "] could not be converted to BigDecimal.", e);
}
}

Expand All @@ -578,10 +581,10 @@ public double getDouble(String key) throws JSONException {
Object object = this.get(key);
try {
return object instanceof Number ? ((Number) object).doubleValue()
: Double.parseDouble((String) object);
: new BigDecimal((String) object).doubleValue();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Introducing a BigDecimal to parse a double will add too much overhead for no reason, if you try to represent a double from a String it will stay as is, if it was indeed a double then nothing will be gained or lost, you are only adding more GC overhead.

Same thing for your other BigDecimal introduction when working with float or double

Copy link
Contributor Author

@johnjaylward johnjaylward May 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, Double.parse will throw an exception if the number can not be represented as a double.
From the javadocs:

double java.lang.Double.parseDouble(String s) throws NumberFormatException

Returns a new double initialized to the value represented by the specified String, as performed by the valueOf method of class Double.
Parameters: s the string to be parsed.
Returns:the double value represented by the string argument.
Throws: NullPointerException - if the string is null
NumberFormatException - if the string does not contain a parsable double.

Since:1.2
See Also:java.lang.Double.valueOf(String)

Double.valueOf seems to do something closer to what you are trying to accomplish, but it would be the same amount of garbage collection.

new BigDecimal(String) will always parse the value as long as it's a valid decimal number (i.e. not "abc"). The doubleValue call will then do a narrowing conversion which may change the value to -Inf or +Inf.

Lastly, this change would be out of scope for this PR. the get* methods were not modified here, only the opt*. If you feel we should evaluate it further, feel free to open another pull request with the changes and notes to performance impacts.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new BigDecimal(...) and Double.parseDouble(...) have the same behavior, an input that fails for parseDouble will also fail with new BigDecimal, the JavaDoc have different wording but the end result will be the same.

As for relevance, you are trying to make opt* operations faster and yet you are introducing an expensive and unneeded instantiation when the return value is a native (small d)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The JavaDoc is what to go by. If someone is running on a different VM with a different implementation, they may not function the same.

The BigDecimal conversion is not that expensive. I'd like to see some stats on it before making a change like that. We have no guarantees on the input. Just because someone asks for an int does not mean I would want to use Integer.parse.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I'm not "introducing" the big decimal conversion. It was there before the refactor.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but then int typecast would be wrong for my proposed shortcut but it can be easily mitigated by checking if the double value is between Integer.MIN_VALUE and Integer.MAX_VALUE and if it isn't throw an exception.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it's an opt* function. The entire point is that it does not throw an exception. I'm not disagreeing that the parsing performance can be improved. But I think it needs more than a cursory glance and wishful thinking.

This PR is focusing strictly on the request made in #337 which was to mitigate the Exception handling impact.

Any parsing impacts should be in a new PR after this one is merged.

Any narrowing coercion has the potential to change the number. This is a given and also why I added the optNumber call to the API.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Narrowing will not change the double for long, maybe for int, but then it is the same for BigDecimal calls, the reason I commented into this particular change is because of what I'm seeing:

-                    : Double.parseDouble((String) object);
+                    : new BigDecimal((String) object).doubleValue();

Am I reading this incorrectly? I'm seeing Double.parseDouble(...) changed to new BigDecimal(...) in this commit.

Narrowing will cause loss always, but the same will happen with BigDecimal hence my point of not bothering with such overhead at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let me check that. If I did change it, I'll likely change it back.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I must have changed that by accident when I was testing stuff and didn't notice. I'll revert that change back and update the respective optDouble and optFloat calls that I copied that to.

As for the narrowing, it is handled differently between BigDecimal and Double. Also, not all Long values fit in a double. see the test case changes @ stleary/JSON-Java-unit-test#71 that I updated today. A Double only has 53 bits for it's mantissa which will not let any value over 2^53 be represented correctly.

} catch (Exception e) {
throw new JSONException("JSONObject[" + quote(key)
+ "] is not a number.");
+ "] is not a number.", e);
}
}

Expand All @@ -602,7 +605,7 @@ public int getInt(String key) throws JSONException {
: Integer.parseInt((String) object);
} catch (Exception e) {
throw new JSONException("JSONObject[" + quote(key)
+ "] is not an int.");
+ "] is not an int.", e);
}
}

Expand Down Expand Up @@ -659,7 +662,7 @@ public long getLong(String key) throws JSONException {
: Long.parseLong((String) object);
} catch (Exception e) {
throw new JSONException("JSONObject[" + quote(key)
+ "] is not a long.");
+ "] is not a long.", e);
}
}

Expand All @@ -678,7 +681,7 @@ public static String[] getNames(JSONObject jo) {
int i = 0;
while (iterator.hasNext()) {
names[i] = iterator.next();
i += 1;
i++;
}
return names;
}
Expand Down Expand Up @@ -933,7 +936,15 @@ public boolean optBoolean(String key) {
* @return The truth.
*/
public boolean optBoolean(String key, boolean defaultValue) {
Object val = this.opt(key);
if (NULL.equals(val)) {
return defaultValue;
}
if (val instanceof Boolean){
return ((Boolean) val).booleanValue();
}
try {
// we'll use the get anyway because it does string conversion.
return this.getBoolean(key);
} catch (Exception e) {
return defaultValue;
Expand Down Expand Up @@ -965,8 +976,23 @@ public double optDouble(String key) {
* @return An object which is the value.
*/
public BigInteger optBigInteger(String key, BigInteger defaultValue) {
Object val = this.opt(key);
if (NULL.equals(val)) {
return defaultValue;
}
if (val instanceof BigInteger){
return (BigInteger) val;
}
if (val instanceof BigDecimal){
return ((BigDecimal) val).toBigInteger();
}
try {
return this.getBigInteger(key);
// the other opt functions handle implicit conversions, i.e.
// jo.put("double",1.1d);
// jo.optInt("double"); -- will return 1, not an error
// this conversion to BigDecimal then to BigInteger is to maintain
// that type cast support that may truncate the decimal.
return new BigDecimal(val.toString()).toBigInteger();
} catch (Exception e) {
return defaultValue;
}
Expand All @@ -984,8 +1010,25 @@ public BigInteger optBigInteger(String key, BigInteger defaultValue) {
* @return An object which is the value.
*/
public BigDecimal optBigDecimal(String key, BigDecimal defaultValue) {
Object val = this.opt(key);
if (NULL.equals(val)) {
return defaultValue;
}
if (val instanceof BigDecimal){
return (BigDecimal) val;
}
if (val instanceof BigInteger){
return new BigDecimal((BigInteger) val);
}
if (val instanceof Double){
return new BigDecimal(((Double) val).doubleValue());
}
if (val instanceof Long || val instanceof Integer
|| val instanceof Short || val instanceof Byte){
return new BigDecimal(((Number) val).longValue());
}
try {
return this.getBigDecimal(key);
return new BigDecimal(val.toString());
} catch (Exception e) {
return defaultValue;
}
Expand All @@ -1003,11 +1046,21 @@ public BigDecimal optBigDecimal(String key, BigDecimal defaultValue) {
* @return An object which is the value.
*/
public double optDouble(String key, double defaultValue) {
try {
return this.getDouble(key);
} catch (Exception e) {
Object val = this.opt(key);
if (NULL.equals(val)) {
return defaultValue;
}
if (val instanceof Number){
return ((Number) val).doubleValue();
}
if (val instanceof String) {
try {
return new BigDecimal((String) val).doubleValue();
} catch (Exception e) {
return defaultValue;
}
}
return defaultValue;
}

/**
Expand Down Expand Up @@ -1035,11 +1088,22 @@ public int optInt(String key) {
* @return An object which is the value.
*/
public int optInt(String key, int defaultValue) {
try {
return this.getInt(key);
} catch (Exception e) {
Object val = this.opt(key);
if (NULL.equals(val)) {
return defaultValue;
}
if (val instanceof Number){
return ((Number) val).intValue();
}

if (val instanceof String) {
try {
return new BigDecimal(val.toString()).intValue();
} catch (Exception e) {
return defaultValue;
}
}
return defaultValue;
}

/**
Expand Down Expand Up @@ -1093,11 +1157,22 @@ public long optLong(String key) {
* @return An object which is the value.
*/
public long optLong(String key, long defaultValue) {
try {
return this.getLong(key);
} catch (Exception e) {
Object val = this.opt(key);
if (NULL.equals(val)) {
return defaultValue;
}
if (val instanceof Number){
return ((Number) val).longValue();
}

if (val instanceof String) {
try {
return new BigDecimal(val.toString()).longValue();
} catch (Exception e) {
return defaultValue;
}
}
return defaultValue;
}

/**
Expand Down Expand Up @@ -1583,7 +1658,7 @@ public static Object stringToValue(String string) {
return d;
}
} else {
Long myLong = new Long(string);
Long myLong = Long.valueOf(string);
if (string.equals(myLong.toString())) {
if (myLong.longValue() == myLong.intValue()) {
return Integer.valueOf(myLong.intValue());
Expand Down