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

Implement streaming json responses #243

Merged
merged 5 commits into from May 19, 2016

Conversation

Projects
None yet
2 participants
@gnufied
Contributor

gnufied commented Feb 8, 2016

This pull request implements streaming json responses. Which user can configure via jolokia properties file.

Using streaming json I saw a drop by 39 times - in allocation sizes while an external python script performed 10 identical collections on the url. I did this numerous times and while numbers vary somewhat, it still holds true that using streaming json results in less allocations.

Should we choose to merge this, I will be happy to get tests sorted for the same. I am also sure, I have only covered jvm-agent output and this feature probably will not work for other use cases.

gc-profile1
gc-profile2

gnufied added some commits Feb 6, 2016

Implement support for chunked streaming of json data
This results in general saving in string allocation
that happens when using non-streaming responses
Add tests for ChunkedWriter
And improve tests for jolokia handler
@rhuss

This comment has been minimized.

Owner

rhuss commented Feb 15, 2016

Thans a lot, and sorry for the delay. 'was skiing in the Alpes for a week ;)

I will have a look at it, and will include it in the one way or other. Could be that it will be added to Jolokia 2.0, but let me have a look first. This might take until the end of the week, though ....

@gnufied

This comment has been minimized.

Contributor

gnufied commented Feb 15, 2016

Cheers. thanks for having a look. My motivation is to use this agent with cassandra cluster which uses ParNew GC and hence can result in stop-the-world gc pauses on young gc collection. I have profiled extensively with cassandra server and these changes have made young GC collections almost disappear.

The reason, we see less allocations is, there is lot of wasteful string creation when serializing full json strings. for example, when JSONObject serializes the Map and the key is a string, it undergoes following transformation:

String -> char array (because I can only write char array to Writer interface)
Char array -> String (because we are asking full json string back via JSONString which uses StringWriter)
String -> byte array (because final output is written to outputput stream )

This code removes lot of these redundant allocations. I haven't looked too closely at other endpoints (such as when ran within a container or proxy mode), since my own usecase uses jvm agent mode prominently. But I am happy to look. Also, do you think, it should be configured on per-request basis too?

@rhuss

This comment has been minimized.

Owner

rhuss commented May 4, 2016

was finally able to have a look, thanks a lot for the PR.

I wonder what would be the drawback to exclusively use streaming (and get rid of the old code). If streaming is always superior, we could get rid of the extra option and make that the default and only behaviour.

Do you see any problems in always using streaming ?

@gnufied

This comment has been minimized.

Contributor

gnufied commented May 4, 2016

At Yelp - we have been running modified version of Jolokia (with this patch enabled) on hundreds of machines and so far we haven't seen a problem.

My initial worry was - what if a particular HTTP client couldn't deal with chunked encoding and this patch broke some of the tooling we have. But after good amount of testing, I haven't seen any regressions.

@rhuss

This comment has been minimized.

Owner

rhuss commented May 4, 2016

ah, cool. So my strategy is now to enable streaming / chunking by default, add a (undocumented) option for reverting to the old behaviour in case of issues and remove the fallback code after some releases.

I will add this to 1.3.4.

Thanks again !

@rhuss rhuss merged commit cbad25f into rhuss:master May 19, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

rhuss added a commit that referenced this pull request May 19, 2016

#243 : Add to changelog, make streaming the default.
In case of problems, the options `streaming=false` can be used to fallback to the old behaviour. Also found a bug when creating the callback when using jsonp callback style (how do I love unit tests ;-)
@rhuss

This comment has been minimized.

Owner

rhuss commented Dec 7, 2017

@gnufied Hi ;-) Sorry for pinging you again, but I have a question about your contribution https://github.com/rhuss/jolokia/blob/master/agent/core/src/main/java/org/jolokia/util/ChunkedWriter.java . It looks like that it takes over some parts or is derived directly from java.nio.StreamEncoder .

If you can confirm this, we have to replace this as StreamEncoder is licensed under the GPL which is not compatible with the license we are using (APL).

Thanks, and sorry for bugging you again. Don't get me wrong, your contribution is very valuable but we need to get this sorted out. It would be awesome if you could reply soon.

thanks ...
... roland

@gnufied

This comment has been minimized.

Contributor

gnufied commented Dec 7, 2017

@rhuss I can confirm that bits of ChunkedWriter was used from StreamEncoder because it implements similar interface.

I can revisit deriving from StreamEncoder rather than using bits of code directly from the class. I think it should be possible. Let me look into that over the weekend.

