Go: Fix bad join order in comparesFirstCharacter

[owen-mc]

Old join order (on uber/aresdb):

[2025-02-20 12:19:36] Evaluated non-recursive predicate StringOps::StringOps::HasPrefix::comparesFirstCharacter/3#24aa5857@5bcd38hd in 3ms (size: 1).
Evaluated relational algebra for predicate StringOps::StringOps::HasPrefix::comparesFirstCharacter/3#24aa5857@5bcd38hd with tuple counts:
           6528  ~0%    {3} r1 = JOIN project#DataFlowUtil::EqualityTestNode#9fb3a74a#2 WITH `DataFlowUtil::BinaryOperationNode.hasOperands/2#dispred#c32311b8` ON FIRST 1 OUTPUT Rhs.1, Lhs.0, Rhs.2
           6562  ~4%    {3}    | JOIN WITH `GlobalValueNumbering::globalValueNumber/1#39395ec1` ON FIRST 1 OUTPUT Rhs.1, Lhs.1, Lhs.2
        1766593  ~1%    {3}    | JOIN WITH `GlobalValueNumbering::globalValueNumber/1#39395ec1_10#join_rhs` ON FIRST 1 OUTPUT Rhs.1, Lhs.1, Lhs.2
             89  ~0%    {5}    | JOIN WITH `DataFlowUtil::ElementReadNode.getIndex/0#dispred#9e685477` ON FIRST 1 OUTPUT Rhs.1, _, Lhs.1, Lhs.2, Lhs.0
             89  ~0%    {5}    | REWRITE WITH Out.1 := 0
             17  ~0%    {3}    | JOIN WITH `DataFlowUtil::Node.getIntValue/0#dispred#95c22b0f#fb` ON FIRST 2 OUTPUT Lhs.4, Lhs.2, Lhs.3
             17  ~0%    {3}    | JOIN WITH `DataFlowUtil::ComponentReadNode.getBase/0#dispred#0757f222` ON FIRST 1 OUTPUT Rhs.1, Lhs.1, Lhs.2
             17  ~0%    {4}    | JOIN WITH `DataFlowUtil::InstructionNode.getType/0#dispred#98c46cd8` ON FIRST 1 OUTPUT Rhs.1, Lhs.1, Lhs.2, Lhs.0
             17  ~0%    {4}    | JOIN WITH `Types::Type.getUnderlyingType/0#dispred#e61d6b4a` ON FIRST 1 OUTPUT Rhs.1, Lhs.1, Lhs.2, Lhs.3
              1  ~0%    {3}    | JOIN WITH @stringtype ON FIRST 1 OUTPUT Lhs.1, Lhs.3, Lhs.2
                        return r1

After the change:

[2025-02-20 14:46:40] Evaluated non-recursive predicate StringOps::StringOps::HasPrefix::comparesFirstCharacter/3#24aa5857@6a98c74d in 0ms (size: 1).
Evaluated relational algebra for predicate StringOps::StringOps::HasPrefix::comparesFirstCharacter/3#24aa5857@6a98c74d with tuple counts:
        1383  ~9%    {3} r1 = SCAN `DataFlowUtil::ElementReadNode.getIndex/0#dispred#9e685477` OUTPUT In.1, _, In.0
        1383  ~0%    {3}    | REWRITE WITH Out.1 := 0
         318  ~0%    {1}    | JOIN WITH `DataFlowUtil::Node.getIntValue/0#dispred#95c22b0f#fb` ON FIRST 2 OUTPUT Lhs.2
         318  ~0%    {2}    | JOIN WITH `DataFlowUtil::ComponentReadNode.getBase/0#dispred#0757f222` ON FIRST 1 OUTPUT Lhs.0, Rhs.1
         318  ~1%    {2}    | JOIN WITH `GlobalValueNumbering::globalValueNumber/1#39395ec1` ON FIRST 1 OUTPUT Rhs.1, Lhs.1
         669  ~0%    {2}    | JOIN WITH `GlobalValueNumbering::globalValueNumber/1#39395ec1_10#join_rhs` ON FIRST 1 OUTPUT Lhs.1, Rhs.1
         669  ~0%    {3}    | JOIN WITH `DataFlowUtil::InstructionNode.getType/0#dispred#98c46cd8` ON FIRST 1 OUTPUT Rhs.1, Lhs.0, Lhs.1
         669  ~0%    {3}    | JOIN WITH `Types::Type.getUnderlyingType/0#dispred#e61d6b4a` ON FIRST 1 OUTPUT Rhs.1, Lhs.1, Lhs.2
           1  ~0%    {2}    | JOIN WITH @stringtype ON FIRST 1 OUTPUT Lhs.2, Lhs.1
           1  ~0%    {3}    | JOIN WITH `DataFlowUtil::BinaryOperationNode.hasOperands/2#dispred#c32311b8_102#join_rhs` ON FIRST 1 OUTPUT Rhs.1, Lhs.1, Rhs.2
           1  ~0%    {3}    | JOIN WITH project#DataFlowUtil::EqualityTestNode#9fb3a74a#2 ON FIRST 1 OUTPUT Lhs.0, Lhs.1, Lhs.2
                     return r1

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

Tip: If you use Visual Studio Code, you can request a review from Copilot before you push from the "Source Control" tab. Learn more

