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.

Sign up for GitHub

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

Jump to bottom

benchmark for SubscribeAsync #774

Merged
merged 13 commits into from
Jul 27, 2024
Merged

benchmark for SubscribeAsync #774

merged 13 commits into from
Jul 27, 2024

Conversation

levb
Copy link
Collaborator

@levb levb commented Jul 22, 2024

No description provided.

@levb levb requested a review from kozlovic July 22, 2024 22:08
@codecov Codecov
Copy link

codecov bot commented Jul 22, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 93.33333% with 1 line in your changes missing coverage. Please review.

Project coverage is 68.86%. Comparing base (1553d4a) to head (bccc82c).
Report is 3 commits behind head on main.

Files Patch % Lines
src/util.c 66.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #774      +/-   ##
==========================================
+ Coverage   68.71%   68.86%   +0.14%     
==========================================
  Files          39       39              
  Lines       15207    15238      +31     
  Branches     3143     3153      +10     
==========================================
+ Hits        10449    10493      +44     
+ Misses       1700     1678      -22     
- Partials     3058     3067       +9

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

kozlovic
kozlovic requested changes Jul 23, 2024
View reviewed changes
bench/CMakeLists.txt Outdated Show resolved Hide resolved
bench/sub-async.c Outdated Show resolved Hide resolved
bench/sub-async.c Outdated Show resolved Hide resolved
bench/sub-async.c Outdated Show resolved Hide resolved
levb
levb commented Jul 24, 2024
View reviewed changes
bench/bench.h Outdated Show resolved Hide resolved
kozlovic
kozlovic requested changes Jul 24, 2024
View reviewed changes
bench/sub-async.c Outdated Show resolved Hide resolved
kozlovic
kozlovic approved these changes Jul 25, 2024
View reviewed changes
Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

LGTM.

@levb
Copy link
Collaborator Author

levb commented Jul 25, 2024 •
edited
Loading

@kozlovic unfortunately, It ended up very noisy, and I am still debugging. More changes coming. DONE

@levb
Copy link
Collaborator Author

levb commented Jul 26, 2024 •
edited
Loading

@kozlovic apologies I pushed here instead of my fork by mistake; still need some touches. WIP, please stand by.

@levb
moved bench to test
08b563c 
refactored bench
Added BenchSubscribeAsync_Inject
Added BenchSubscribeAsync_InjectSlow
@levb levb requested a review from kozlovic July 26, 2024 19:13
levb
levb commented Jul 26, 2024
View reviewed changes
.github/workflows/on-pr-debug.yml Outdated
@@ -98,4 +106,4 @@ jobs:
- name: Test
run: |
cd build
./bin/Debug/testsuite
ctest -L 'test' --timeout 60 --output-on-failure
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cc @mtmk ctest seems to work fine?

Copy link
Contributor

Choose a reason for hiding this comment

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

doesn't run any tests
https://github.com/nats-io/nats.c/actions/runs/10116438126/job/27979286329?pr=774#step:5:10

Test project D:/a/nats.c/nats.c/build
No tests were found!!!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@mtmk I think I cleaned it up well, borrowing from your PR. Not sure why many tests are flaky, for now retry=3. Please review when you have a chance.

levb
levb commented Jul 26, 2024
View reviewed changes
test/bench_sub_async.c Outdated Show resolved Hide resolved
@levb levb mentioned this pull request Jul 26, 2024
Closed
kozlovic
kozlovic requested changes Jul 26, 2024
View reviewed changes
Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

Also @mtmk mentioned that the ctest -L does not seem to be working.

test/bench_sub_async.c Show resolved Hide resolved
test/bench_sub_async.c Show resolved Hide resolved
@levb levb requested a review from mtmk July 27, 2024 19:31
levb
levb commented Jul 27, 2024
View reviewed changes
src/util.c Show resolved Hide resolved
@kozlovic
Copy link
Member

@levb I saw this test JetStreamInfoAlternates start to fail a lot in this PR. Seem unrelated and maybe due to server's main vs other releases?

@levb
Copy link
Collaborator Author

levb commented Jul 27, 2024 •
edited
Loading

@kozlovic yes, I think it was caused by nats-io/nats-server#5506, but haven't had a chance to understand/adjust the test yet. If you know how - would MUCH appreciate it! I raised it in the server channel back then, but didn't get much traction.

To be clear, it has been flaking for some time now, not this PR in particular.

@levb
Copy link
Collaborator Author

levb commented Jul 27, 2024

402 Payment Required - that's a nice wrinkle (Travis)

@levb levb requested a review from kozlovic July 27, 2024 19:55
kozlovic
kozlovic approved these changes Jul 27, 2024
View reviewed changes
Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

LGTM (looking at the flap test to see if I understand what's going on. If I do, will post PR if this is an issue with the test itself).

@kozlovic
Copy link
Member

@levb Are you going to merge this to main or is it to be merged only onto the other PR? I want to submit a PR for a fix to the test, but wanted to base it on that one so that there is less chance of a conflict?

@levb levb merged commit d8f52d9 into main Jul 27, 2024
27 of 29 checks passed
@levb levb deleted the lev-bench branch July 27, 2024 23:01
@mtmk mtmk mentioned this pull request Sep 6, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kozlovic kozlovic kozlovic approved these changes

@mtmk mtmk Awaiting requested review from mtmk

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@levb @kozlovic @mtmk