@rhuss

This comment has been minimized.

Owner

rhuss commented Dec 7, 2017

@gnufied Awesome, thanks a lot !

@rhuss

This comment has been minimized.

Owner

rhuss commented Jan 19, 2018

@gnufied had you any change to look into this ?

headers.set("Content-Type", getMimeType(pParsedUri) + "; charset=utf-8");
String callback = pParsedUri.getParameter(ConfigKey.CALLBACK.getKeyValue());
pExchange.sendResponseHeaders(200, 0);
writer = new ChunkedWriter(pExchange.getResponseBody(), "UTF-8");

This comment has been minimized.

@rhuss

rhuss Jan 20, 2018

Owner

@gnufied one question: Why actually it is required to use a ChunkedWriter here ? Or stated otherwise: What would be the drawback to use the response body outputstream directly when streaming out the response ?

I'm still looking for a quick solution to get rid of the IP issue we have (and which has been escalate quite a bit), so if don't find a quick solution I'm going to remove ChunkedWriter altogether this weekend.

This comment has been minimized.

@rhuss

rhuss Jan 20, 2018

Owner

@gnufied I mean, replacing ChunkedWriter with OutputStreamWriter, what would we loose or what would be the issues ?

This comment has been minimized.

@rhuss

rhuss Jan 20, 2018

Owner

@gnufied If I understand the code right, then the only difference between a plain OutputStreamWriter and ChunkedWriter is, then the latter does always "full" flush on flush() whereas the OutputStreamWriter does this during close().

Why is this required ?

This comment has been minimized.

@gnufied

gnufied Jan 21, 2018

Contributor

There are other minor differences - ChunkedWriter writes a empty byte on flush , the OutputStreamWriter doesn't - https://github.com/rhuss/jolokia/blob/master/agent/core/src/main/java/org/jolokia/util/ChunkedWriter.java#L117

This comment has been minimized.

@gnufied

gnufied Jan 21, 2018

Contributor

So partly this is coming back to me(sorry I forgot bunch of this code) - the main reason we had to reimplement chunkedWriter from scratch is because of empty byte handling on flush.

StreamEncoder has OutputStream property marked as private OutputStreamWriter delegates to StreamEncoder for majority of its functionality anyways.

So the problem boils down to - we need to write the empty byte on flush, to mark it as end of the whole message. OutputStreamWriter does not do that and hence its chunking implementation is weirdly incomplete.

Now - in StreamEncoder because OutputStream property is private, a subclass can't access it to write that empty byte. Also directly calling streamEncoder.write(EMPTY_BYTE) will result in message getting buffered and that is not what we want, because this was supposed to be end of whole message. We want to be able to directly write the EMPTY byte to OuputputStream.

I think that was the most crucial reason. There may be something else but I don't remember it.

This comment has been minimized.

@rhuss

rhuss Jan 21, 2018

Owner

@gnufied thanks a lot for the explanation. However, I don't get why you need to write an empty byte array to the stream. Whit is this needed ?

Also, if I'm not completely wrong, this write is a no-op anyway:

private static final byte[] EMPTY = {};
...
out.write(EMPTY);

then in OutputStream:

  public void write(byte b[]) throws IOException {
        write(b, 0, b.length);
   }
   ....
 public void write(byte b[], int off, int len) throws IOException {
        if (b == null) {
            throw new NullPointerException();
        } else if ((off < 0) || (off > b.length) || (len < 0) ||
                   ((off + len) > b.length) || ((off + len) < 0)) {
            throw new IndexOutOfBoundsException();
        } else if (len == 0) {
             // This is where we end up when writing an empty array. So the original 
            // operation is a complete no-op. What did I miss ?
            return;
        }
        for (int i = 0 ; i < len ; i++) {
            write(b[off + i]);
        }
    } 

This comment has been minimized.

@gnufied

gnufied Jan 21, 2018

Contributor

I need to double check but that final EMPTY byte array causes "0\r\n\r\n" to be written to the message and that is essential for chunking to be finished.

@gnufied

This comment has been minimized.

Contributor

gnufied commented Jan 20, 2018

@rhuss I will look into this tonight. I haven't had a chance to look yet.

@rhuss

This comment has been minimized.

Owner

rhuss commented Jan 20, 2018

@gnufied cool, thanks ! sorry for bugging you, but let's get that elephant out of the room ;-) Going to make a 1.4.0 release short after the fix ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment