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

do not reparse JSON responses in a loop #172

Merged
merged 2 commits into from
Nov 22, 2021
Merged

do not reparse JSON responses in a loop #172

merged 2 commits into from
Nov 22, 2021

Conversation

Geal
Copy link
Contributor

@Geal Geal commented Nov 22, 2021

Potential fix for #144
related: #33 #80

with very large responses, we were looping over HTTP chunks by
accumulating them, trying to parse the JSON response, then go for
another iteration if it was not enough, so we end up parsing the same
data very frequently

This commit first accumulates the data entirely, then parses it

⚠️ this potentially breaks @stream: the previous code was trying to recognize an entire json response, return it, then try parsing another json response if there's still some data, and return a stream of responses. From what I see the code could not handle stream responses anyway since multipart is expected: https://github.com/graphql/graphql-over-http/blob/main/rfcs/IncrementalDelivery.md#content-type-multipartmixed
We should investigate that
Edit: @stream is not currently supported, so this can be merged right now, and we'll modify once we do it (we're keeping the Stream of responses to that end)

performance results

main d9b3c43

Summary:                                                     
  Total:        200.0049 secs                                
  Slowest:      20.0006 secs                                 
  Fastest:      9.0386 secs                                  
  Average:      19.1225 secs                                 
  Requests/sec: 2.4999                                       
                                                             
                                                             
Response time histogram:                                     
  9.039 [1]     |                                            
  10.135 [1]    |                                            
  11.231 [1]    |                                            
  12.327 [11]   |■                                           
  13.423 [9]    |■                                           
  14.520 [13]   |■                                           
  15.616 [10]   |■                                           
  16.712 [15]   |■                                           
  17.808 [18]   |■■                                          
  18.904 [9]    |■                                           
  20.001 [412]  |■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■    
                                                             
                                                             
Latency distribution:                                        
  10% in 15.9425 secs                                        
  25% in 20.0001 secs                                        
  50% in 20.0002 secs                                        
  75% in 20.0002 secs                                        
  90% in 20.0002 secs                                        
  95% in 20.0002 secs                                        
  99% in 20.0003 secs                                        
                                                             
Details (average, fastest, slowest):                         
  DNS+dialup:   0.0003 secs, 9.0386 secs, 20.0006 secs       
  DNS-lookup:   0.0000 secs, 0.0000 secs, 0.0000 secs        
  req write:    0.0001 secs, 0.0000 secs, 0.0008 secs        
  resp wait:    6.8858 secs, 0.0001 secs, 19.0536 secs       
  resp read:    12.2363 secs, 0.9462 secs, 19.9999 secs      
                                                             
Status code distribution:                                    
  [200] 500 responses                

Screenshot from 2021-11-22 10-59-09

spending most of the time deserializing strings (I was testing a products subgraph where the name is 40MB long)

this PR

Summary:                                                  
  Total:        26.1397 secs                              
  Slowest:      5.3922 secs                               
  Fastest:      0.2809 secs                               
  Average:      2.4454 secs                               
  Requests/sec: 19.1280                                   
                                                          
                                                          
Response time histogram:                                  
  0.281 [1]     |                                         
  0.792 [6]     |■■                                       
  1.303 [17]    |■■■■■                                    
  1.814 [78]    |■■■■■■■■■■■■■■■■■■■■■■                   
  2.325 [145]   |■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■ 
  2.837 [115]   |■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■         
  3.348 [78]    |■■■■■■■■■■■■■■■■■■■■■■                   
  3.859 [35]    |■■■■■■■■■■                               
  4.370 [14]    |■■■■                                     
  4.881 [8]     |■■                                       
  5.392 [3]     |■                                        
                                                          
                                                          
Latency distribution:                                     
  10% in 1.5380 secs                                      
  25% in 1.9118 secs                                      
  50% in 2.3362 secs                                      
  75% in 2.9391 secs                                      
  90% in 3.4465 secs                                      
  95% in 3.8630 secs                                      
  99% in 4.6872 secs                                      
                                                          
Details (average, fastest, slowest):                      
  DNS+dialup:   0.0002 secs, 0.2809 secs, 5.3922 secs     
  DNS-lookup:   0.0000 secs, 0.0000 secs, 0.0000 secs     
  req write:    0.0001 secs, 0.0000 secs, 0.0072 secs     
  resp wait:    0.4656 secs, 0.0002 secs, 2.0409 secs     
  resp read:    1.9795 secs, 0.2806 secs, 4.6103 secs     
                                                          
Status code distribution:                                 
  [200] 500 responses                                     

Screenshot from 2021-11-22 11-44-07

There's definitely an improvement on large responses. It probably won't have a big impact on small responses that can fit in one chunk

@Geal Geal force-pushed the handle-large-responses branch 2 times, most recently from 7f6cf45 to 7c609ad Compare November 22, 2021 13:43
with very large responses, we were looping over HTTP chunks by
accumulating them, trying to parse the JSON response, then go for
another iteration if it was not enough, so we end up parsing the same
data very frequently

This commit first accumulates the data entirely, then parses it
@Geal Geal force-pushed the handle-large-responses branch from 7c609ad to 87259f8 Compare November 22, 2021 13:53
@Geal Geal marked this pull request as ready for review November 22, 2021 14:14
@Geal Geal mentioned this pull request Nov 22, 2021
8 tasks
Copy link
Contributor

@BrynCooke BrynCooke left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@o0Ignition0o o0Ignition0o left a comment

Choose a reason for hiding this comment

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

lgtm! :shipit:

None
}
},
serde_json::from_slice::<graphql::Response>(&current_payload_bytes).unwrap_or_else(
Copy link
Contributor

Choose a reason for hiding this comment

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

yay for unwrap or else :D

@Geal Geal merged commit 61b5a8d into main Nov 22, 2021
@Geal Geal deleted the handle-large-responses branch November 22, 2021 15:24
@Geal Geal self-assigned this Dec 1, 2021
tinnou pushed a commit to Netflix-Skunkworks/router that referenced this pull request Oct 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants