Skip to content

backport: performance improvements for the event query API (#7319)#9334

Merged
cmwaters merged 12 commits intotendermint:mainfrom
mmsqe:adr075/main
Sep 13, 2022
Merged

backport: performance improvements for the event query API (#7319)#9334
cmwaters merged 12 commits intotendermint:mainfrom
mmsqe:adr075/main

Conversation

@mmsqe
Copy link

@mmsqe mmsqe commented Aug 30, 2022


PR checklist

  • Tests written/updated, or no tests needed
  • CHANGELOG_PENDING.md updated, or no changelog entry needed
  • Updated relevant documentation (docs/) and code comments, or no
    documentation updates needed

M. J. Fromberger and others added 4 commits August 30, 2022 17:38
Rework the implementation of event query parsing and execution to
improve performance and reduce memory usage.

Previous memory and CPU profiles of the pubsub service showed query
processing as a significant hotspot. While we don't have evidence that
this is visibly hurting users, fixing it is fairly easy and self-contained.

Updates tendermint#6439.

Typical benchmark results comparing the original implementation (PEG) with the reworked implementation (Custom):
```
TEST                        TIME/OP  BYTES/OP  ALLOCS/OP  SPEEDUP   MEM SAVING
BenchmarkParsePEG-12       51716 ns  526832    27
BenchmarkParseCustom-12     2167 ns    4616    17         23.8x     99.1%
BenchmarkMatchPEG-12        3086 ns    1097    22
BenchmarkMatchCustom-12    294.2 ns      64     3         10.5x     94.1%
```

Components:
* Add a basic parsing benchmark.
* Move the original query implementation to a subdirectory.
* Add lexical scanner for Query expressions.
* Add a parser for Query expressions.
* Implement query compiler.
* Add test cases based on OpenAPI examples.
* Add MustCompile to replace the original MustParse, and update usage.
@mmsqe mmsqe marked this pull request as ready for review August 31, 2022 02:17
@mmsqe mmsqe requested a review from ebuchman as a code owner August 31, 2022 02:17
@mmsqe mmsqe requested a review from a team August 31, 2022 02:17
Copy link
Contributor

@cmwaters cmwaters left a comment

Choose a reason for hiding this comment

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

This looks good to me. Just needs a changelog (I realise it shouldn't change user behaviour but I think it's appropriate to still advertise the change)

@sergio-mena
Copy link
Contributor

Please do not merge it to main for the moment.
We need to decide when this should be merged to main. We also need to review the process followed to backport Michael PR's, to make sure there are no loose ends.
I'm more than willing to discuss details over Slack.

@mmsqe
Copy link
Author

mmsqe commented Sep 8, 2022

I just follow the steps in #7801 and test rest adr075 backports in ethermint with more websocket e2e tests in cronos, free to ping me when need change.

@cmwaters
Copy link
Contributor

cmwaters commented Sep 8, 2022

I just want to clarify that this PR isn't actually a reimplementation of adr075. It's just the performance improvements that Michael did in #7319 which AFAIK wasn't part of the ADR which actually involved adding a new API.

@mmsqe
Copy link
Author

mmsqe commented Sep 8, 2022

I just want to clarify that this PR isn't actually a reimplementation of adr075. It's just the performance improvements that Michael did in #7319 which AFAIK wasn't part of the ADR which actually involved adding a new API.

yes, since this is prerequisites of the rest backports.

@cmwaters
Copy link
Contributor

cmwaters commented Sep 8, 2022

yes, since this is prerequisites of the rest backports.

Do you plan to do the rest of the commits in this PR or as a separate PR? (I think I would prefer them separated)

@mmsqe
Copy link
Author

mmsqe commented Sep 8, 2022

Do you plan to do the rest of the commits in this PR or as a separate PR? (I think I would prefer them separated)

yes, to keep as minimal as possible.

@sergio-mena
Copy link
Contributor

Let me clarify my comment. This effort (i.e. porting ADR075) looks like a good candidate for a feature branch. Hence I was saying to hold on this PR. If this PR is the first of a series of backports that are needed to get ADR075 onmain, I would prefer to:

  • First create a feature branch
  • Rebase this PR against it, and merge as its first commit
  • Reach agreement (probably synchronously) how we are going to review, and/or follow up, the backport work taking place in that branch.

I don't think it's a good idea to backport the whole set of PRs for ADR075 directly on main, it's a long shot IMO.

@mmsqe
Copy link
Author

mmsqe commented Sep 8, 2022

I don't think it's a good idea to backport the whole set of PRs for ADR075 directly on main, it's a long shot IMO.

That would work, but which feature branch I should based on?

@thanethomson thanethomson changed the title backport-adr075: performance improvements for the event query API (#7319) backport: performance improvements for the event query API (#7319) Sep 8, 2022
@sergio-mena
Copy link
Contributor

That would work, but which feature branch I should based on?

I'll come back to you shortly on that

@JayT106
Copy link
Contributor

JayT106 commented Sep 8, 2022

I just want to clarify that this PR isn't actually a reimplementation of adr075. It's just the performance improvements that Michael did in #7319 which AFAIK wasn't part of the ADR which actually involved adding a new API.

@sergio-mena
That's correct, I chatted this to @thanethomson last weekend. It might be good to merge it into v37 and v34 if possible.

@sergio-mena
Copy link
Contributor

After some internal discussion, we've decided that it's worth merging this PR directly on main as this PR makes sense by itself, and thus is not really ADR075-related work, for which we will create a feature branch.

Copy link
Contributor

@cmwaters cmwaters left a comment

Choose a reason for hiding this comment

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

added changelog entry to v0.38

@cmwaters cmwaters merged commit e80dd00 into tendermint:main Sep 13, 2022
@cmwaters
Copy link
Contributor

Thanks for contributing @mmsqe!

@mmsqe
Copy link
Author

mmsqe commented Sep 13, 2022

Thanks for contributing @mmsqe!

I take a free ride, thanks @creachadair

@mmsqe
Copy link
Author

mmsqe commented Sep 15, 2022

I'll come back to you shortly on that

@sergio-mena May I ask any update on feature branch of backport adr075?

@sergio-mena
Copy link
Contributor

@sergio-mena May I ask any update on feature branch of backport adr075?

Sorry, @mmsqe, for my late reply (had to deal with personal issues).

We have created branch feature/adr075-backport. This branch (which forks from main) is to act as base branch for all PRs related to the backport of adr075-related commits.

We will be reviewing all those PRs and merging them into feature/adr075-backport. Please backport PRs on a one-to-one basis, for better traceability. Also, please make use of git cherry-pick whenever possible (thus leveraging git's 3-way merge to reduce conflicts).

Once the backport is complete, and all CI tests pass, we will proceed to merge feature/adr075-backport back to main. Note this is the process we are following to backport other features such as ABCI++.

If you have any questions/comments/concerns don't hesitate to contact me over Slack, or email (sergio@informal.systems).

mmsqe added a commit to mmsqe/tendermint that referenced this pull request Dec 9, 2022
…t#7319) (tendermint#9334)

* Performance improvements for the event query API (tendermint#7319)

Rework the implementation of event query parsing and execution to
improve performance and reduce memory usage.

Previous memory and CPU profiles of the pubsub service showed query
processing as a significant hotspot. While we don't have evidence that
this is visibly hurting users, fixing it is fairly easy and self-contained.

Updates tendermint#6439.

Typical benchmark results comparing the original implementation (PEG) with the reworked implementation (Custom):
```
TEST                        TIME/OP  BYTES/OP  ALLOCS/OP  SPEEDUP   MEM SAVING
BenchmarkParsePEG-12       51716 ns  526832    27
BenchmarkParseCustom-12     2167 ns    4616    17         23.8x     99.1%
BenchmarkMatchPEG-12        3086 ns    1097    22
BenchmarkMatchCustom-12    294.2 ns      64     3         10.5x     94.1%
```
mmsqe added a commit to mmsqe/tendermint that referenced this pull request Dec 9, 2022
…t#7319) (tendermint#9334)

* Performance improvements for the event query API (tendermint#7319)

Rework the implementation of event query parsing and execution to
improve performance and reduce memory usage.

Previous memory and CPU profiles of the pubsub service showed query
processing as a significant hotspot. While we don't have evidence that
this is visibly hurting users, fixing it is fairly easy and self-contained.

Updates tendermint#6439.

Typical benchmark results comparing the original implementation (PEG) with the reworked implementation (Custom):
```
TEST                        TIME/OP  BYTES/OP  ALLOCS/OP  SPEEDUP   MEM SAVING
BenchmarkParsePEG-12       51716 ns  526832    27
BenchmarkParseCustom-12     2167 ns    4616    17         23.8x     99.1%
BenchmarkMatchPEG-12        3086 ns    1097    22
BenchmarkMatchCustom-12    294.2 ns      64     3         10.5x     94.1%
```
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.

5 participants