Api: Addressing performance issues

[JamesGuthrie]

I have recently been trying to figure out where my application spends most of its time when preparing requests and uncovered a couple of performance issues in the way that this api library is designed.

To illustrate the issue clearly I have profiled the application with a span-based profiler and annotated the result to better discuss the result.

The profile should be read from left to right and illustrates at which time a function which was being profiled was entered and exited. The long blue bar at the top of the image is the span of the application, whereas the short blue bar at the top left is the Application::boot() running. The short red bars represent db accesses.

For this discussion there are four distinct and important segments (the large, tall coloured boxes, which I added), coloured blue, red, yellow, and green.
The blue segment is the time that the application spent in the controller, after which, my business logic is over. The red, yellow, and green blocks represent time which is spend in preparing the response to send out.

To be precise, the red block is time spent in Laravel's response handling, while the yellow and green blocks are time spent in Dingo's response handling. The fact that the db access patterns look the same should be alarming. In fact, the access patterns are the same because the same queries are being run.

So what is going in the red, yellow, and green blocks? Basically $model->toJson() is being called on the same model three times. My models have some attributes which are generated and appended using Laravel's appends, some of them require running db queries.

The real question is, why is toJson() being called on the same model three times? The answer to which exposes the design issue in this api library. The red block is just laravel doing what laravel does when the controller returns a collection or model (actually, anything it thinks can be made into json) turn it into json. Because Dingo wraps Laravel's response processing, this step basically can't be skipped.

After the red block is run, Dingo receives Laravel's response, and prepares it, which involves discarding the result of the toJson() above, extracting the original model which was used, and constructing a Dingo Response object, which is a subclass of Laravel's Response object. On constructing the response, laravel's Response::setContent() method is called, which also runs toJson() on the model, because that's what Laravel's Response does!

Finally, the third toJson() (in the green block) is the transformer I'm using to get the json representation of the model. In effect, the red and yellow blocks are unnecessary, and discarded, but still take between 1/3 to 1/2 of total request processing time.

So what are the solutions to this problem? As a user of this library I see the following solutions:

1. Make toJson() cheap
2. Cache the result of attribute methods on my objects
3. Fix this library
4. Use a different library

Unfortunately options 1. and 2. aren't real solutions. 1. limits me unnecessarily, 2. is just going to give me a headache.

Option 3. could be possible, but I think that the fix might be too drastic for a library which hasn't even hit 1.0 yet. It appears to me as though the core issue is the fact that dingo wraps itself around laravel's response. As far as I can tell there is no clean way to prevent the red block from happening in the current design, short of conditionally overriding the behaviour of Illuminate\Http\Response::setContent. I think that solution would be to pass Laravel the actual formatted response in string format directly from the controller. I assume that this would break some or all of dingo's current design, but I don't have a good overview of how all of the pieces of dingo fit together, so I don't know what the repercussions of this would be.

If we don't reach some sort of consensus here then I guess I will have to resort to 4.

I hope that I have clearly presented the issue I am currently having and that we can begin a fruitful discussion based on my findings.

[JamesGuthrie]

Having taken another look at this, I think I have found the root cause and a simple solution.

In my controllers I am currently returning models and collections directly because I was intending to use internal requests in my application. Instead of returning models and collections I will use the Helpers trait to return json directly from the controllers.

[tembra]

@JamesGuthrie please let me know if using Helpers trait you solve this issue.
Your post really gives me a headache xD

[johannesschobel]

@JamesGuthrie same here - please report and give feedback on how to speed up dingo ;)

[JamesGuthrie]

Sorry it's taken me a while to get back to this. I don't have the time to make this as thorough as the above post. I can give some feedback as to the behaviour of the helpers, as that was what @tembra was asking about.

What I have done to reduce the problem originally mentioned is to use the response->array() helper and ensure that I convert my object or collection to an array _directly in the controller_. A concrete example of the code I used is as follows:

return $this->response->array($myModel->toArray());

The response in the helper trait offers a number of other methods for returning data though, and the question remains as to how those affect performance. The other methods are: created, accepted, noContent, collection, item, paginator, and some error methods.

created, and accepted will have the same performance penalty if one passes an Eloquent\Model object to them. noContent doesn't apply, it has no content, and the error methods do not take Eloquent\Model or Collection objects.

collection, item, and paginator are interesting because they use the specified transformer. I have found that using these methods as documented in the wiki incur the same performance penalty mentioned above. The documented method in the wiki looks as follows:

return $this->collection($users, new UserTransformer);

I have found that the following variation does not incur the performance penalty:

$fractal = new Manager();
$collection = new Collection($users, new UserTransformer());
return $this->response->array($fractal->createData($collection)->toArray());

Once again, the transformation to an array representation is taking place in the controller, and not within dingo's magic response transformers.

[JamesGuthrie]

I just wanted to add that I do see a potential solution to the problem in the collection, item, and paginator methods.

The problem is that the response factory doesn't immediately process the response, rather it returns a Dingo\Api\Http\Response object with the correct transformer bindings set, so that the Dingo\Api middleware can process it on its way out. As mentioned in my first post, this incurs the penalties of:

1. Creating the Dingo Response object (which calls setContent, calling toArray() on the associated model or collection).
2. Laravel's response processing, which also results in toArray() being called.
3. Dingo's final response transformation, which also runs toArray().

So the solution would be to process and transform the request immediately in the response factory, and just return a "plain" Illuminate Response, and skip the response transformation in the Dingo\Api middleware.

[tembra]

Thank you for your feedback.
Let's see if @jasonlewis gets attention for this issue.

[hskrasek]

Definitely something to look into. Can't make any promises that it can be addressed by version 1.0.0 though. Depends on how breaking the fix would be, and the potential of that fix introducing a slew of new bugs.

[tembra]

@jasonlewis and @hskrasek I'm closer to production. May you have time to please give a clue/attention here? Also put this into 1.0.0 milestone?

[jasonlewis]

Going to chime in on this.

I'll be the first to admit that this project has some performance issues. These stem from the fact that a lot of things have been bolted on, and then bumped from earlier versions of Laravel, and eventually made to support several versions of Laravel. So there's a lot of issues that present themselves due to how this project has evolved.

Personally, I use this package on in production and haven't had any problems.

Going forward I do want to address a lot of the issues in the library by slimming it down significantly and removing a lot of bloat. I'd also like to drop support for older versions of Laravel and simply iterate as new versions become available.

Unfortuantely dedicated a lot of my free time to this project just isn't possible so I do what I can when I can. If anyone can patch some of these performance issues without severly breaking things then I'd love to look at a PR and get it merged in.

[jasonlewis]

Oh and I'd love to drop support for Lumen as well. :laughing:

[crynobone]

Oh and I'd love to drop support for Lumen as well.

😞 dingo has been working nicely with Lumen for us.

But it might be worth noting that we avoid using fractal transformer and stick with a simple transformer approach using something similar to https://github.com/laravel/framework/pull/14357. Too bad it wasn't merged in but we're pretty happy with the results.

[jasonlewis]

Simple transformers are best.

I'm currently toying with a new approach for how Dingo meshes with Laravel. When this package moved from 4.x to 5.x there were some significant routing changes and the fact that Lumen was now a thing. This meant that the way the package integrated with Laravel needed to be a lot looser. So we've ended up with our own Router class that was independant of Laravel's.

I'm currently messing around with going back to the original way of doing it whereby our Router extends Laravel's.

The idea is to then use more middleware to handle request hijacking and response formatting.

At the moment I'm looking at how Dingo handles responses. Currently it's pretty bad, which has been pointed out in the initial post of this discussion. Dingo totally discards the response generated by Laravel. I don't want to touch the prepareResponse method that's called on Laravel's router, and I don't want to generate an entirely new response.

So this is what I'm thinking...

1. Dingo needs to negotiate requests. That's the first priority. So all requests to the application go through a request hijacking middleware that looks at the accept header of the request and determines if it's an API request or not. This is how it works right now as well.
2. The request hijacker will take the request and runs it through a new pipeline.
3. The request hijacker appends a global middleware prior to running through the router to handle the response.
4. The router finds and executes the correct route.
5. The response handling middleware is fired last. It gets the Reponse object.
6. This middleware needs to decide a few things. First, it needs to assess whether or not any formatting needs to take place. I think if the content is already a JSON string then it should just return it as is. If the negotiated format is something other than JSON then we'll need to do one of two things. Either grab the original content or decode the current content.
   I think for most cases decoding the content to its array form and simply handing that off to the desired formatter would be enough. That way we don't have to waste any potential calls on toJson for models.

Now for transformers I think we do need to investigate handling the transformation prior to the response being generated to avoid all those wasted calls. Like you've said we can potentially do this within the response factory itself instead of binding and waiting until the response is generated.

So that's what I'm thinking at the moment. It simplifies the process a lot and it should allow for better integration with both the Laravel and Lumen routers.

[catalinux]

A next version should alsoe drop 5.* PHP version as 5.6 was active until 19 ian 2017, http://php.net/supported-versions.php

[johannesschobel]

yeah, but it is still maintained (security updates) till the end of 2018.
I think, one should keep the support for 5.6.* at least for some time, as many projects still rely on php 5.6.*

[catalinux]

@johannesschobel It's true, but i5.6 users could use this version of dingo API. If they want a gain of performance a simple upgrade to PHP7 would add a really nice boost.

It's hard to pass to a new version without dropping some old things.

[shawn-simon]

Just got bit by this in production super hard (because of the structure of our computed attributes, it wound up causing a stack overflow). I would love to try to find a work around so that I can use transformers without having toArray called on all of my models.

Update: Potential work-around here that isn't too bad, extend all of your models from a common model class, and then add the following:

    protected $enableSerialization = true;

    /**
     * Disables JSON serialization on the model instance.
     *
     * Any calls toArray will be skipped, and an empty array will be returned instead.
     */
    public function disableSerialization(): void
    {
        $this->enableSerialization = false;
    }

    /**
     * Convert the model instance to an array.
     *
     * If serialization is disabled, an empty array is returned instead of the model's contents.
     *
     * @return array
     */
    public function toArray()
    {
        if (!$this->enableSerialization) {
            Log::debug('Skipping toArray() call on Model because serialization is disabled.');
            return [];
        }
        return parent::toArray();
    }

Now before you call the transformer, just remember to call disableSerialization on the model! The only risk is that if you try to use toArray in one of your transformers. But that seems pretty unlikely to me (you could also just re-enable it at that point.)

[xwiz]

@jasonlewis Based on your 1 to 6 above, I believe Dingo is already doing all that. (cc @specialtactics )

If so, what else is needed to close this issue?