Change how types are processed to avoid gobs of memory usage #37
Conversation
|
could you add your testcase. For the test in the /test folder. I see no difference with your implementation. |
|
"See no difference" what does that mean? You mean you see no difference in compiler memory usage? The test case I have is Symmetry's proprietary schema. I will try to come up with one that can be public. |
|
Yes, the difference for the graphqld/test in memory consumption is only a few KiB. |
|
Hm.. I can't build the test. Always get a segfault, with both my version and master. I don't know if it's an issue with memory, or something else. But it makes a huge different for my project. |
|
I commented out the entire schema, and it still segfaults. Something else is wrong with the test (well, not wrong, but it's triggering a compiler bug I think). |
|
I have a system at work with 16GB of RAM. It crashes there too. Vanilla clone of graphql. dmd 2.088.1. And if I'm reading top correctly, it's only using 5GB at that point. |
|
Note: are you sure you are using my PR branch? I noticed the test directory does its own download of graphqld. |
|
something is very strange, let me do some investigating |
|
try to rebase to master, I removed a dflag in the test and it seems to compile again. |
source/graphql/graphql.d
Outdated
| default: {} | ||
| import graphql.traits : execForAllTypes; | ||
| alias defaultArgFn = Json function(string) @safe; | ||
| static defaultArgFn[string] items; |
There was a problem hiding this comment.
can you move this out into the graphql object.
I usually try to avoid static variables
There was a problem hiding this comment.
I might rework this function anyway. It seems kind of odd to generate these closures inside a method, when they can just be generated on the object itself, especially since it's a class. Less to allocate.
There was a problem hiding this comment.
Hm... I was incorrect, I was thinking of the setDefaultResolver function, which is outside the GraphQLD type.
I did move this out into the class, but my comment about generating closures inside a method was referring to the other function, which needs to keep allocating closures. However, it could easily be reworked into a class, which might actually make more sense as there may be more allocations than is necessary. A class would only have one allocation.
schveiguy
force-pushed
the
alternatetypeprocessing
branch
from
9c297d4
to
982d665
Compare
|
Addressed all the review comments. I went through and think I got all the braces. I also removed static from all my static-foreach, as I didn't need static for any of them. |
|
this probably needs a squash, let me know if we are close, and I will. |
|
I have just been able to reproduce your memory savings, paytracker got done to ~6.5GB from 9 and change. Could you squash please. |
|
even nice is the drop from 34 to 24 seconds |
…stead. Should save a bunch of compile-time memory.
schveiguy
force-pushed
the
alternatetypeprocessing
branch
from
c589eee
to
722c702
Compare
schveiguy
force-pushed
the
alternatetypeprocessing
branch
from
722c702
to
7737340
Compare
|
OK, squashed into the 3 basic commits I think are relevant. Also could just squash to one if necessary. |
Hm... my tests showed 3.5GB. But I still had the -lowmem switch on. Maybe we should keep that one. |
|
yeah, lowmen is a switch we could test with our internal project. |
|
thank you |
Memory usage of the compiler is way high for graphql because of the usage of multiple iterations of compile-time lists to collect all relevant types of a schema. Some of the templates it uses are either linear recursive or divide-and-conquer which both result in lots of compiler memory used to track these templates.
In this version, instead of generating the list of types at compile-time, we are simply recursively calling a function alias ONCE for each type in the list. This allows us to build runtime reflection capabilities but only instantiate one template per type instead of thousands of templates. On my system, this makes a project which consumes over 10GB of memory to compile (not sure how much, I ran out of memory!) down to 3.5GB.
I expect we can possibly improve this some more, but this at least gets me compiling again.
The reflection library is haphazardly thrown together, just getting what I needed to replace
collectTypesand is obviously inconsistent. I'm happy to modify this to fit the needs, but my first goal was simply to replace the functionality that exists today. Not being 100% certain of how the library works or the graphql spec, I didn't want to mess with anything.