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

perf(storage): simplify table watermark index #15931

Merged
merged 9 commits into from
Mar 29, 2024

Conversation

wenym1
Copy link
Contributor

@wenym1 wenym1 commented Mar 26, 2024

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

Avoid building per vnode table watermark index.

Checklist

  • I have written necessary rustdoc comments
  • I have added necessary unit tests and integration tests
  • I have added test labels as necessary. See details.
  • I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).
  • My PR contains breaking changes. (If it deprecates some features, please create a tracking issue to remove them in the future).
  • All checks passed in ./risedev check (or alias, ./risedev c)
  • My PR changes performance-critical code. (Please run macro/micro-benchmarks and show the results.)
  • My PR contains critical fixes that are necessary to be merged into the latest release. (Please check out the details)

Documentation

  • My PR needs documentation updates. (Please use the Release note section below to summarize the impact on users)

Release note

If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.

@github-actions github-actions bot added the type/fix Bug fix label Mar 26, 2024
@wenym1
Copy link
Contributor Author

wenym1 commented Mar 28, 2024

Added benchmark.

Benchmark result is as followed. The test setting is, 500 epochs between safe epoch and committed epoch, and 500 staging epochs. Vnodes are partitioned into 16 parts.

This branch

new pinned version      time:   [37.867 µs 38.279 µs 38.689 µs]
                        change: [-100.000% -100.000% -100.000%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 7 outliers among 100 measurements (7.00%)
  3 (3.00%) high mild
  4 (4.00%) high severe

apply committed watermark
                        time:   [161.95 µs 167.16 µs 172.45 µs]
                        change: [-99.989% -99.989% -99.988%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
  3 (3.00%) high mild

read latest watermark   time:   [2.4513 µs 2.5030 µs 2.6131 µs]
                        change: [+2.5368% +3.8159% +6.2119%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 3 outliers among 100 measurements (3.00%)
  1 (1.00%) high mild
  2 (2.00%) high severe

read committed watermark
                        time:   [81.068 µs 81.169 µs 81.271 µs]
                        change: [-0.2821% -0.0573% +0.1584%] (p = 0.61 > 0.05)
                        No change in performance detected.

main branch

read latest watermark   time:   [16.029 µs 16.058 µs 16.088 µs]
                        change: [+0.5389% +0.9036% +1.2732%] (p = 0.00 < 0.05)
                        Change within noise threshold.
Found 6 outliers among 100 measurements (6.00%)
  2 (2.00%) low mild
  4 (4.00%) high mild

read committed watermark
                        time:   [9.9623 µs 9.9815 µs 10.004 µs]
                        change: [-3.9867% -2.9393% -2.2220%] (p = 0.00 < 0.05)
                        Performance has improved.

The time for build_index and apply_committed_watermarks can be ignored in this branch compared to the main branch, because there is no index to maintain. The time in the main branch is so great that we had to reduce the sample size to be able to finish the test in reasonable time.

For query, the time to query the latest epoch is 85% lower than the main branch, and the time to query a middle epoch is 7 times the main branch. This is because the implementation is linearly searching from latest epoch to old epoch, and therefore is't biased for querying the watermark in the latest epoch.

In conclusion, compared to the main branch, this PR save the significant time to build and maintain the table watermark index. In terms of query, this branch is even faster than the main branch in querying the latest epoch. It only performs poorly in querying a mvcc epoch, which is not a common use case.

@wenym1 wenym1 requested review from hzxa21 and Li0k March 28, 2024 13:03
Copy link
Collaborator

@hzxa21 hzxa21 left a comment

Choose a reason for hiding this comment

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

LSTM! The micro-bench result looks good! Thanks for the quick fix.

.cloned()
})
// iterate from new epoch to old epoch
for (watermark_epoch, vnode_watermark_list) in self.staging_watermarks.iter().rev().chain(
Copy link
Collaborator

Choose a reason for hiding this comment

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

the time to query a middle epoch is 7 times the main branch. This is because the implementation is linearly searching from latest epoch to old epoch, and therefore is't biased for querying the watermark in the latest epoch

This is a note only: I guess we can use binary search to make query from non-latest epoch faster but given that only internal state table contains table watermark and internal state table reads are all latest, we don't need to worry about that right now.

@hzxa21 hzxa21 changed the title fix(storage): simplify table watermark index perf(storage): simplify table watermark index Mar 28, 2024
@github-actions github-actions bot added type/perf and removed type/fix Bug fix labels Mar 28, 2024
@wenym1 wenym1 enabled auto-merge March 29, 2024 03:45
@wenym1 wenym1 added this pull request to the merge queue Mar 29, 2024
Merged via the queue into main with commit f396195 Mar 29, 2024
29 of 30 checks passed
@wenym1 wenym1 deleted the yiming/simplify-table-watermark branch March 29, 2024 05:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants