Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't reapply norms already applied in the same session #53

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tobias
Copy link

@tobias tobias commented May 18, 2023

This makes norm application faster by not repeatedly applying the same
norms. This can be slow when there are many norms with a deep dependency
tree.

In order to implement this, the structure was changed significantly:

  • the reduction accumulator is no longer passed all the way down to
    reduce-txes
  • error-handling is now top-level

We are currently using a fork of conformity at Shortcut that includes this change because we have a fairly complicated norm dependency tree. Before this change, a full conformity run against a fresh db would take 90 seconds. With this change, it now takes 1.8 seconds.

I'm offering it upstream in case it is useful for others.

This makes norm application faster by not repeatedly applying the same
norms. This can be slow when there are many norms with a deep dependency
tree.

In order to implement this, the structure was changed significantly:

- the reduction accumulator is no longer passed all the way down to
  `reduce-txes`
- error-handling is now top-level
Comment on lines +239 to +252
acc (-> acc
(update :results concat (:results requires-results))
(update :applied-norms into (:applied-norms requires-results)))]
(if (contains? (:applied-norms acc) norm-name)
acc
(try
(-> acc
(update :results concat
(if first-time-only
(handle-first-time-only-norm conn norm-attr norm-map norm-name
sync-schema-timeout tx-instant)
(handle-mutable-norm conn norm-attr norm-map norm-name
sync-schema-timeout tx-instant)))
(update :applied-norms conj norm-name))
Copy link
Author

Choose a reason for hiding this comment

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

This is the heart of the change; we keep a set of norms we have processed in the current session, and skip processing for a norm if we see it again in the dependency tree.

It may be better to topologically sort the dependency tree and walk it once, but that would have been a bigger change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants