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.

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

HSEARCH-2818 Optimise encoding of GSON elements into HTTP client buffers #1490

Merged
merged 4 commits into from Aug 11, 2017

Conversation

Projects
None yet
2 participants
@Sanne
Copy link
Member

commented Jul 15, 2017

Similar optimisations as proposed before but using a different approach, now more carefully taking into account the AWS digest requests.

Also taking into account flow control of the underlying buffers, this was a bug in the previous proposal.

https://hibernate.atlassian.net/browse/HSEARCH-2818

@Sanne Sanne referenced this pull request Jul 15, 2017

Closed

HSEARCH-2818 #1485

@Sanne Sanne force-pushed the Sanne:HSEARCH-2818 branch from 644a597 to 727821d Jul 16, 2017

@yrodiere yrodiere force-pushed the Sanne:HSEARCH-2818 branch from 727821d to 7a3c218 Jul 18, 2017

@yrodiere
Copy link
Member

left a comment

Nice job! I think some changes are required, be it only the typos, but I don't have strong opinions on the other changes, so feel free to dismiss the comments if you disagree.

*
* @author Sanne Grinovero (C) 2017 Red Hat Inc.
*/
public interface DigestSelfSigningCapable {

This comment has been minimized.

Copy link
@yrodiere

yrodiere Jul 18, 2017

Member

This is effectively used as an inter-module interface, so maybe we should promote it to SPI? You know, so that other implementors can also use request signing.

This comment has been minimized.

Copy link
@yrodiere

yrodiere Jul 18, 2017

Member

See also my comment on GsonHttpEntity#writeTo: we may not need this interface at all.

This comment has been minimized.

Copy link
@Sanne

Sanne Jul 31, 2017

Author Member

having the explicit hint that this class is being invoked for the purpose of the digest computation helps a lot in making it efficient..

Could make it SPI but IMO it's borderline as something we'd not support others to use / rely on.
Is any invocation "across our modules" needing to be an SPI ?

This comment has been minimized.

Copy link
@yrodiere

yrodiere Aug 1, 2017

Member

having the explicit hint that this class is being invoked for the purpose of the digest computation helps a lot in making it efficient..

I don't understand how; there doesn't seem to be any particular optimization in the digest writing code. Except the content length computing, which isn't really related to signing and could be implemented without a separate interface.
On the other hand, if, instead of having a separate interface, we leveraged the writeTo(Outpustream) method:

  1. We could simply use a standard writer wrapped around the output stream and directly write all the JSON to it, or use our own writer if you feel we can optimize encoding.
  2. We could use a wrapper around the OutputStream to detect the content length if necessary.
  3. The AWS module would only have to pass a DigestOutputStream (a standard JDK class) to compute its digest.
  4. Other modules (say, a module implementing signing for another cloud) could very simply do the same without having to rely on implementation classes.

Is any invocation "across our modules" needing to be an SPI ?

I would think so, otherwise we begin to have uncontrolled dependencies between modules. Which is a bad practice in my opinion, but more importantly is a pain as soon as we control imports/exports more rigorously, like in OSGi or (maybe?) in Java 9.

This comment has been minimized.

Copy link
@Sanne

Sanne Aug 11, 2017

Author Member

I've tried such things before. It didn't work out for various reasons.

  1. the writeTo(Outpustream) is invoked in a different context by the http client and you can't do flow control in that case - not without buffering which is what I'm working to avoid.

We could use a wrapper around the OutputStream to detect the content length if necessary.

That's too late. It's going to need the content lenght before the content is being produced, as the content lenght needs to be written in the output stream first thing, and also it might affect how the rest of the content is actually encoded (chunked, buffered, etc..).

Also you can't report a different lenght after you've produced the content as that gets you in trouble (bug of the client?).

private void writeOrBufferWrite(ByteBuffer toWriteBytes) throws IOException {
totalWrittenBytes += toWriteBytes.remaining();
flushPendingBuffers();
if ( flowControlPushingBack == false ) {

This comment has been minimized.

Copy link
@yrodiere

yrodiere Jul 18, 2017

Member

What's wrong with the "!" operator? O_o

This comment has been minimized.

Copy link
@Sanne

Sanne Jul 31, 2017

Author Member

It's not as readable as an explicit == false :)

This comment has been minimized.

Copy link
@yrodiere

yrodiere Aug 1, 2017

Member

Ok. There's no accounting for taste, as they say.

flushPendingBuffers();
if ( flowControlPushingBack == false ) {
//If it wasn't all written, buffer for later
if ( fullyWrite( toWriteBytes ) == false ) {

This comment has been minimized.

Copy link
@yrodiere

yrodiere Jul 18, 2017

Member

What's wrong with the "!" operator? O_o

Same here

}

private void flushPendingBuffers() throws IOException {
while ( flowControlPushingBack == false && unwrittenLazyBuffers != null && unwrittenLazyBuffers.isEmpty() == false ) {

This comment has been minimized.

Copy link
@yrodiere

yrodiere Jul 18, 2017

Member

What's wrong with the "!" operator? O_o

Same here

this.digest.update( NEWLINE );
}

public long getContentLenght() {

This comment has been minimized.

Copy link
@yrodiere

yrodiere Jul 18, 2017

Member

Typo here, should be getContentLength

/**
* We don't want to compute the length in advance as it would defeat all optimisations.
* Still it's possible that we happen to find out, for example if a Digest from all
* content needs to be computed.

This comment has been minimized.

Copy link
@yrodiere

yrodiere Jul 18, 2017

Member

Note that since the AWS signature is computed last (after all other headers have been added), it's very likely that the content-length header won't be added to the request anyway (because the interceptor doing that is executed before the AWS interceptor).
I don't see a reliable solution to this problem, though, so we can leave it at that. At least service providers will have the option of adding the content-length header themselves after having computed the digest.

This comment has been minimized.

Copy link
@Sanne

Sanne Jul 31, 2017

Author Member

This was actually buggy and I figured it out later: once the content length is provided we need to stick to it or the http client driver gets seriously confused. The new design is better and guarantees a "stable" response w/o drawbacks.


@Override
public void close() throws IOException {
//Nothing to close

This comment has been minimized.

Copy link
@yrodiere

yrodiere Jul 18, 2017

Member

Shouldn't you reset nextBodyToEncodeIndex, writer and such here? So as to actually be "repeatable"? Otherwise a half-consumed entity may end up producing garbage when it's next consumed...

This comment has been minimized.

Copy link
@Sanne

Sanne Jul 31, 2017

Author Member

that's not possible: the async http client expects to be able to call us multiple times and resume the data stream from the last time, so I can't just reset for the sake of it.

This comment has been minimized.

Copy link
@yrodiere

yrodiere Aug 1, 2017

Member

I would expect the client not to call close between two calls... Otherwise how can you be repeatable at all? You'd force the client to consume everything, whatever happens, in order for him to be able to consume content twice?

Also, quoting the javadoc of HttpAsyncContentProducer#isRepeatable:

Determines whether or not this producer is capable of producing
its content more than once. Repeatable content producers are expected
to be able to recreate their content even after having been closed.

Which, in my opinion, implies that after the producer has been closed, we expect it to be forced to recreate content, and not to continue the previously ongoing writes.

Mind you, if you really don't want to do that, an alternate solution would be to simply return "false" in isRepeatable, warning clients that this entity can only produce content once.

public void writeTo(OutputStream outstream) throws IOException {
//This could be implemented but would be sub-optimal compared to using produceContent().
//We therefore prefer throwing the exception so that we can easily spot unintended usage via tests.
throw new UnsupportedOperationException( "Not implemented! Expected to produce content only over produceContent()" );

This comment has been minimized.

Copy link
@yrodiere

yrodiere Jul 18, 2017

Member

Seems to me this could be implemented quite efficiently (memory-wise), especially if the client is responsible of passing a buffered stream (if needed) and if we don't add a buffered wrapper ourselves.

Of course it's still a blocking call, but that's the choice of the caller, and it may be useful sometimes. In particular, unless I'm mistaken, this method could be used in the AWS module to generate a digest: the client would provide a DigestOutputStream very similar to your DigestWriter below, this method would wrap it in an OutputStreamWriter and just push all the JSON to it.

Note I'm not saying we must do it, I just wanted to check with you if this could get us rid of the DigestSelfSigningCapable interface, and also if you think it's worth it.

This comment has been minimized.

Copy link
@Sanne

Sanne Jul 31, 2017

Author Member

I'll not do it as the code is already very complex, and this method isn't even ever called so not worth the effort..

This comment has been minimized.

Copy link
@Sanne

Sanne Aug 11, 2017

Author Member

BTW I don't want to buffer w/o boundarier. I'm using a buffer of a very specific size which I know is a good balance for our use case.

}

@Test
public void testDigestToTriggerLenghtComputation() {

This comment has been minimized.

Copy link
@yrodiere

yrodiere Jul 18, 2017

Member

typo here, should be testDigestToTriggerLengthComputation

@Sanne Sanne force-pushed the Sanne:HSEARCH-2818 branch 2 times, most recently from e0a396b to 6e93331 Jul 31, 2017

updated

@Sanne

This comment has been minimized.

Copy link
Member Author

commented Jul 31, 2017

I've updated it, fixed the typos and applied several more optimisations.

The previous version would avoid all allocations "out of TLAB" - which are the most expensive.
After the second round, we don't see any allocations within TLAB either - the only remaining significant allocations are performed during response parsing.

@yrodiere yrodiere self-assigned this Aug 1, 2017

@yrodiere
Copy link
Member

left a comment

I tested the patch, and the results are indeed impressive: the allocation volume is about ten times smaller!
I added a few comments here and there. I think we won't be able to agree on all of them, so feel free to ignore those you disagree with and merge anyway.


@Override
public void close() throws IOException {
//Nothing to close

This comment has been minimized.

Copy link
@yrodiere

yrodiere Aug 1, 2017

Member

I would expect the client not to call close between two calls... Otherwise how can you be repeatable at all? You'd force the client to consume everything, whatever happens, in order for him to be able to consume content twice?

Also, quoting the javadoc of HttpAsyncContentProducer#isRepeatable:

Determines whether or not this producer is capable of producing
its content more than once. Repeatable content producers are expected
to be able to recreate their content even after having been closed.

Which, in my opinion, implies that after the producer has been closed, we expect it to be forced to recreate content, and not to continue the previously ongoing writes.

Mind you, if you really don't want to do that, an alternate solution would be to simply return "false" in isRepeatable, warning clients that this entity can only produce content once.

*
* @author Sanne Grinovero (C) 2017 Red Hat Inc.
*/
public interface DigestSelfSigningCapable {

This comment has been minimized.

Copy link
@yrodiere

yrodiere Aug 1, 2017

Member

having the explicit hint that this class is being invoked for the purpose of the digest computation helps a lot in making it efficient..

I don't understand how; there doesn't seem to be any particular optimization in the digest writing code. Except the content length computing, which isn't really related to signing and could be implemented without a separate interface.
On the other hand, if, instead of having a separate interface, we leveraged the writeTo(Outpustream) method:

  1. We could simply use a standard writer wrapped around the output stream and directly write all the JSON to it, or use our own writer if you feel we can optimize encoding.
  2. We could use a wrapper around the OutputStream to detect the content length if necessary.
  3. The AWS module would only have to pass a DigestOutputStream (a standard JDK class) to compute its digest.
  4. Other modules (say, a module implementing signing for another cloud) could very simply do the same without having to rely on implementation classes.

Is any invocation "across our modules" needing to be an SPI ?

I would think so, otherwise we begin to have uncontrolled dependencies between modules. Which is a bad practice in my opinion, but more importantly is a pain as soon as we control imports/exports more rigorously, like in OSGi or (maybe?) in Java 9.

//can calculate the accurate size upfront.
//Computing this size after multiple buffers is possible but not helpful,
//as we'll already be using the chunked encoding mode.
long totalWrittenBytes = 0;

This comment has been minimized.

Copy link
@yrodiere

yrodiere Aug 1, 2017

Member

The value of this member is never read. There must be a problem somewhere.

This comment has been minimized.

Copy link
@Sanne

Sanne Aug 11, 2017

Author Member

thanks, I'll remove it. It's just a leftover from a previous approach which didn't work out.

*/
public final class GsonHttpEntity implements HttpEntity, HttpAsyncContentProducer, DigestSelfSigningCapable {

private static byte[] NEWLINE = "\n".getBytes( StandardCharsets.UTF_8 );

This comment has been minimized.

Copy link
@yrodiere

yrodiere Aug 1, 2017

Member

This should be final.

private void writeOrBufferWrite(ByteBuffer toWriteBytes) throws IOException {
totalWrittenBytes += toWriteBytes.remaining();
flushPendingBuffers();
if ( flowControlPushingBack == false ) {

This comment has been minimized.

Copy link
@yrodiere

yrodiere Aug 1, 2017

Member

Ok. There's no accounting for taste, as they say.

}
//The original CharBuffer is not safe as it's backed by a char array
//which happens to be the GSON write buffer: it will get mutated.
CharBuffer defensiveCopy = CharBuffer.allocate( toWriteLater.remaining() );

This comment has been minimized.

Copy link
@yrodiere

yrodiere Aug 1, 2017

Member

It may be silly, but... since you have to perform an allocation anyway, why not directly allocate ByteBuffers, and directly encode the characters in there? You could then remove this list of char buffers, and use a list of ByteBuffers instead. This would mean turning "needsWritingBuffer" into a list, if you will: it would initially only contain one buffer, and would grow as necessary.

I tested it a bit with the performance tests, and it seems having a lot of "unwritten lazy buffers" is not rare; I was above the 16 buffer initial capacity about 25% of the time, I think, going all the way up to more than 50 buffers. So it could be useful.
But more importantly, I wonder if it couldn't make the code a bit simpler. Do you think it could?

This comment has been minimized.

Copy link
@Sanne

Sanne Aug 11, 2017

Author Member

I'm not sure I understand what you'd hope to achieve. I'd love to simplify the code a bit but not at cost of performance, and I'm not sure if you've considered all aspect .. for example that you actually don't know how large the bytebuffers need to be as that depends on the encoding details, and guessing gets you into re-sizing trouble.

I need to wrap this up, let's apply this as-is but then if you want to play with it and show me I'm happy to review some follow-up polishing PR.

hintContentLenght( digestWriter.getContentLenght() );
}

private void hintContentLenght(long contentLenght) {

This comment has been minimized.

Copy link
@yrodiere

yrodiere Aug 1, 2017

Member

Typo: "hintContentLenght" => "hintContentLength", "contentLenght" => "contentLength"

@Sanne Sanne force-pushed the Sanne:HSEARCH-2818 branch from 6e93331 to ed8ef23 Aug 11, 2017

@Sanne

This comment has been minimized.

Copy link
Member Author

commented Aug 11, 2017

I've rebased it and fixed the problems with missing static final and what you highlighted about close() not guaranteeing to make it repeatable.

Also promoted that disgest interface to SPI as suggested.

I didn't change the essential strategy around buffers as I'm not seeing a clearly better idea. Happy to be proven wrong but let's split that out as a different PR if you think it's worth trying.

@yrodiere yrodiere force-pushed the Sanne:HSEARCH-2818 branch from ed8ef23 to 7dbdcc8 Aug 11, 2017

@yrodiere yrodiere merged commit 7dbdcc8 into hibernate:master Aug 11, 2017

1 check was pending

default Build triggered. sha1 is merged.
Details
@yrodiere

This comment has been minimized.

Copy link
Member

commented Aug 11, 2017

Merged, thanks! Good to see this one in :)
I'll try to submit a follow-up PR to show what I was talking about.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.