Support for Streams and Iterators#2067
Conversation
|
To give some more context: I tested this idea in a concrete use-case at work. It allows to serve a particularly big GQL request by allocating half of the memory (max heap usage down from 4.1GB to just 2GB). Since the domain model contains some very big POJOs (i.e. with hundreds of properties) the ability to lazily allocate those in a |
6bf6a87 to
a14f55c
Compare
There was a problem hiding this comment.
This is a mild breaking change - the input is "Object" but the actual type has changed.
I think this is ok (did we say it WOULD always be a collection??) but I just want to call it out and see others thoughts on this.
There was a problem hiding this comment.
yes, this could be a breaking change.
Arrays are reallocated anymore as List (but I could easily re-introduce this behavior).
For Collections we should be fine: toIterable does not change the concrete type.
For Streams/Iterators we should be fine as well: nobody is using that right now.
bbakerman
left a comment
There was a problem hiding this comment.
Thanks very much for this PR. I think this puts us into a better place (its possible to reduce memory usage if you use streams) without being too invasive.
There was a problem hiding this comment.
I had a look at graphql.execution.ExecutionStrategyParameters#getListSize - we never use it.
I was worried this -1 semantics could affect some one )badly but we never use it - I think this will be ok
There was a problem hiding this comment.
yes, I think it is backward compatible.
Maybe we can introduce a new method that returns OptionalInt ?
There was a problem hiding this comment.
At the end of the day we are still allocated an array the size of the list - So source of the data can be a stream but we can stay streaming all the way.
This is an improvement - as you say it greatly reduces your memory usage (nearly half??) but longer term could we have streams all the way down to the turtles?
Thinking aloud here it might be too theoretical - I know there is code out there that assumes ExecutionResult is really a Map of lists and maps and values. Would introducing "streams" in that break things?
There was a problem hiding this comment.
At the end of the day we are still allocated an array the size of the list - So source of the data can be a stream but we can stay streaming all the way.
yes, I was not able to remove this big list.
This is an improvement - as you say it greatly reduces your memory usage (nearly half??) but longer term could we have streams all the way down to the turtles?
Don't know yet: eventually everything is "materialized" in a single ExecutionResult. Would be nice to minimize everything else.
This the most big query we have so far.
Before
After
Thinking aloud here it might be too theoretical - I know there is code out there that assumes ExecutionResult is really a Map of lists and maps and values. Would introducing "streams" in that break things?
We are merely introducing streaming between datafetchers and execution strategy: so this is very conservative choice by now.
Allowing DataFetchers to return Stream<T> and Iterator<T>.
a14f55c to
101e06a
Compare
|
it would be possible to merge this tomorrow? I would like to use it in our applications, to provide further feedback to you guys 😊 |
|
@dfa1 I want to point you to the graphql RFC which aims to add defer and stream to GraphQL: https://github.com/graphql/graphql-spec/blob/master/rfcs/DeferStream.md (in case you missed it so far) |
|
@andimarek thank you :-) yes, I was aware of this proposal and I look forward to it! |
Hi @bbakerman @andimarek,
please find my first attempt to be a bit more lazy during list processing in
completeValueForList.The basic idea is to avoid to "materialize" a
Stream(and later maybeIterator) into anArrayList.I don't like some details of this PR (notably passing -1 as
listSize) but let's start the discussion anyway. Maybe someone have better ideas there.