Skip to content

Conversation

jbj
Copy link
Contributor

@jbj jbj commented Aug 27, 2019

No description provided.

@jbj jbj added the C++ label Aug 27, 2019
@jbj jbj requested a review from a team as a code owner August 27, 2019 14:20
@jbj jbj added this to the 1.22 milestone Aug 27, 2019
@jbj jbj added the Priority PR that should be reviewed and merged as a matter of priority. label Aug 27, 2019
@jbj jbj force-pushed the existsCompleteWithName-perf branch from 36cee9f to 3700a63 Compare August 27, 2019 14:29
@geoffw0
Copy link
Contributor

geoffw0 commented Aug 27, 2019

Code change LGTM.

@pavgust pavgust merged commit 1bd0c69 into github:master Aug 27, 2019
@jbj
Copy link
Contributor Author

jbj commented Aug 29, 2019

@pavgust I've tried to investigate the cause of this performance regression in code that hasn't changed for a long time. I think the slowdown is a side effect of an optimiser improvement, possibly the fix for QL-878. Whatever it was, it caused a slight simplification of how the strictcount range and term were compiled, which meant there was no longer a need for a #shared predicate. But I think that #shared predicate was helping performance by deduplicating tuples in the pipeline in exactly the place where it happened to be needed.

In conclusion, I think the optimiser is to blame for this fix being necessary, but I think the optimiser is doing a better job in 1.22 than in 1.21 even though random chance meant this PR went from being nice-to-have to being critical.

For the record, this is the RA from 1.21:

EVALUATE NONRECURSIVE RELATION:
  SYNTHETIC ResolveClass::existsCompleteWithName#ff#shared(unique int arg0, extensional string arg1) :-
    {1} r1 = JOIN is_complete WITH ResolveClass::getTopLevelClassName#ff ON is_complete.<0>=ResolveClass::getTopLevelClassName#ff.<0> OUTPUT FIELDS {ResolveClass::getTopLevelClassName#ff.<1>}
    {2} r2 = JOIN r1 WITH ResolveClass::getTopLevelClassName#ff_10#join_rhs ON r1.<0>=ResolveClass::getTopLevelClassName#ff_10#join_rhs.<0> OUTPUT FIELDS {ResolveClass::getTopLevelClassName#ff_10#join_rhs.<1>,r1.<0>}
    return r2

EVALUATE NONRECURSIVE RELATION:
  SYNTHETIC ResolveClass::existsCompleteWithName#ff#count_range(extensional string arg0, unique int arg1) :-
    {2} r1 = JOIN ResolveClass::existsCompleteWithName#ff#shared WITH is_complete ON ResolveClass::existsCompleteWithName#ff#shared.<0>=is_complete.<0> OUTPUT FIELDS {ResolveClass::existsCompleteWithName#ff#shared.<1>,ResolveClass::existsCompleteWithName#ff#shared.<0>}
    return r1

EVALUATE NONRECURSIVE RELATION:
  SYNTHETIC ResolveClass::existsCompleteWithName#ff#count_term(extensional string arg0, unique int arg1) :-
    {2} r1 = JOIN ResolveClass::existsCompleteWithName#ff#shared WITH is_complete ON ResolveClass::existsCompleteWithName#ff#shared.<0>=is_complete.<0> OUTPUT FIELDS {ResolveClass::existsCompleteWithName#ff#shared.<0>,ResolveClass::existsCompleteWithName#ff#shared.<1>}
    {2} r2 = JOIN r1 WITH usertypes ON r1.<0>=usertypes.<0> OUTPUT FIELDS {r1.<1>,r1.<0>}
    return r2

EVALUATE NONRECURSIVE RELATION:
  SYNTHETIC ResolveClass::existsCompleteWithName#ff#join_rhs(extensional string arg0, int arg1) :-
    {2} r1 = AGGREGATE ResolveClass::existsCompleteWithName#ff#count_range, ResolveClass::existsCompleteWithName#ff#count_term ON {} WITH COUNT OUTPUT {ResolveClass::existsCompleteWithName#ff#count_term.<0>,agg.<0>}
    return r1

EVALUATE NONRECURSIVE RELATION:
  ResolveClass::existsCompleteWithName#ff(extensional string name, unique int d) :-
    {2} r1 = JOIN is_complete WITH ResolveClass::getTopLevelClassName#ff ON is_complete.<0>=ResolveClass::getTopLevelClassName#ff.<0> OUTPUT FIELDS {ResolveClass::getTopLevelClassName#ff.<1>,is_complete.<0>}
    {3} r2 = JOIN r1 WITH ResolveClass::existsCompleteWithName#ff#join_rhs ON r1.<0>=ResolveClass::existsCompleteWithName#ff#join_rhs.<0> OUTPUT FIELDS {r1.<1>,r1.<0>,ResolveClass::existsCompleteWithName#ff#join_rhs.<1>}
    {3} r3 = SELECT r2 ON CONSTANT r2.<2>=1
    {2} r4 = SCAN r3 OUTPUT FIELDS {r3.<1>,r3.<0>}
    return r4

This is the RA from 1.22:

EVALUATE NONRECURSIVE RELATION:
  SYNTHETIC ResolveClass::existsCompleteWithName#ff#count_range(extensional string arg0, unique int arg1) :-
    {1} r1 = JOIN is_complete WITH ResolveClass::getTopLevelClassName#ff ON is_complete.<0>=ResolveClass::getTopLevelClassName#ff.<0> OUTPUT FIELDS {ResolveClass::getTopLevelClassName#ff.<1>}
    {2} r2 = JOIN r1 WITH ResolveClass::getTopLevelClassName#ff_10#join_rhs ON r1.<0>=ResolveClass::getTopLevelClassName#ff_10#join_rhs.<0> OUTPUT FIELDS {ResolveClass::getTopLevelClassName#ff_10#join_rhs.<1>,r1.<0>}
    {2} r3 = JOIN r2 WITH is_complete ON r2.<0>=is_complete.<0> OUTPUT FIELDS {r2.<1>,r2.<0>}
    return r3

EVALUATE NONRECURSIVE RELATION:
  SYNTHETIC ResolveClass::existsCompleteWithName#ff#join_rhs(extensional string arg0, int arg1) :-
    {2} r1 = AGGREGATE ResolveClass::existsCompleteWithName#ff#count_range, ResolveClass::existsCompleteWithName#ff#count_range ON {} WITH COUNT OUTPUT {ResolveClass::existsCompleteWithName#ff#count_range.<0>,agg.<0>}
    return r1

EVALUATE NONRECURSIVE RELATION:
  ResolveClass::existsCompleteWithName#ff(extensional string name, unique int d) :-
    {2} r1 = JOIN is_complete WITH ResolveClass::getTopLevelClassName#ff ON is_complete.<0>=ResolveClass::getTopLevelClassName#ff.<0> OUTPUT FIELDS {ResolveClass::getTopLevelClassName#ff.<1>,is_complete.<0>}
    {3} r2 = JOIN r1 WITH ResolveClass::existsCompleteWithName#ff#join_rhs ON r1.<0>=ResolveClass::existsCompleteWithName#ff#join_rhs.<0> OUTPUT FIELDS {r1.<1>,r1.<0>,ResolveClass::existsCompleteWithName#ff#join_rhs.<1>}
    {3} r3 = SELECT r2 ON CONSTANT r2.<2>=1
    {2} r4 = SCAN r3 OUTPUT FIELDS {r3.<1>,r3.<0>}
    return r4

@jbj
Copy link
Contributor Author

jbj commented Aug 29, 2019

(RA edited)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C++ Priority PR that should be reviewed and merged as a matter of priority.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants