NEW (GraphEngine): @W-11989381@: New MultipleMassSchemaLookupRule to detect performance degrading schema lookups. #1054

rmohan20 · 2023-04-17T16:56:39Z

No description provided.

rmohan20 · 2023-04-25T21:18:02Z

sfge/src/main/java/com/salesforce/config/UserFacingMessages.java

+        public static final String MESSAGE_TEMPLATE =
+                "Avoid excessive Schema lookups in a single path. Detected %s %s at %s:%d.";
+        public static final String OCCURRENCE_LOOP_TEMPLATE = "inside a %s";
+        public static final String OCCURRENCE_MULTIPLE_TEMPLATE = "preceded by a call to %s";


Will update these based on UI text review.

rmohan20 · 2023-04-25T21:28:26Z

sfge/src/main/java/com/salesforce/graph/ops/MethodUtil.java

No real changes. spotless doing its thing.

rmohan20 · 2023-04-25T21:29:01Z

sfge/src/main/java/com/salesforce/graph/ops/expander/ApexPathExpander.java

spotless, can be ignored.

rmohan20 · 2023-04-25T21:29:28Z

sfge/src/main/java/com/salesforce/graph/visitor/DefaultNoOpPathVertexVisitor.java

New afterVisit methods for loop vertices.

rmohan20 · 2023-04-25T21:37:03Z

