inversify / InversifyJS

A powerful and lightweight inversion of control container for JavaScript & Node.js apps powered by TypeScript.
http://inversify.io/
MIT License
11.34k stars 719 forks source link

Huge performance drop when having thousands calls to factory which resolves singletone dependencies #423

Closed sanex3339 closed 8 years ago

sanex3339 commented 8 years ago

So, i have a project https://github.com/javascript-obfuscator/javascript-obfuscator/tree/dev and now i'm trying to move it on Inversify.

Inside some class i have code that traverses over AST tree (nodes) and for each node calls factory which returns array of node obfuscators for this node.

Without Inversify performance tests are returns following results: 1) 500 runs of program - ~8 sec total. 2) run with very large code size - ~1.8 sec.

With Inversify i got huge performance drop: 1) 500 runs of program - ~28 sec total. 2) run with very large code size - 5-6 sec.

With Inversify i created factory which resolves and returns instances of node obfuscators. All instances are bind in singletone scope.

Now code examples.

Old code:

Factory call when traversing through ast tree https://github.com/javascript-obfuscator/javascript-obfuscator/blob/dev/src/Obfuscator.ts#L116

Factory itself https://github.com/javascript-obfuscator/javascript-obfuscator/blob/dev/src/node-transformers/AbstractNodeTransformersFactory.ts#L38

With Inversify

Factory call when traversing through ast tree https://github.com/javascript-obfuscator/javascript-obfuscator/blob/inversify/src/Obfuscator.ts#L147

Factory itself https://github.com/javascript-obfuscator/javascript-obfuscator/blob/inversify/src/container/InversifyContainerFacade.ts#L64

Binding setup for classes which factory is resolve https://github.com/javascript-obfuscator/javascript-obfuscator/blob/inversify/src/container/modules/NodeObfuscatorsModule.ts#L15

So i'm doing something wrong?

Expected Behavior

Small performance drop.

Current Behavior

Huge performance drop.

Steps to Reproduce (for bugs)

You can clone this two branches https://github.com/javascript-obfuscator/javascript-obfuscator/tree/dev https://github.com/javascript-obfuscator/javascript-obfuscator/tree/inversify

then npm i && npm test

latest two asserts are performance asserts.

lholznagel commented 8 years ago

Do you use version 2 or version 3 (current beta)?

Version 3 will have a big performance boost https://github.com/inversify/InversifyJS/issues/395#issuecomment-258036753

sanex3339 commented 8 years ago

Version 3.0.0-beta.2

remojansen commented 8 years ago

Hi @sanex3339 thanks for reporting this issue I will investigate and let you know my thoughts ASAP.

remojansen commented 8 years ago

I think I might have an idea of what is going on... Your individual entities are singletons but your factory is not a singleton.

There is something that is not very clear in the docs: The factory is a singleton by default and there is no way to change that. However, the value returned by the factory may or may not be a factory.

The factory is just a function which is injected as a function always but when you invoke the factory the value returned may or may not be a singleton. The behavior is controlled by the way to implement your factory and it is not controlled by InversifyJS.

In your case is a bit more complicated because your factory uses named bindings but you can try the following (Please note that I have written the following code in notepad so it may contain errors).

this.container
    .bind<INodeTransformer[]>(ServiceIdentifiers['Factory<INodeTransformer[]>'])
    .toFactory<INodeTransformer[]>((context: interfaces.Context) => {

        let cache = new Map<string, INodeTransformer[]>(); // singleton!

        return (nodeTransformersMap: Map<string, string[]>) => {

            (nodeType: string) => {

                if (cache.has(nodeType)) {

                    // Try to access singleton from cache
                    return cache.get(nodeType);

                } else {

                    // Try to resolve

                    const nodeTransformers: string[] = nodeTransformersMap.get(nodeType) || [];
                    const instancesArray: INodeTransformer[] = [];

                    nodeTransformers.forEach((transformer: string) => {
                        instancesArray.push(
                            context.container.getNamed<INodeTransformer>(
                                'INodeTransformer', transformer
                            )
                        );
                    });

                    // Add to cache
                    cache.set(nodeType, instancesArray); // add to cache
                    return instancesArray;

                }

            };
        }
    });    

Please let me know if it improves the performance. Also please note that the performance should be good but is never going to be as fast as your original code because your original code didn't have to resolve a dependency tree at all. Everything was in memory as a singleton since the moment it was declared.

sanex3339 commented 8 years ago

Yep, i done practically same thing simultaneously with your comment. Thank you for quick response and amazing Inversify!

remojansen commented 8 years ago

Happy to help. I will close this issue.