typestack / routing-controllers

Create structured, declarative and beautifully organized class-based controllers with heavy decorators usage in Express / Koa using TypeScript and Routing Controllers Framework.
MIT License
4.41k stars 394 forks source link

Performance Issue with big json result #226

Open ahmedsaedhub opened 7 years ago

ahmedsaedhub commented 7 years ago

Hello Guys,

The response time take too much time in order to return the response of a big json result. while tracing the issue i get the result from database in about 1.2 sec as a json result, then after return it to action method takes too much time to resolve (about 20-30 seconds).

I tried using @Controller() instead of @JsonController() in order to not forcing the response to re-parse again, with no effect.

The only solution worked with me is, getting the KOA context "@Ctx()" and send the result directly, and it's working fine now.

Any other solution for this issue?

NoNameProvided commented 7 years ago

Can you do some perf metrics to see where the majority of the time is spent? One quick advice, you should try to skip circular check in class-transformer

ahmedsaedhub commented 7 years ago

Hello @NoNameProvided, I did that as i mentioned and found that i have the result in memory after about 1.2 sec and after return the data to action method it takes this much time to respond the request.

PS: how can i skip circular check in class-transformer?

NoNameProvided commented 7 years ago

The only way is to disable class-transfomer when you create your server via the classTransformer: false option. However this disables class-transfomer entirely (so no typed paramters) not just the skipCircularCheck. It is ok to turn it off for testing this issue, but probably a bad idea to turn it of completely.

There have been some discussion about adding an option to disable circular check recently, but seems it haven't been implemented yet. If disabling class-transfomer solves your problem you can do more research by cloning the class-transformer repo and calling plainToClass by hand on your test data.

ahmedsaedhub commented 7 years ago

This is so cool, but despite talking about "class-transfomer" what is the difference between "@Controller" and "@JsonController", I think the first one should return the result as it is without re-parsing it to JSON because in my case its actually JSON, this is because after sending the result directly to KOA @Ctx the problem has been solved.

NoNameProvided commented 7 years ago

Both @Controller and @JsonController is used to signal that you want to register each decorated controller method (with @Get, @Post, etc...) in that given class as a route handler.

When you use @JsonController then all controller actions will return serialized json data, and the response's content-type always will be application/json. A basic @Controller will do nothing with the returned value.

MichalLytek commented 7 years ago

A basic @Controller will do nothing with the returned value.

Are you sure?

if (options.classTransformer !== undefined) {
    driver.useClassTransformer = options.classTransformer;
} else {
    driver.useClassTransformer = true;
}
// check if we need to transform result and do it
if (this.useClassTransformer && result && result instanceof Object) {
    const options = action.responseClassTransformOptions || this.classToPlainTransformOptions;
    result = classToPlain(result, options);
}
if (action.isJsonTyped) {
    options.response.json(result);
} else {
    options.response.send(result);
}
NoNameProvided commented 7 years ago

Are you sure?

If you read carefully you can see that I have mentioned before that class-transformer is called on the responses every time:

The only way is to disable class-transfomer when you create your server via the classTransformer: false option.

MichalLytek commented 7 years ago

So we have to implement separate on/off options for transforming inputs and output, maybe by extending #209 default options PR or by route/action decorator. What do you think, @NoNameProvided @pleerock ?

NoNameProvided commented 7 years ago

maybe by extending #209 default options PR or by route/action decorator.

I think we should go with adding this as option to the controller and route decorators, but not as a global option.

My reasoning: We already have an option to disable class-transformer entirely, and I don't see it as a common use-case to disable one direction entirely (input or output transformation) but not the other. While I can see it as a totally valid use-case to disable it per controller or per route basis for one direction only. (like now when class-transfomer chokes on a big response.)

So I would extend the decorators something like this:

JsonController(baseRoute?: string, options: ControllerOptions): (object: Function) => void;
// and 
Get(route?: string, options: HandlerOptions): Function;

and both ControllerOptions, and HandlerOptions implements a common TransformationOptions interface, something like:

interface TransformationOptions {
  transformRequest: boolean;
  transformResponse: boolean;
}

Thoughts?

Edit: I added the different ControllerOptions, HandlerOptions types to make it future proof, when we want to add some other stuff for each of them, we dont have to break the signature by introducing new types.

pleerock commented 7 years ago

@asaieed can you please update class-transformer to the latest version? This issue was recently fixed there

MichalLytek commented 7 years ago

@NoNameProvided Could you make a PR with a controller/action options for disabling the request/response transforming as you described? We would then handled issues #153 #179 #193 as it's a needed feature. We could then make it more intelligent as discussed in #236 .

NoNameProvided commented 7 years ago

This discussion is copied from #193.

@charsleysa wrote on 29 June 2017 01:02 CEST on #193

Hi all,

One pain point I have is when using class-transformer I can't seem to return a buffer because the class-transformer keeps trying to process it.

I use class-transformer for almost all routes but I need some individual routes returning stuff that express can handle (such as buffers) but shouldn't be processed by class-transformer.

The current workaround is to get the response object using the @Res() decorator and calling the send() method with the buffer as the parameter.

I dislike this because it doesn't follow the execution flow of routing-controllers (e.g. none of the interceptors are called before the result is returned to the client).

Cheers!

@19majkel94 wrote on 7 Aug 2017 13:46 CEST on https://github.com/pleerock/routing-controllers/issues/193#issuecomment-320642387

I wonder how we should do this:

  1. Detect if the action is json typed (@JsonController or @JsonResponse), so the returned object is data object and we should transform it using class-transform and return using res.json(). Otherwise don't transform and use res.send(result).
  2. Detect type of the response - if it's Buffer don't transform, if Stream pipe to response, otherwise transform and send.
  3. Introduce decorator like @RawResponse() to bypass the class-transformer transforming - might be helpfull with big and weird objects from ODM/ORM.

@pleerock @NoNameProvided any thoughts? 😉

NoNameProvided commented 7 years ago

This discussion is copied from #153.

@ssljivic wrote on 25 May 2017 19:57 CEST on https://github.com/pleerock/routing-controllers/issues/153#issuecomment-304078213

@pleerock do you think that this performanace issue can be resolved in class-transformer project?

Maybe, for a start, we can split useClassTransformer into 2 new config options: useRequestClassTransformer and useResponseClassTransformer. Right now setting the useClassTransformer = false is not an option as request will not be transformed.

@ssljivic wrote on 1 July 2017 12:59 CEST on https://github.com/pleerock/routing-controllers/issues/153#issuecomment-312425534

@pleerock with class-transformer 0.1.7 it is about 7 times faster than with previous 0.1.6 version. That is great increase in speed 👍 🥇 .

However, it is still 2 times slower than the res.send(). Although 0.1.7 provides tremendous speed improvement, it would still be a good to have an option to disable class transformer for responses, both as a global cfg and on per controller and/or method level.

NoNameProvided commented 7 years ago

This discussion is copied from #179.

@jselesan wrote on 9 Jun 2017 19:27 CEST on https://github.com/pleerock/routing-controllers/issues/179#issue-234891126

Hi there. A time ago, I had to disable classTransformer (in useExpressServer) because it didn't work fine with mongoose models, but now, I would like to take advantage of classTransformer to transform the query params to an object, so it would be nice to have the ability to enable classTransformer only for the input and keep it disabled for output. Is it possible?

NoNameProvided commented 7 years ago

Could you make a PR with a controller/action options for disabling the request/response transforming as you described?

Yes I will when I have time

NoNameProvided commented 7 years ago

Just a quick report, I had no much time recently, but I will try to implement this next week. Sorry for the delays.

github-actions[bot] commented 4 years ago

Stale issue message