-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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.
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
Improve toString Performance: Use StringBuilderWriter for toString methods #867
Improve toString Performance: Use StringBuilderWriter for toString methods #867
Conversation
resulting in a faster toString generation.
(cherry picked from commit df0e3e9)
@Simulant87 The benchmarks look good, but I tried testing against a 30m JSON file. The improvement was less than 2% between the latest master branch code (943 ms) and this PR (927 ms). I don't think that is enough to justify accepting this PR. If you want to run the test case yourself, I used the file |
@stleary Thank you for having a look at my PR and sharing your source file for your performance test. I repeated the performance test with your input file and wrote this "poor mans performance test" (see below). It was just faster to implement than a correct JMH setup and it should still give an approximate feelingabout the difference. I executed it on master and my branch with the following result: master branch: the improvement is comparable to the java-json-benchmark I used before with ~35% improvement. package org.json;
import java.io.FileInputStream;
import java.io.FileNotFoundException;
import java.io.InputStream;
public class Main {
public static void main(String[] args) throws FileNotFoundException {
// TODO: update path to 30mb input file train-v1.1.json from https://www.kaggle.com/datasets/stanfordu/stanford-question-answering-dataset
InputStream inputStream = new FileInputStream("/path/to/train-v1.1.json");
JSONTokener tokener = new JSONTokener(inputStream);
JSONObject object = new JSONObject(tokener);
String output = null;
// JVM warm up: 100 iterations of toString
for (int i = 0; i < 100; i++) {
output = object.toString();
System.out.println(output.charAt(0));
}
// measured run: 200 iterations of toString
long start = System.currentTimeMillis();
int iterations = 200;
for (int i = 0; i < iterations; i++) {
output = object.toString();
// access the to string output, so it does not get optimised away
System.out.println(output.charAt(0));
}
long end = System.currentTimeMillis();
long totalTime = end - start;
System.out.println(totalTime + "ms for " + iterations + " iterations of toString. " + totalTime / iterations + "ms per toString on average.");
}
} |
@Simulant87 I think that measuring how fast
|
@stleary You are right, in my performance improvement I only focused on the |
replacing a switch-case statement with few branches by if-else cases
@stleary I also took a look into the deserialisation parsing a JSONObject from String. I was able to pull of a ~30% improvement by replacing Performance checks I did with java-json-benchmark Here is the result for the benchmark on my local machine (thoughput measured in operations per second, higher is better):
improvement on this branch:
Also doing my simple performance test on your large JSON input file shows an improvement, although it is not that large as on the benchmark above: on master branch: Improvement on my branch:
|
What problem does this code solve? Does the code still compile with Java6? Risks Changes to the API? Will this require a new release? Should the documentation be updated? Does it break the unit tests? Was any code refactored in this commit? Review status Starting 3-day comment window |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed and tested locally, ok.
Implementation for #863
Using a
StringBuilder
wrapped in a newStringBuilderWriter
class to improve the performance for the defaulttoString
methods.using the not synchronised
StringBuilder
instead of theStringBuffer
results in a ~37% fastertoString
method, which is mostly used for serialisation, so this improvement is valuable for any service communication receiving JSON from this library.I validated the performance with this project: https://github.com/fabienrenaud/java-json-benchmark
./run ser --libs ORGJSON
Here is the result for the benchmark on my local machine (thoughput measured in operations per second, higher is better):
currently latest library version 20240205:
Improved implementation from this PR: