feat(dcs): optimize decorate models #857

sanketshevkar · 2024-06-07T17:31:24Z

Closes #856

Proposal for optimizing decorate models. More details on #856

Changes

Index all the commands in dcs in their respective maps based on their primary target: namespace, declaration, property, mapElement, types all have their separate maps. Wrap the commands with their index in dcs before indexing them.
Traverse through the model ast and lookup into the maps for any command related to the targets. Collect them in a list and sort them based on index before applying on the model element.
Function signatures have more or less stayed the same, except for executeCommand but are not a breaking change in nature.
Logic for executeCommand has changed a bit and decorateModels has changed significantly. New private methods have been introduced to support these.
New test case has added that aims at covering all possible command target combinations in dcs. (though list is not exhanustive)

Performance and Memory Impact.

Performance

Auto-generated model of size 1.6MB took Execution time: 6696.545083999634 ms to apply autogenerated vocabulary to the model with the older implementation.
New implementation takes Execution time: 44.81274998188019 ms for the same operation.

const start = performance.now();
DecoratorManager.decorateModels(mm, vocDcs);
const end = performance.now();
console.log(`Execution time: ${end - start} ms`);

152 times faster. <<hmm this makes me skeptical 💭 >>

Memory

`total_heap_size`

DecoratorManager.decorateModels(mm, vocDcs);
console.log(v8.getHeapStatistics());

total_heap_size recorded after an auto-generated model of size 1.6MB was applied autogenerated vocabulary to the model with the older implementation was 58261504
New implementation recorded total_heap_size as 61571072
1.06 times more space
5% increase in total_heap_size

`used_heap_size`

console.log(v8.getHeapStatistics());
DecoratorManager.decorateModels(mm, vocDcs);
console.log(v8.getHeapStatistics());

Increase in used_heap_size recorded after an auto-generated model of size 1.6MB was applied autogenerated vocabulary to the model with the older implementation was 6541360
New implementation recorded increase used_heap_size as 13041876
2 times more increase in the increase of used_heap_space
99.3% increase in increase of used_heap_size

Author Checklist

Ensure you provide a DCO sign-off for your commits using the --signoff option of git commit.
Vital features and changes captured in unit and/or integration tests
Commits messages follow AP format
Extend the documentation, if necessary
Merging to main from fork:branchname

Signed-off-by: Sanket Shevkar <[email protected]>

packages/concerto-core/lib/decoratormanager.js

mttrbrts

It looks like you're on the right track, @sanketshevkar. I look forward to seeing the perf numbers!

packages/concerto-core/lib/decoratormanager.js

Signed-off-by: sanketshevkar <[email protected]>

…d to decorate models Signed-off-by: sanketshevkar <[email protected]>

Signed-off-by: sanketshevkar <[email protected]>

packages/concerto-core/test/dcsIndexWrapper.js

packages/concerto-core/api.txt

packages/concerto-core/lib/decoratormanager.js

packages/concerto-core/test/dcsIndexWrapper.js

Signed-off-by: sanketshevkar <[email protected]>

dselman

Couple of minor changes. Looks good.

dselman · 2024-07-22T10:16:35Z

packages/concerto-core/lib/decoratormanager.js

+    /**
+     * Creates five different maps to index decorator command sets by target type and returns them
+     * @param {*} decoratorCommandSet the DecoratorCommandSet object
+     * @returns {Object} a new model manager with the decorations applied


Incorrect JSDoc.

dselman · 2024-07-22T10:17:28Z

packages/concerto-core/lib/decoratormanager.js

+
+        decoratorCommandSet.commands.map((decoratorCommand, index) => {
+            const dcsWithIndex = new DcsIndexWrapper(decoratorCommand, index);
+            switch (true) {


Is this switch supposed to be exhaustive? If yes, add a default case and throw an exception.

I was under the assumption that if we pass a invalid dcs, it will only get caught if validation is enabled. In the case of a malformed decorator, if we pass (in this case target is undefined, falsy, null or does not match with any of the switch statements) that decorator won't get applied.

This is the older behavior: https://replit.com/@sanketshevkar/Decorator-malformed-dcs#index.js
This differs a bit, I think is was a bug as well. shouldn't target: false be invalid dcs, but its passing the syntax validation. In either case, if we validate or not, it just applies the decorator on top of all the declarations, which should be a bug?

dselman · 2024-07-22T10:23:52Z

packages/concerto-core/lib/decoratormanager.js

     * @param {string} namespace the namespace for the declaration
     * @param {*} declaration the class declaration
     * @param {*} command the Command object from the dcs
-     * @param {boolean} [enableDcsNamespaceTarget] - flag to control applying namespace targeted decorators on top of the namespace instead of all declarations in that namespace
+     * @param {*} property the property


Looks like property is now an optional argument? Would be good to document that.

Yes corrected

dselman · 2024-07-22T10:28:01Z

packages/concerto-core/lib/decoratormanager.js

+ * [DecoratorCommandSet](https://models.accordproject.org/concerto/decorators.cto)
+ * @memberof module:concerto-core
+ */
+class DcsIndexWrapper {


dselman · 2024-07-22T10:28:28Z

packages/concerto-core/types/lib/dcsIndexWrapper.d.ts

Should not be exported? Private?

Yes made the method private

dselman · 2024-07-22T10:29:24Z

packages/concerto-core/types/lib/decoratormanager.d.ts

+     */
+    private static getDecoratorMaps;
+    /**
+     * Migrate or validate the DecoratorCommandSet object if the options are set as true


Migrate AND validate?

It can also do either of the operation if parameters for the arguments are passed so. It can only validate or it might only migrate and can do both if the parameters are true for both.

The only reason I moved this out of decorateModels is to reduce the code in the decorateModels method, so is a private method just used in decorateModels method.

Signed-off-by: sanketshevkar <[email protected]>

dselman

Bravo!

feat(dcs): optimize decorate models

95f88cb

Signed-off-by: Sanket Shevkar <[email protected]>

sanketshevkar requested review from dselman and mttrbrts June 7, 2024 17:31

sanketshevkar mentioned this pull request Jun 7, 2024

Optimizations for applying decorators on a model #856

Closed

sanketshevkar commented Jun 7, 2024

View reviewed changes

packages/concerto-core/lib/decoratormanager.js Outdated Show resolved Hide resolved

mttrbrts requested changes Jun 9, 2024

View reviewed changes

refactor(core): optimized decorateModels added

9ced9ac

Signed-off-by: sanketshevkar <[email protected]>

sanketshevkar changed the title ~~feat(dcs): optimize decorate models~~ feat(dcs): optimize decorate models - not ready for review Jul 15, 2024

sanketshevkar added 6 commits July 15, 2024 23:53

chore(core): code refactor and cleanup

7eeb27d

Signed-off-by: sanketshevkar <[email protected]>

chore(core): code refactor and cleanup

9983a96

Signed-off-by: sanketshevkar <[email protected]>

chore(core): new test added for validation of new and old methods use…

8372fb7

…d to decorate models Signed-off-by: sanketshevkar <[email protected]>

chore(core): code refactor

46a40d9

Signed-off-by: sanketshevkar <[email protected]>

chore(*): resolve merge conflict

9e09100

Signed-off-by: sanketshevkar <[email protected]>

chore(*): resolve merge conflict

368e90a

Signed-off-by: sanketshevkar <[email protected]>

sanketshevkar commented Jul 17, 2024

View reviewed changes

packages/concerto-core/test/dcsIndexWrapper.js Outdated Show resolved Hide resolved

sanketshevkar changed the title ~~feat(dcs): optimize decorate models - not ready for review~~ feat(dcs): optimize decorate models Jul 17, 2024

sanketshevkar marked this pull request as ready for review July 17, 2024 12:08

sanketshevkar requested a review from mttrbrts July 17, 2024 12:09

mttrbrts reviewed Jul 18, 2024

View reviewed changes

chore(core): resolve review comments and suggestions

b77eebe

Signed-off-by: sanketshevkar <[email protected]>

sanketshevkar requested a review from mttrbrts July 18, 2024 19:41

mttrbrts approved these changes Jul 19, 2024

View reviewed changes

mttrbrts requested review from ekarademir, jamieshorten and DianaLease July 19, 2024 12:48

dselman requested changes Jul 22, 2024

View reviewed changes

sanketshevkar added 2 commits July 22, 2024 17:25

chore(*): resolve PR suggestions and comments

422b3cc

Signed-off-by: sanketshevkar <[email protected]>

chore(*): resolve PR suggestions and comments

ee902e8

Signed-off-by: sanketshevkar <[email protected]>

sanketshevkar requested a review from dselman July 22, 2024 11:59

dselman approved these changes Jul 23, 2024

View reviewed changes

sanketshevkar merged commit bf3385c into accordproject:main Jul 24, 2024
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(dcs): optimize decorate models #857

feat(dcs): optimize decorate models #857

sanketshevkar commented Jun 7, 2024 •

edited

Loading

mttrbrts left a comment

dselman left a comment

dselman Jul 22, 2024

sanketshevkar Jul 22, 2024

dselman Jul 22, 2024

sanketshevkar Jul 22, 2024 •

edited

Loading

dselman Jul 22, 2024

sanketshevkar Jul 22, 2024

dselman Jul 22, 2024

dselman Jul 22, 2024

sanketshevkar Jul 22, 2024

dselman Jul 22, 2024

sanketshevkar Jul 22, 2024

dselman left a comment

feat(dcs): optimize decorate models #857

feat(dcs): optimize decorate models #857

Conversation

sanketshevkar commented Jun 7, 2024 • edited Loading

Closes #856

Changes

Performance and Memory Impact.

Performance

Memory

total_heap_size

used_heap_size

Author Checklist

mttrbrts left a comment

Choose a reason for hiding this comment

dselman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanketshevkar Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dselman left a comment

Choose a reason for hiding this comment

sanketshevkar commented Jun 7, 2024 •

edited

Loading

`total_heap_size`

`used_heap_size`

sanketshevkar Jul 22, 2024 •

edited

Loading