search

jakartaee / jsonp-api

Jakarta JSON Processing
https://eclipse.org/ee4j/jsonp
Other
141 stars 61 forks source link

Issue #205 - Redo of PR #206 #218

Closed JohnTimm closed 4 years ago

JohnTimm commented 4 years ago

Signed-off-by: John T.E. Timm johntimm@us.ibm.com

JohnTimm commented 4 years ago

@lukasj @m0mus @alexsm82

Please review this PR.

I had to redo PR #206 based on the current version of master including the javax.json -> jakarta.json package naming change. I also added JsonParserCachedProviderTest which is identical to JsonParserTest but uses the new Json.setProvider method.

[INFO] Running org.glassfish.json.tests.JsonParserTest
[INFO] Tests run: 49, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 22.303 s - in org.glassfish.json.tests.JsonParserTest
[INFO] Running org.glassfish.json.tests.JsonParserCachedProviderTest
[INFO] Tests run: 49, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 14.976 s - in org.glassfish.json.tests.JsonParserCachedProviderTest
leadpony commented 4 years ago

@JohnTimm Sorry for delayed reply. I will provide the pointer to the original benchmark code.

JohnTimm commented 4 years ago

I do not think Json class is the right place to fix the problem described in #205 . The problem, in my opinion, lies in the implementation which is not following what the API states - implementation is not caching provider while the API (JsonProvider.provider()) recommends doing so. This also means that the implementation should not be calling "Json" API but reuse its knowledge of implementation internals and use direct access to classes it knows. Ie in JsonPatchImpl.getPointer(...) replace return Json.createPointer(pointerString.getString()); by return new JsonPointerImpl(pointerString.getString()). There are more places suffering by this...

@lukasj I don't disagree. I think that it is better to fix internally as you have suggested. I was going for expediency in my solution. Any idea on the effort involved on fixing this problem the right way?

lukasj commented 4 years ago

Any idea on the effort involved on fixing this problem the right way?

The quick way would be to do Search & Replace "Json." by "new SomeImpl" (or similar) in the implementation. This should eliminate unnecessary provider lookups and speed up things a bit with a little (time/effort) investment. In a second iteration, some caching could be introduced on places in the implementation where it makes sense but this would require more time (=read as "probably can wait a bit").

JohnTimm commented 4 years ago

The quick way would be to do Search & Replace "Json." by "new SomeImpl" (or similar) in the implementation.

@lukasj I will consider submitting a new PR that takes this approach (likely the week after next).

leadpony commented 4 years ago

@JohnTimm I pushed the original benchmaking code to jsonp-benchmark in my repositories. This personal project is for comparing performance of 3 distinct implementations of JSON-P API: this one, Apache Johnzon, and Joy.

You can run the benchmark of JsonPatch as shown below:

mvn clean package
java -jar target/benchmarks.jar apply

Please note that the original benchmark code uses the JSON-P API 1.1, that is still in javax.json package. You can freely copy and modify the code under the original License (Apache).

P.S. I believe the simplest way to gain better performance is replacing the implementation with Joy.:wink:

JohnTimm commented 4 years ago

I pushed the original benchmaking code to [jsonp-benchmark](https://github.com/leadpony

@leadpony Thanks for putting that out there. I appreciate it.

lukasj commented 4 years ago

Replaced "Json." by "new SomeImpl" in #244 . If something more is needed, create new issue/PR, please. Thanks.