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

Some additional tests for competition #205

Closed
wants to merge 4 commits into from

Conversation

sedyh
Copy link

@sedyh sedyh commented Apr 5, 2023

I've added the tests for sedyh/mizu@experimental and some additional fixes.
From this discussion: #200.

@mlange-42 mlange-42 self-requested a review April 5, 2023 00:22
@mlange-42
Copy link
Owner

mlange-42 commented Apr 5, 2023

🤔 Any idea how I can approve the CI to run? Or why I can't? No such option anywhere...

EDIT: Ok, seems it is becaused actions are restricted to push. Will fix...

@mlange-42
Copy link
Owner

Ok, fixed. Could you rebase onto main please?

@sedyh
Copy link
Author

sedyh commented Apr 5, 2023

Ok, fixed. Could you rebase onto main please?

Ok, I'll do it tomorrow. It's 4 AM rn at my tz.

@sedyh
Copy link
Author

sedyh commented Apr 5, 2023

Ok. Looks like I've synced a fork and rebased my branch onto main.

@mlange-42
Copy link
Owner

mlange-42 commented Apr 6, 2023

GLFW dependencies are missing. They need to be installed before the benchmarks run. See here for an example: https://github.com/mlange-42/arche-pixel/blob/main/.github/workflows/tests.yml#L24

@sedyh
Copy link
Author

sedyh commented Apr 7, 2023

Is it about changing the build stage?
Added two commands under the run key.

@mlange-42
Copy link
Owner

No, it is required for the failing jobs in benchmarks.yml.

@sedyh
Copy link
Author

sedyh commented Apr 9, 2023

Ah, I get it. The problem is that I wrapped ebitengine at myself.
Thats because I wanted to make a simple api for the user, but the engine api doesn't really work with di and "accept interface, return struct" principle.

@mlange-42
Copy link
Owner

Yeah, I would try to avoid such dependencies where possible, esp. if it includes something as heavy as OpenGL.

@mlange-42
Copy link
Owner

Mh, another error around GLFW. But this time, it should not happen when just importing it. It looks like Ebiten actually does some GLFW stuff. I got this error only when actually trying to create a window, or something else that requires a "display".

So I guess this won't run in the CI in it's current form.

@mlange-42
Copy link
Owner

@sedyh Finally, I found a way to circumvent the problem of no display in the CI runners.

There is xvfb, which emulates a display. Here is an example how to use it:

  test:
    name: Run tests
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v3
      - name: Setup Go
        uses: actions/setup-go@v3
        with:
          go-version: '1.20.x'
      - name: Install dependencies
        run: |
          sudo apt-get update -y
          sudo apt-get install -y libgl1-mesa-dev xorg-dev xvfb
          go get .
      - name: Run tests
        run: |
          xvfb-run -a go test -v ./... -covermode=count -coverprofile="coverage.out"
          go tool cover -func="coverage.out"

@sedyh
Copy link
Author

sedyh commented Apr 13, 2023

Yeah... if we are going to merge that, then, probably, only in a separate branch, so as not to litter with additional dependencies?
Or it will just better to close the PR after getting the test results?

@mlange-42
Copy link
Owner

Yeah... if we are going to merge that, then, probably, only in a separate branch, so as not to litter with additional dependencies? Or it will just better to close the PR after getting the test results?

I am a bit undecided. Maybe I will merge it, as it adds the dependencies only to the benchmark module, which is separate from arche.

But maybe I will treat it like the Entitas benchmarks, and merge both branches together. Would then rebase when re-running due to improvements here or in the other implementations.

Will prepare and upload the plots in the next days.

velocity := &Velocity{}

for i := 0; i < nEntities; i++ {
w.AddEntity(&position)
Copy link
Owner

@mlange-42 mlange-42 Apr 14, 2023

Choose a reason for hiding this comment

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

@sedyh This should not be a pointer (position is already a pointer). The effect is that everything below does nothing. Try it by counting w.Each loops iterations.

Copy link
Author

@sedyh sedyh Apr 22, 2023

Choose a reason for hiding this comment

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

Ok, I'll fix it soon.

@mlange-42
Copy link
Owner

mlange-42 commented Apr 14, 2023

Another thing I notice while preparing the plots is that something in your PR (a dependency?) is slowing down some of the benchmarks, particularly in the Build category. Not sure what this can be.

Take iterations as a baseline, and compare build against it...

On main:

BenchmarkIterArche-2           	  284737	      4069 ns/op	       0 B/op	       0 allocs/op
BenchmarkBuildArche-2          	     945	   1354442 ns/op	 2538583 B/op	   10089 allocs/op
BenchmarkIterArcheGeneric-2    	  235425	      5002 ns/op	       0 B/op	       0 allocs/op
BenchmarkBuildArcheGeneric-2   	    2462	    506672 ns/op	 1093978 B/op	      70 allocs/op

On your branch:

BenchmarkIterArche-2           	  246014	      4768 ns/op	       0 B/op	       0 allocs/op
BenchmarkBuildArche-2          	     632	   1785171 ns/op	 2538583 B/op	   10089 allocs/op
BenchmarkIterArcheGeneric-2    	  241064	      5120 ns/op	       0 B/op	       0 allocs/op
BenchmarkBuildArcheGeneric-2   	    1890	    627462 ns/op	 1093978 B/op	      70 allocs/op

@sedyh
Copy link
Author

sedyh commented Apr 22, 2023

Hello, sorry for the long answer. The situation with the build slowdown quite strange, I'll try my best to solve it later.

@mlange-42 mlange-42 marked this pull request as draft April 30, 2023 22:37
@mlange-42 mlange-42 added the tests Tests, benchmarks and profiling label May 8, 2023
@sedyh
Copy link
Author

sedyh commented Oct 10, 2024

Well, it's been sooo long until the last message. I think I should close this PR for now until I figure out the problem so that it doesn’t just hang around.

@sedyh sedyh closed this Oct 10, 2024
@sedyh
Copy link
Author

sedyh commented Oct 10, 2024

Arche is nice btw ;)

@mlange-42
Copy link
Owner

mlange-42 commented Oct 10, 2024

Thanks! Actually, IMO it would be useful to keep this issue open.

EDIT: Oops, this is an MR, not an issue. So all fine with closing it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests Tests, benchmarks and profiling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants