Skip to content
This repository has been archived by the owner on Nov 10, 2018. It is now read-only.

Cache JsonProvider.provider() result as per javadoc recommendation #78

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

gbloisi
Copy link

@gbloisi gbloisi commented Mar 18, 2018

I got through this because I found a performance hostpot in calling Json methods (about 500 microsec per call on a i5)

@rogersnm
Copy link

rogersnm commented Apr 6, 2018

+1, I'm serialising a large object and it takes about 2000ms on my machine currently. Switching to this implementation drops it down to 50ms.

@keilw
Copy link
Member

keilw commented Apr 6, 2018

See jakartaee/jsonp-api#80 it seems there already is a similar PR in Jakarta EE. I doubt that this repository results in any new updates or releases.

@m0mus m0mus requested review from lukasj and bravehorsie April 6, 2018 15:49
@m0mus
Copy link
Member

m0mus commented Apr 6, 2018

@lukasj @bravehorsie Recently there were some similar PRs in different projects. I am not 100% convinced that it's needed. Folks, I want you to review it before letting it go.

@@ -81,7 +81,8 @@
* threads.
*/
public final class Json {

static final JsonProvider PROVIDER = JsonProvider.provider();
Copy link
Member

Choose a reason for hiding this comment

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

Currently this would actually decrease performance with default implementation if JsonProvider is cached by client application (as it is supposed to). See

private final BufferPool bufferPool = new BufferPoolImpl();

When JsonProvider instance is cached like this buffer pool exists only once. Because it uses ConcurrentLinkedQueue it will decrease performance in multithreaded environment because of synchronization.

Copy link
Member

@keilw keilw Apr 9, 2018

Choose a reason for hiding this comment

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

I certainly see dangers making it final. That way for the lifetime of the Json class in a particular JVM it would remain the same even if the underlying provider may change (less likely without OSGi but possible for some implementations)
Keeping a static cache, maybe, as long as there's at least a protected or similar way to reset it, see how we did in JSR 363 with ServiceProvider or CDI.
I am not convinced we still need this in Java EE 8, maybe leave to a future (Jakarta EE) version of JSON-P?

Copy link
Member

@lukasj lukasj left a comment

Choose a reason for hiding this comment

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

same comments as in jakartaee/jsonp-api#80 apply here

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants