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

Benchmark Runner for C Benchmarks #777

Merged
merged 54 commits into from
Feb 5, 2022
Merged

Benchmark Runner for C Benchmarks #777

merged 54 commits into from
Feb 5, 2022

Conversation

mattchorlian
Copy link
Collaborator

@mattchorlian mattchorlian commented Dec 1, 2021

Opening this draft PR to track progress of converting all C benchmarks to use the benchmark runner:

  • Port current C++ benchmark runner to C
  • Big.lf
  • Chameneos.lf
  • Counting.lf
  • PingPong.lf
  • ThreadRing.lf
  • Throughput.lf
  • BoundedBuffer.lf
  • CigaretteSmoker.lf
  • Dictionary.lf
  • LogisticMap.lf
  • Philosophers.lf
  • SleepingBarber.lf
  • SortedLinkList.lf
  • Apsp.lf
  • FilterBank.lf
  • GuidedSearch.lf
  • MatMul.lf
  • NQueens.lf
  • PiPrecision.lf
  • RadixSort.lf
  • Trapezoidal.lf

@mattchorlian mattchorlian marked this pull request as draft December 1, 2021 06:03
LDeng0205 and others added 22 commits December 1, 2021 17:15
@mattchorlian
Copy link
Collaborator Author

I'm noticing that we have BoundedBuffer.lf and ProdCons.lf both here even though they are the same benchmark. I suppose this is (in part) the Savina naming convention causing confusion again. In any case, we should decide which one to keep around before merging this in!

Copy link
Member

@lhstrh lhstrh 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 great, @mattchorlian and @LDeng0205! I've only requested minor changes. Let's merge this into master ASAP.

@LDeng0205 LDeng0205 linked an issue Jan 26, 2022 that may be closed by this pull request
@housengw
Copy link
Contributor

Seems like running MatMul.lf with more than 1 thread produces a random validation error. I'm guessing it's a memory issue..

Copy link
Collaborator

@cmnrd cmnrd 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 great! Looks like we are close to completing the C benchmarks.

Should we also include the Banking Benchmark #624 here and fix it up?

Seems like running MatMul.lf with more than 1 thread produces a random validation error. I'm guessing it's a memory issue.

Could you describe what the issue seems to be? Is it a segfault that occurs sometimes? Or is the result invalid (since you mention a validation error)? The MatMul benchmark has a built-in race condition and hence the result is expected to be invalid for parallel execution. This is a bug in the original Savina benchmark...

.github/workflows/ci.yml Outdated Show resolved Hide resolved
@housengw
Copy link
Contributor

housengw commented Jan 28, 2022

the validation error in multithreaded run of MatMul.lf is likely a race condition.

@lhstrh
Copy link
Member

lhstrh commented Feb 3, 2022

the validation error in multithreaded run of MatMul.lf is likely a race condition.

Could you elaborate?

@housengw
Copy link
Contributor

housengw commented Feb 4, 2022

The MatMul benchmark has random validation errors when running with more than 1 thread. Since there is no segfault and the validation error happens at random indices in the matrix, it is most likely a race condition.
As @cmnrd mentioned, the MatMul benchmark has a built-in race condition, so running it with multiple threads would cause such an error.

@mattchorlian mattchorlian requested a review from lhstrh February 4, 2022 05:55
Copy link
Member

@lhstrh lhstrh left a comment

Choose a reason for hiding this comment

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

Looks ready to me! This is a big feat and will be very useful for the evaluation of the new schedulers in reactor-c 🚀🚀🚀 Thanks!

@mattchorlian mattchorlian requested a review from cmnrd February 4, 2022 06:27
@mattchorlian
Copy link
Collaborator Author

Should we also include the Banking Benchmark #624 here and fix it up?

We decided to get this merged first and deal with Banking separately @cmnrd

@lhstrh
Copy link
Member

lhstrh commented Feb 4, 2022

Should we also include the Banking Benchmark #624 here and fix it up?

We decided to get this merged first and deal with Banking separately @cmnrd

Come to thing of it, there is one distinct advantage to adding the other stuff to this PR: we'll have a cleaner history and release notes (if we automatically generate them). Doesn't seem like it would be much work?

Copy link
Collaborator

@cmnrd cmnrd left a comment

Choose a reason for hiding this comment

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

Looks great! I am happy with both options, merging as is or including banking as well.

@LDeng0205
Copy link
Collaborator

I just added benchmark runner to Banking as well so we could include it with this PR. However, I still have a java.lang.OutOfMemoryError when I compile it with NumAccounts = 1000.

@LDeng0205 LDeng0205 requested review from cmnrd and lhstrh February 4, 2022 19:29
@edwardalee
Copy link
Collaborator

Is this branch merged with master? It looks like it is still generating in-line code, which suggests it is using a version of master that predates scalability-banks.

@lhstrh lhstrh merged commit 30015c7 into master Feb 5, 2022
@lhstrh lhstrh deleted the c-benchmark-runner branch February 5, 2022 00:01
@lhstrh lhstrh mentioned this pull request Feb 5, 2022
@lhstrh lhstrh added the exclude Exclude from change log label Mar 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c Related to C target exclude Exclude from change log
Projects
None yet
6 participants