[owen-mc]

DCA shows it fixing the bad join order on all the repos 🎉 .

[smowton]

You may as well go one better: right now it's saying, "take all the e[0] statements, find all their gvn-aliases, then check if e is a string, and finally check this is an equality-test LHS". You might as well move the "check e is a string" bit before the inherently-quadratic map through GVN, which I think would be achieved by only tagging the read in the context of the GVN lookup, or perhaps it would be better to kick the "find e[0] such that e is a string" out into an inline-late predicate.

[owen-mc]

perhaps it would be better to kick the "find e[0] such that e is a string" out into an inline-late predicate.

Surely that's the part you want to do early.

I did try only putting the binding pragma in the GVN part. It had a slightly worse join order because it choose to do "reads of strings" first. But you may be right that it protects us better from the worst case.

[smowton]

Yeah I'd say that's better, in that it's linear in the size of the database -- ideally we should restrict the pool as small as possible before going through GVN, because that's where the quadratic term goes in.

[owen-mc]

The docs on join orders say that there isn't any way to use pragmas to make a predicate go last, so I pulled what we want out as a pragma[noinline] predicate and made that go first. Within that I made it start with all reads of an index rather than all string-typed nodes as that produced fewer tuples on the db I'm testing on, and I intuitively it feels better in general. DCA looks good.

[aschackmull]

The docs on join orders say that there isn't any way to use pragmas to make a predicate go last

Amended: https://github.com/github/codeql-team/pull/3785

[owen-mc]

@aschackmull Would inline_late help here? There are three predicates that reference read and I thought that if I put the one I want to come last in an inline_late predicate then it could still be be ordered in the middle, because read has been bound.

[aschackmull]

@aschackmull Would inline_late help here?

I haven't looked into this particular case - I was just triggered to amend the doc based on your statement.

There are three predicates that reference read and I thought that if I put the one I want to come last in an inline_late predicate then it could still be be ordered in the middle, because read has been bound.

True. What you did in this particular case looks reasonable to me. (Although the RA you posted as the "after" version above doesn't look right - you have two predicates instead of one now, so I'd expect two pipelines in the "after" version).

[owen-mc]

Yes, that RA is out of date. With the latest version of the code that uses a separate noinline predicate it is:

[2025-02-21 00:58:27] Evaluated non-recursive predicate StringOps::StringOps::HasPrefix::getReadOfFirstChar/1#c1b85cb9@66e653ef in 0ms (size: 1).
Evaluated relational algebra for predicate StringOps::StringOps::HasPrefix::getReadOfFirstChar/1#c1b85cb9@66e653ef with tuple counts:
        1383  ~0%    {3} r1 = SCAN `DataFlowUtil::ElementReadNode.getIndex/0#dispred#9e685477` OUTPUT In.1, _, In.0
        1383  ~0%    {3}    | REWRITE WITH Out.1 := 0
         318  ~0%    {1}    | JOIN WITH `DataFlowUtil::Node.getIntValue/0#dispred#95c22b0f#fb` ON FIRST 2 OUTPUT Lhs.2
         318  ~0%    {2}    | JOIN WITH `DataFlowUtil::ComponentReadNode.getBase/0#dispred#0757f222` ON FIRST 1 OUTPUT Rhs.1, Lhs.0
         318  ~0%    {3}    | JOIN WITH `DataFlowUtil::InstructionNode.getType/0#dispred#98c46cd8` ON FIRST 1 OUTPUT Rhs.1, Lhs.1, Lhs.0
         318  ~0%    {3}    | JOIN WITH `Types::Type.getUnderlyingType/0#dispred#e61d6b4a` ON FIRST 1 OUTPUT Rhs.1, Lhs.1, Lhs.2
           1  ~0%    {2}    | JOIN WITH @stringtype ON FIRST 1 OUTPUT Lhs.2, Lhs.1
                     return r1

[2025-02-21 00:58:28] Evaluated non-recursive predicate StringOps::StringOps::HasPrefix::comparesFirstCharacter/3#24aa5857@7a72d6t5 in 0ms (size: 1).
Evaluated relational algebra for predicate StringOps::StringOps::HasPrefix::comparesFirstCharacter/3#24aa5857@7a72d6t5 with tuple counts:
        1  ~0%    {2} r1 = SCAN `StringOps::StringOps::HasPrefix::getReadOfFirstChar/1#c1b85cb9` OUTPUT In.1, In.0
        1  ~0%    {2}    | JOIN WITH `GlobalValueNumbering::globalValueNumber/1#39395ec1` ON FIRST 1 OUTPUT Rhs.1, Lhs.1
        1  ~0%    {2}    | JOIN WITH `GlobalValueNumbering::globalValueNumber/1#39395ec1_10#join_rhs` ON FIRST 1 OUTPUT Rhs.1, Lhs.1
        1  ~0%    {3}    | JOIN WITH `DataFlowUtil::BinaryOperationNode.hasOperands/2#dispred#c32311b8_102#join_rhs` ON FIRST 1 OUTPUT Rhs.1, Lhs.1, Rhs.2
        1  ~0%    {3}    | JOIN WITH project#DataFlowUtil::EqualityTestNode#9fb3a74a#2 ON FIRST 1 OUTPUT Lhs.0, Lhs.1, Lhs.2
                  return r1