sfge/src/main/java/com/salesforce/rules/PathBasedRuleRunner.java

                        if (ruleThrowable instanceof FlsViolationInfo) {
                            incompleteThrowables.add((FlsViolationInfo) ruleThrowable);
+                        } else if (ruleThrowable instanceof MassSchemaLookupInfo) {
+                            tempDeleteMe.add((MassSchemaLookupInfo) ruleThrowable);


Needs a minor refactor to clean up. Also, there seems to be a bug with multiple rule types. I'll address this in another commit.

rmohan20 · 2023-05-04T00:37:20Z

sfge/src/main/java/com/salesforce/config/UserFacingMessages.java

+    public static final class MultipleMassSchemaLookupRuleTemplates {
+        public static final String MESSAGE_TEMPLATE = "%s was %s at %s:%d.";
+        public static final String OCCURRENCE_LOOP_TEMPLATE = "called inside a %s";
+        public static final String OCCURRENCE_MULTIPLE_TEMPLATE = "preceded by a call to %s";
+    }


Approved UI text.

rmohan20 · 2023-05-04T00:38:29Z

sfge/src/main/java/com/salesforce/graph/visitor/DefaultNoOpPathVertexVisitor.java

Opening up loop vertices in visitor.

rmohan20 · 2023-05-04T00:39:23Z

sfge/src/main/java/com/salesforce/rules/MultipleMassSchemaLookupRule.java

+
+    @Override
+    protected boolean isEnabled() {
+        return true;


Rule is now enabled.

rmohan20 · 2023-05-04T00:39:58Z

sfge/src/main/java/com/salesforce/rules/PathBasedRuleRunner.java

                        if (ruleThrowable instanceof FlsViolationInfo) {
-                            incompleteThrowables.add((FlsViolationInfo) ruleThrowable);
+                            flsViolationInfos.add((FlsViolationInfo) ruleThrowable);
+                        } else if (ruleThrowable instanceof MultipleMassSchemaLookupInfo) {
+                            // FIXME: PR incoming with refactors to this portion
+                            mmsLookupInfos.add((MultipleMassSchemaLookupInfo) ruleThrowable);


There's some duplication here that can be streamlined.

I'm assuming the subsequent PR will involve refactoring this so it doesn't require a Set of MMS violations specifically?

Right, I'm trying to move the ownership of conversion to the ViolationInfo object so that this portion can work on the higher-level interface.

jfeingold35 · 2023-05-04T20:21:35Z

sfge/src/main/java/com/salesforce/rules/PathBasedRuleRunner.java

                        if (ruleThrowable instanceof FlsViolationInfo) {
-                            incompleteThrowables.add((FlsViolationInfo) ruleThrowable);
+                            flsViolationInfos.add((FlsViolationInfo) ruleThrowable);
+                        } else if (ruleThrowable instanceof MultipleMassSchemaLookupInfo) {
+                            // FIXME: PR incoming with refactors to this portion
+                            mmsLookupInfos.add((MultipleMassSchemaLookupInfo) ruleThrowable);


I'm assuming the subsequent PR will involve refactoring this so it doesn't require a Set of MMS violations specifically?

jfeingold35 · 2023-05-04T20:30:49Z

...est/java/com/salesforce/rules/multiplemassschemalookup/MultipleMassSchemaLookupRuleTest.java

+                "public class MyClass {\n"
+                        + "   void foo() {\n"
+                        + "       "
+                        + firstCall
+                        + "();\n"
+                        + "       "
+                        + secondCall
+                        + "();\n"
+                        + "   }\n"
+                        + "}\n";


For the future, you can use spotless:off and spotless:on to avoid lines being split like this. Keeps the code readable.

Agree. This got missed. Will clean it up.

jfeingold35 · 2023-05-04T20:31:53Z

...est/java/com/salesforce/rules/multiplemassschemalookup/MultipleMassSchemaLookupRuleTest.java

+    }
+
+    @Test
+    public void testLoopAfterGgdShouldNotGetViolation() {


This is a good test. Do we also want one for a loop before a GGD as a counterpart to this one? Also possibly a loop, then a separate loop with a GGD, to verify that it doesn't stop looking after the first loop?

Just tried it out and the test fails. All great use cases. Will include them.

jfeingold35 · 2023-05-04T20:37:49Z

...main/java/com/salesforce/rules/multiplemassschemalookup/MultipleMassSchemaLookupVisitor.java

+        if (vertex.equals(sinkVertex)) {
+            // Mark sink as visited. From this point on, we don't need to
+            // look for anymore loops or additional calls.
+            // TODO: A more performant approach would be to stop walking path from this point


Is the idea that once a path is confirmed to violate the rule, there's no sense in continuing to check?

This is to avoid continuing to look beyond the sink vertex.

jfeingold35 · 2023-05-04T20:44:23Z

...main/java/com/salesforce/rules/multiplemassschemalookup/MultipleMassSchemaLookupVisitor.java

+    /**
+     * @return Violations collected by the rule.
+     */
+    Set<MultipleMassSchemaLookupInfo> getViolation() {


Pedantic note: Should this be getViolations, plural, or is it only possible for there to be one violation?

Should be plural. I had forgotten to rename it after a change in structure where a single violation will be returned.

jfeingold35 · 2023-05-04T20:47:44Z

.../java/com/salesforce/rules/multiplemassschemalookup/MultipleMassSchemaLookupRuleHandler.java

+        // TODO: I'm expecting to add other visitors depending on the other factors we will analyze
+        // to decide if a GGD call is not performant.
+        DefaultSymbolProviderVertexVisitor symbols = new DefaultSymbolProviderVertexVisitor(g);
+        ApexPathWalker.walkPath(g, path, ruleVisitor, symbols);


Performance question: Will this cause redundant path walking for the case where there's multiple GGDs? It seems like we'd walk the path once for the first sink, then walk it again for the second sink (and that second time would cause a violation when it reaches the sink from earlier). Am I missing something?

You're right. This is a problem that ApexFlsViolationRule has as well. The path walking should be relatively more performant than the initial path expansion. But certainly something to improve.

jfeingold35 · 2023-05-04T20:50:16Z

...main/java/com/salesforce/rules/multiplemassschemalookup/MultipleMassSchemaLookupVisitor.java

+
+    @Override
+    protected void execAfterLoopVertexVisit(BaseSFVertex vertex, SymbolProvider symbols) {
+        if (shouldContinue()) {


I'm not quite sure I understand what's going on here. This looks like it would throw a violation for:

for (int i = 0; i < 5; i++) { //some code }} getGlobalDescribe()

How does it avoid doing that? I don't understand how it knows when it exits the loop.

Ahh! I'm just dumb. You're spot on. Let me rework this.

rmohan20 commented Apr 25, 2023

View reviewed changes

sfge/src/main/java/com/salesforce/graph/ops/MethodUtil.java

Copy link

Contributor Author

rmohan20 Apr 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No real changes. spotless doing its thing.

rmohan20 commented Apr 25, 2023

View reviewed changes

sfge/src/main/java/com/salesforce/graph/ops/expander/ApexPathExpander.java

Copy link

Contributor Author

rmohan20 Apr 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spotless, can be ignored.

rmohan20 commented Apr 25, 2023

View reviewed changes

rmohan20 force-pushed the rm/ggdInLoop1 branch from dabb806 to 3bcf38c Compare April 25, 2023 21:44

rmohan20 marked this pull request as ready for review April 25, 2023 21:46

rmohan20 changed the title ~~Draft: Outline of GetGlobalDescribeViolationRule~~ @W-11989381@: New MultipleMassSchemaLookupRule to detect performance degrading schema lookups Apr 25, 2023

rmohan20 added 6 commits April 28, 2023 17:00

Outline of GGD in loop rule

7392501

Basics + tests before changing violation generation

432d5a7

Fixing asserts on tests, adding more rule scenarios

33d27c7

Renaming to MultipleMassSchemaLookupRule

905f2e4

Renames to match updated rule name

d21844a

Fixing NPE on comparator issue

4a48ffc

rmohan20 force-pushed the rm/ggdInLoop1 branch from 3bcf38c to 4a48ffc Compare May 2, 2023 00:19

rmohan20 changed the base branch from rm/instanceMethodInForLoop-part1 to dev May 3, 2023 18:59

rmohan20 added 2 commits May 3, 2023 15:42

Applying UI text changes + minor renames

0946b07

Enabling MMSLookupRule + format fixes

81eb5d3

rmohan20 commented May 4, 2023

View reviewed changes

sfge/src/main/java/com/salesforce/graph/visitor/DefaultNoOpPathVertexVisitor.java

Copy link

Contributor Author

rmohan20 May 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opening up loop vertices in visitor.

rmohan20 commented May 4, 2023

View reviewed changes

Minor cleanup

3b1b738

jfeingold35 reviewed May 4, 2023

View reviewed changes

Feedback items. Loop exit detection not included

8524450

jfeingold35 approved these changes May 4, 2023

View reviewed changes

jfeingold35 changed the title ~~@W-11989381@: New MultipleMassSchemaLookupRule to detect performance degrading schema lookups~~ NEW (GraphEngine): @W-11989381@: New MultipleMassSchemaLookupRule to detect performance degrading schema lookups May 4, 2023

rmohan20 changed the title ~~NEW (GraphEngine): @W-11989381@: New MultipleMassSchemaLookupRule to detect performance degrading schema lookups~~ NEW (GraphEngine): @W-11989381@: New MultipleMassSchemaLookupRule to detect performance degrading schema lookups. May 4, 2023

Merge branch 'dev' into rm/ggdInLoop1

3c556cc

rmohan20 merged commit 6bba121 into dev May 4, 2023

jfeingold35 deleted the rm/ggdInLoop1 branch January 16, 2024 15:44

NEW (GraphEngine): @W-11989381@: New MultipleMassSchemaLookupRule to detect performance degrading schema lookups. #1054

NEW (GraphEngine): @W-11989381@: New MultipleMassSchemaLookupRule to detect performance degrading schema lookups. #1054

Uh oh!

Conversation

rmohan20 commented Apr 17, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants