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

Faster json parsing #1980

Merged
merged 10 commits into from
May 17, 2017
Merged

Faster json parsing #1980

merged 10 commits into from
May 17, 2017

Conversation

pmlopes
Copy link
Contributor

@pmlopes pmlopes commented May 10, 2017

Implement a toBuffer to all json objects.

This PR extends #1976 and fixes the ip-check that was resolved using the github web interface which does not sign off commits.

amannocci and others added 6 commits May 8, 2017 16:38
@amannocci
Faster json parsing #1975
a4ec87b 
Signed-off-by: amannocci <adrien.mannocci@gmail.com>
@amannocci
Add convenient ctor #1975
75487fe 
Signed-off-by: amannocci <adrien.mannocci@gmail.com>
@amannocci
Remove InputStream constructor relate #1975
0ae9aa6 
Signed-off-by: amannocci <adrien.mannocci@gmail.com>
@amannocci
Remove unused imports in test #1975
147b3cd 
Signed-off-by: amannocci <adrien.mannocci@gmail.com>
@pmlopes
Merge branch 'master' into faster-json-parsing
cad2454 
Signed-off-by: Paulo Lopes <pmlopes@gmail.com>
@pmlopes
Add encode to buffer
f3e0802 
Signed-off-by: Paulo Lopes <pmlopes@gmail.com>
@pmlopes pmlopes mentioned this pull request May 10, 2017
Closed
@pmlopes
Fix merge conflicts
d9a38f3 
Signed-off-by: Paulo Lopes <pmlopes@gmail.com>
@pmlopes
Avoid outputstream
8888d0c 
Signed-off-by: Paulo Lopes <pmlopes@gmail.com>
@amannocci
Copy link
Contributor

Maybe we can directly write into a buffer ?

  public static Buffer encodeToBuffer(Object obj) throws EncodeException {
    try {
      Buffer buf = Buffer.buffer();
      mapper.writeValue(new ByteBufOutputStream(buf.getByteBuf()), obj);
      return buf;
    } catch (Exception e) {
      throw new EncodeException("Failed to encode as JSON: " + e.getMessage());
    }
  }

We need also to remove the method decodeValue based on InputStream.

public static <T> T decodeValue(InputStream stream, Class<T> clazz) throws DecodeException {

I will attempt to do a little jmh benchmark to confirm performance improvement before merge.
Refer to #1975

@amannocci
Copy link
Contributor

We can add a method with TypeReference too ?

public static <T> T decodeValue(Buffer buf, TypeReference<T> type) throws DecodeException

@pmlopes
Remove InputStream references, added decode to buffer with type refer…
4c8dcba 
…ence

Signed-off-by: Paulo Lopes <pmlopes@gmail.com>
@pmlopes
Copy link
Contributor Author

pmlopes commented May 11, 2017

@amannocci your comments have been added to the PR

@pmlopes pmlopes added this to the 3.4.2 milestone May 11, 2017
@amannocci
Copy link
Contributor

@pmlopes @vietj Sounds good for me.
Benchs are available on #1975

@pmlopes pmlopes requested review from vietj and jponge May 15, 2017 06:52
vietj
vietj reviewed May 15, 2017
View reviewed changes
Copy link
Member

@vietj vietj left a comment

Choose a reason for hiding this comment

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

a few doc changes before proceeding

src/main/java/io/vertx/core/json/Json.java Outdated
@@ -69,6 +72,16 @@ public static String encode(Object obj) throws EncodeException {
}
}

public static Buffer encodeToBuffer(Object obj) throws EncodeException {
Copy link
Member

Choose a reason for hiding this comment

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

missing doc

src/main/java/io/vertx/core/json/Json.java Outdated
@@ -93,6 +106,22 @@ public static String encodePrettily(Object obj) throws EncodeException {
}
}

public static <T> T decodeValue(Buffer buf, TypeReference<T> type) throws DecodeException {
Copy link
Member

Choose a reason for hiding this comment

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

missing doc

src/main/java/io/vertx/core/json/Json.java Outdated
}
}

public static <T> T decodeValue(Buffer buf, Class<T> clazz) throws DecodeException {
Copy link
Member

Choose a reason for hiding this comment

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

missing doc

src/main/java/io/vertx/core/json/JsonArray.java Outdated
@@ -545,6 +554,15 @@ public String encode() {
}

/**
* Encode this JSON object as a string.
*
* @return the string encoding.
Copy link
Member

Choose a reason for hiding this comment

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

the encoded string, string encoding means something else

src/main/java/io/vertx/core/json/JsonObject.java Outdated
@@ -764,6 +773,15 @@ public String encodePrettily() {
}

/**
* Encode this JSON object as a string.
*
* @return the string encoding.
Copy link
Member

Choose a reason for hiding this comment

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

the encoded string

@pmlopes
Added javadoc
008c793 
Signed-off-by: Paulo Lopes <pmlopes@gmail.com>
jponge
jponge reviewed May 15, 2017
View reviewed changes
Copy link
Member

@jponge jponge left a comment

Choose a reason for hiding this comment

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

I've had a look at @amannocci benchmarks at https://github.com/amannocci/vertx-bench-json/blob/master/src/main/java/io/vertx/bench/JsonBenchmark.java but I have concerns as it is difficult to evaluate wether Hotspot is doing DCE / partial constant propagation, etc.

@amannocci your benchmark methods should return some value, here the JSON object. See my article at http://www.oracle.com/technetwork/articles/java/architect-benchmarking-2266277.html

@jponge
Copy link
Member

jponge commented May 15, 2017

To be clear: I do think your changes boost performance, but I'd like a more rigorous evaluation in your benchmarks to know exactly by how much 😉 (and maybe you will just end up with the same figures)

@amannocci
Copy link
Contributor

I'm agree with you @jponge. I will try to improve the actual benchmark to be sure.

@amannocci
Copy link
Contributor

I have done some modifications on my benchmark as proposed by @jponge .
I get similar results.

# Run complete. Total time: 00:27:19

Benchmark                         Mode  Cnt       Score     Error  Units
JsonBenchmark.testNewJsonArray   thrpt  200   52004,058 ± 391,153  ops/s
JsonBenchmark.testNewJsonObject  thrpt  200  103620,960 ± 681,895  ops/s
JsonBenchmark.testOldJsonArray   thrpt  200   40164,809 ± 295,782  ops/s
JsonBenchmark.testOldJsonObject  thrpt  200   78245,268 ± 608,647  ops/s

Let me know if I'm wrong.

@jponge
Copy link
Member

jponge commented May 16, 2017

LGTM 👍

@amannocci
Copy link
Contributor

LGTM

@vietj
Copy link
Member

vietj commented May 17, 2017

can you rebase the branch so it can be merged ? thanks

@pmlopes
Copy link
Contributor Author

pmlopes commented May 17, 2017

it can be merged, why do we need to rebase?

@vietj
Copy link
Member

vietj commented May 17, 2017

I can see "This branch cannot be rebased due to conflicts" in github

@jponge
Copy link
Member

jponge commented May 17, 2017

@vietj "Merging can be performed automatically" 😉

@jponge jponge merged commit c6d84d3 into master May 17, 2017
@vietj vietj removed the to review label May 17, 2017
@vietj
Copy link
Member

vietj commented May 17, 2017

the UI was not really clear

@vietj vietj deleted the amannocci-faster-json-parsing branch May 17, 2017 21:47
@cescoffier cescoffier changed the title Amannocci faster json parsing Faster json parsing Jun 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jponge jponge jponge left review comments

@vietj vietj vietj left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.4.2
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@pmlopes @amannocci @jponge @vietj