Skip to content

Conversation

@antiguru
Copy link
Member

@antiguru antiguru commented Jan 10, 2023

Similarly to #17056, use a non-allocating monoid for all top-k aggregations.

This currently depends on a change to Differential (TimelyDataflow/differential-dataflow#375) before it can land (it has landed.)

Checklist

  • This PR has adequate test coverage / QA involvement has been duly considered.

  • This PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way) and therefore is tagged with a T-proto label.

  • If this PR will require changes to cloud orchestration, there is a
    companion cloud PR to account for those changes that is tagged with
    the release-blocker label (example).

  • This PR includes the following user-facing behavior changes:

antiguru and others added 3 commits January 13, 2023 11:34
Introduce a top-1 monoid that shares the column order among all peers and
retains buffers for efficient row unpacking. This allows for
allocation-free row comparisons where otherwise the row needs to be
unpacked into a new vector. The implementation relies on
reference-counted shared state, which makes it unsuitable for sharing
across thread boundaries.

Signed-off-by: Moritz Hoffmann <mh@materialize.com>
Signed-off-by: Moritz Hoffmann <mh@materialize.com>
@vmarcos
Copy link
Contributor

vmarcos commented Jan 19, 2023

This PR extends #17056 with improvements for monotonic top-1 aggregation, namely reuse of column order across top-1 monoids as well as per-worker pre-aggregation. However, in a local development environment, these improvements have not yet shown performance benefits as documented in an internal spreadsheet:

Monotonic Query Time - 1 worker [ms] Query Time - 8 workers [ms] Speedup Dataflow records Time Factor - 1 worker Time Factor - 8 workers Records Factor
Top-1 2,393.51 366.63 6.53 3 1.02 1.01 1

The settings used for the evaluation are the same described in #17056 (comment). Here, the time and record factors are wrt. main, and show no real benefit from the techniques added in the setting evaluated.

After discussion with @antiguru, we felt that the draft PR #17058 adds quite a bit of complexity for a relatively small (at this point) benefit. So we are suggesting leaving the top-1 improvement in #17058 in the icebox, perhaps for when we actually can see some benefit of per-worker pre-aggregation for top-1, e.g., in distributed dataflow executions.

@antiguru antiguru closed this Jan 19, 2023
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