Reducing object allocations by reusing the Array object from the parent middleware

[amatsuda]

This patch drastically reduces Array object allocations per each request here and there in the whole Rack stack.

Background

Rack became dominant in the Ruby web by its simple and elegant API as a stack of middleware that returns a set of [status, headers, and body].

But, due to the combination of that API and Ruby's language specification that returning multiple values from a method uses an Array instance, it ends up with allocating an Array object per each Rack middleware. And in case of relatively big applications (e.g. Rails), it allocates dozens of Array objects per every request just for passing values between the middleware stack.

The Patch

We, middleware implementors, used to get the status, headers, and body from the upper middleware and assign them into each respective local variable. Then, for passing a new set of status, headers, and body to the next middleware, we usually wrap them in another Array literal like this [status, headers, body] which allocates another instance of Array object (which will usually be discarded very soon afterwards).

Instead, this patch introduces a new approach to reuse the "container" Array between middleware. The point is to keep the reference to the Array object as well when assigning the three local variables, then reuse that same Array object for packing the return values. For instance:

 def call(env)
   _status, _headers, _body = response = @app.call(env)
   response[2] = 'hello'
   response
 end

I should admit that it looks a little bit tricky, especially when assigning status and body. Maybe I should choose some other word than response for the ivar name. Any better idea is welcome.

Anyway, note that the above code does not allocate a new Array object for returning the response. With this patch, I confirmed that I could reduce a dozen of Array object allocations per each request in my local Rails app (and indeed I'm almost sure that I would be able to reduce another dozen of Arrays with another patch for Rails codebase with the same approach).

[matthewd]

This has previously come up as a compatibility issue where an application returns a frozen array (e.g. in a zero-allocation final fallback error-handler).

(There's also obviously at least some risk of an application returning a shared array without freezing it, but I'm not sure how much consideration that merits.)

Perhaps this would be a good use for a replace_body helper method that could simultaneously package the more obscure assign-into-array-element aspects of the implementation, while also including a dup if frozen? bit for that edge case?

[jeremyevans]

We've updated SPEC to require the request environment and response headers are unfrozen. We could change SPEC to require response array is unfrozen. However, this optimizes for different things. If you are using multiple middleware that modify the response status/body, unfrozen is better so you can have a change like this. If you don't have such middleware, requiring unfrozen is worse because it means you cannot use a frozen array for responses.

I'm hesitant to break backwards compatibility here without evidence that the performance advantage is really worth it. Seems like even dozens of array allocations would barely be measurable against the backdrop of a Rails request. Do you have benchmarks showing this will actually make a difference in production applications?

[ioquatix]

I know in past applications I've done things like SPECIFIC_RESPONSE = [...].freeze so this would break that use case. Whether that's expected, I don't know.

[ioquatix]

One argument that could be made here, is that a specific "Response object" might make more sense than an Array if it allows for wins like this, assuming it's big enough to justify the change, which I'm not sure it is. But at least worth considering.

[jeremyevans]

One argument that could be made here, is that a specific "Response object" might make more sense than an Array if it allows for wins like this, assuming it's big enough to justify the change, which I'm not sure it is. But at least worth considering.

I think you still have the same issue. If you have a Response object instead of an array, do you support frozen Response objects, or do you expect to be able to mutate Response objects?

[ioquatix]

IIRC, in Async::HTTP the response is considered mutable in every case and at least one response is allocated per request, for pretty much exactly this reason.

[ioquatix]

As it stands, I don't think we can do this. However, the suggested "dup if frozen?" approach to probably get 90% of the benefit with a clear model for people who don't want the response to be mutated.

e.g.

def call(env)
  response = @app.call(env)
  response = response.dup if response.frozen?
  response[2] = ...
  return response
end

@jeremyevans Regarding performance, with a sever like Puma handling 8 simultaneous requests, this is not a big deal. But for a server like falcon handling 1000s of simultaneous requests, this can be an issue. So performance in this regard is context sensitive. In my benchmarks, I've seen a single trivial request create tons of strings, arrays and hash instances per request which are ultimately garbage - so I think there is a valid case to be made here.

I believe what we would need to do is document that the response array is considered mutable unless frozen, in which case it may be duped. To be frank, anyone who is returning responses which are not frozen, and expects them to not be mutated (e.g. headers, etc) is playing with fire. Realistically, because headers are considered mutable, the implication is it will be impossible to have frozen responses anyway. i.e.

ERROR_RESPONSE = [400, {}.freeze, ["Error"]].freeze

def call(env)
  return ERROR_RESPONSE
end

Is no longer valid in Rack 3, and this is also dangerous/wrong:

# Headers is mutable and may be modified:
ERROR_RESPONSE = [400, {}, ["Error"]].freeze

[jeremyevans]

Realistically, because headers are considered mutable, the implication is it will be impossible to have frozen responses anyway.

I hadn't considered this previously, but you are correct. If response headers must be unfrozen (as it is in the rack 3 SPEC), it makes sense to require the response array to be unfrozen, since it is no longer allowable to reuse response arrays. So I think we should modify SPEC to require that response arrays be unfrozen in rack 3, and accept this PR (after conflicts are fixed).

[ioquatix]

@amatsuda do you mind considering the most recent feedback and updating this PR? We might need some updated documentation and SPEC Rack::Lint checks as outlined by Jeremy's most recent comments.

[ioquatix]

Thanks everyone.

[amatsuda]

@ioquatix Thank you for completing this patch! (and I'm sorry that I couldn't come back here earlier...)