Class-transformer: MetadataStorage perf issues

[sheiidan]

MetadataStorage is using Arrays as it's internal storage (instead of es6 Map) to store metadata which leads to perf issues.
findMetadatas() and findMetadata() are the expensive methods since they use Array.prototype.filter() and Array.prototype.find().
In case a client library has a large number of class-transformer metadatas, these operations will be very expensive since every MetadataStorage's internal array keep all types of the same metadata flat for all classes and properties.
My suggestion is to convert these arrays to ES6 Map, for O(1) access like we did in #58

[sheiidan]

For a 250K objects with 100 @Type() metadatas, plainToClass() took 10 seconds:
As you can see in the image below, most of the time spent on Array.prototype.find() and Array.prototype.filter()

1. find() - 6.1 seconds which is 61% from the total duration
2. filter() - 1.1 seconds which is 11% from the total time

[sheiidan]

Now the same test above runs 2875 ms instead of 10026 ms. The Pull request changes MetadataStorage to use ES6 Map instead of regular Arrays and that did the trick:

Now the most expensive method call is .filter() that we still have inside getMetadata(), but it's only took
814 ms for 250K objects:

[gempain]

Looking great ! Since you have changed MetadataStorage, maybe you have an idea for #28 ?

[sheiidan]

@gempain , I executed your plunker test with my fix and it looks like my fix also fixes #28.
Please take a look at the results below, let me know if the parsing is ok:

[sheiidan]

I found an issue with inheritance. I'll update the thread once I'll have a fix

[sheiidan]

@gempain I fixed the inheritance issue and now my fix able to correctly parse your test.
As you can see, now the custom transformation in created and lastUpdated is working:

[gempain]

@sheiidan I feel honoured but it is actually not my punker :p I just tried to reference related issues because I feel like the follow up is left a bit behind. I will have a look at your code and let you know :) Thanks for the time you spent fixing these issues, I love seeing how people can come together to make amazing projects grow :)

[gempain]

@sheiidan Indeed it seems like you fixed the issue :D Can you see if the @Transform works for a test I created a while ago, but which I never merged ? Back then it seemed to be fixed by adding @Type(() => Date) above the @Transform, but I feel like this was more of a hack (and I made a PR to fix the docs but don't feel too right about it).

I also think that in the future, when we find bug, we should add a test that reproduces the bug. This makes fixing much easier :) Don't forget to add tests for your PR if you have time, this way we can make sure that any further PR does not break your changes :)

[sheiidan]

@pleerock can you please review my PR? Thanks!

[sheiidan]

@pleerock this PR fixes a very important perf issue. Can you please review my PR? Thank you!!!

[sheiidan]

@pleerock, any chance my fix will get in?

[NoNameProvided]

Hi @sheiidan!

@pleerock is pretty busy with moving the TypeORM project forward right now, for what I am really grateful. I am sure he will review this PR and comment or merge it as soon as he has time for it.

In the meantime you can install your fork via npm with the following command:

git+https://[email protected]/sheiidan/class-transformer.git#storage-perf-fix

Thanks for your understanding.

[sheiidan]

@NoNameProvided, sure no problem. thanks for the update!

[simonalbrecht]

It's been 6 months. Is there any update on this?

[AlexKolesnik]

Hi sheiidan,

I assume your solution works for you. I have problems though here.... Any ideas?

TypeError: Cannot read property 'prototype' of undefined
    at MetadataStorage.getAncestors (http://localhost:8100/build/vendor.js:140278:63)
    at MetadataStorage.findMetadatas (http://localhost:8100/build/vendor.js:140264:36)
    at MetadataStorage.findTransformMetadatas (http://localhost:8100/build/vendor.js:140133:21)
    at TransformOperationExecutor.applyCustomTransformations (http://localhost:8100/build/vendor.js:139942:58)
    at _loop_1 (http://localhost:8100/build/vendor.js:139909:41)
    at TransformOperationExecutor.transform (http://localhost:8100/build/vendor.js:139932:17)
    at _loop_1 (http://localhost:8100/build/vendor.js:139908:45)
    at TransformOperationExecutor.transform (http://localhost:8100/build/vendor.js:139932:17)
    at ClassTransformer.plainToClass (http://localhost:8100/build/vendor.js:65581:25)
    at http://localhost:8100/build/main.js:410:77

[247GradLabs]

Btw is there a reason class-transformer uses it's own metadata storage instead of using reflect-metadata?

[NoNameProvided]

It stores extra data, in a format what suits this lib better.

[247GradLabs]

Well - at least reflect-metadata uses maps instead of arrays 😅
Anyway, I looked at the code again to make sure but from what I can see it should be possible to store the metadata properly and maybe even more efficient using reflect-metadata as well.
I just think that using a well known library would make the code a bit easier and if I have some time again I can try to write up a PR for that.

At least I hope you can merge #137 fast so this nice library can get rid of that horrible performance leak.

[NoNameProvided]

Hi all!

I am closing this as this has been fixed in #137.

[github-actions[bot]]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.