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

[Merged by Bors] - Add cart's fork of ecs_bench_suite #4225

Closed
wants to merge 2 commits into from

Conversation

james7132
Copy link
Member

@james7132 james7132 commented Mar 16, 2022

Objective

Better benchmarking for ECS. Fix #2062.

Solution

Port @cart's fork of ecs_bench_suite to the official bench suite for bevy_ecs, replace cgmath with glam, update to latest bevy.

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Mar 16, 2022
@james7132 james7132 added A-ECS Entities, components, systems, and events C-Performance A change motivated by improving speed, memory usage or compile times and removed S-Needs-Triage This issue needs to be labelled labels Mar 16, 2022
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Looks good to me. We can revise these further in the future IMO.

@IceSentry
Copy link
Contributor

IceSentry commented Mar 16, 2022

I'm not sure if I'm doing something wrong, but if I clone this PR, go to the /benches folder and run cargo bench I get this error.

error[E0658]: trait bounds other than `Sized` on const fn parameters are unstable
   --> C:\Users\<user>\.cargo\registry\src\git.luolix.top-1ecc6299db9ec823\crossbeam-epoch-0.9.8\src\atomic.rs:314:6
    |
314 | impl<T: ?Sized + Pointable> Atomic<T> {
    |      ^
...
346 |     pub const fn null() -> Atomic<T> {
    |     -------------------------------- function declared as const here
    |
    = note: see issue #93706 <https://github.com/rust-lang/rust/issues/93706> for more information
    = help: add `#![feature(const_fn_trait_bound)]` to the crate attributes to enable

   Compiling cast v0.2.7
For more information about this error, try `rustc --explain E0658`.

I tried doing a cargo clean and I get the same error

@hymm
Copy link
Contributor

hymm commented Mar 18, 2022

I was able to run the tests just fine. Windows 10, rust 1.59.

@james7132 james7132 added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Mar 19, 2022
Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

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

Awesome! I agree that we might ultimately want to tweak these to meet our own needs (instead of assuming ecs_bench_suite patterns are the right call). But having these tests in-tree makes optimizing bevy_ecs much easier. This is definitely a win.

@cart
Copy link
Member

cart commented Mar 19, 2022

bors r+

bors bot pushed a commit that referenced this pull request Mar 19, 2022
# Objective
Better benchmarking for ECS. Fix #2062.

## Solution
Port @cart's fork of ecs_bench_suite to the official bench suite for bevy_ecs, replace cgmath with glam, update to latest bevy.
@bors bors bot changed the title Add cart's fork of ecs_bench_suite [Merged by Bors] - Add cart's fork of ecs_bench_suite Mar 19, 2022
@bors bors bot closed this Mar 19, 2022
@DJMcNab
Copy link
Member

DJMcNab commented Mar 19, 2022

Sorry to do this, but I think we need to revert this. We don't have a license to include the code from ecs_bench_suite, which is all rights reserved.

We can seperately get in touch with @TomGillen to try and get such a license, but otherwise we can't use this.

DJMcNab added a commit to DJMcNab/bevy that referenced this pull request Mar 19, 2022
bors bot pushed a commit that referenced this pull request Mar 19, 2022
This reverts commit 08ef2f0.

# Objective

- #4225 was merged without considering the licensing considerations.
- It merges in code taken from https://github.com/cart/ecs_bench_suite/tree/bevy-benches/src/bevy.
- We can safely assume that we do have a license to cart's contributions. However, these build upon cart/ecs_bench_suite@377e96e, for which we have no license. 
- This has been verified by looking in the Cargo.toml, the root folder and the readme, none of which mention a license. Additionally, the string "license" [doesn't appear](https://github.com/rust-gamedev/ecs_bench_suite/search?q=license) in the repository.
- This means the code is all rights reserved.
- (The author of these commits also hasn't commented in #2373, though even if they had, it would be legally *dubious* to rely on that to license any code they ever wrote)
- (Note that the latest commit on the head at https://github.com/rust-gamedev/ecs_bench_suite hasn't had a license added either.)
- We are currently incorrectly claiming to be able to give an MIT/Apache 2.0 license to this code.

## Solution

- Revert it
@alice-i-cecile alice-i-cecile mentioned this pull request Mar 21, 2022
@james7132 james7132 deleted the ecs-bench-suite branch March 21, 2022 01:37
@cart
Copy link
Member

cart commented Mar 23, 2022

This was the right call, as we hadn't previously discussed this. But I think generally these fall under the "trivially reproducible / not novel" category of thing. From scratch impls would look pretty much the same. Couple that with me being the one that wrote most of the code here, and I think adding these isn't risky.

That being said, its better to ask and be safe.

@TomGillen, can we have permission to relicense the bevy ecs_bench_suite tests under bevy's dual MIT/Apache-2.0 license?

@TomGillen
Copy link

I don't have any problem with that. It should have had such a license in the first place, but it didn't occur to me that someone would want to embed the code into another project or use it as a library.

alice-i-cecile pushed a commit to alice-i-cecile/bevy that referenced this pull request Mar 25, 2022
bors bot pushed a commit that referenced this pull request Mar 29, 2022
# Objective

- Benchmarks are good.
- Licensing situation appears to be [cleared up](#4225 (comment)).

## Solution

- Add the benchmark suite back in
- Suggested PR title: "Revert "Revert "Add cart's fork of ecs_bench_suite (#4225)" (#4252)"


Co-authored-by: Daniel McNab <36049421+DJMcNab@users.noreply.github.com>
@cart
Copy link
Member

cart commented Mar 29, 2022

Thanks @TomGillen!

aevyrie pushed a commit to aevyrie/bevy that referenced this pull request Jun 7, 2022
# Objective
Better benchmarking for ECS. Fix bevyengine#2062.

## Solution
Port @cart's fork of ecs_bench_suite to the official bench suite for bevy_ecs, replace cgmath with glam, update to latest bevy.
aevyrie pushed a commit to aevyrie/bevy that referenced this pull request Jun 7, 2022
…gine#4252)

This reverts commit 08ef2f0.

# Objective

- bevyengine#4225 was merged without considering the licensing considerations.
- It merges in code taken from https://github.com/cart/ecs_bench_suite/tree/bevy-benches/src/bevy.
- We can safely assume that we do have a license to cart's contributions. However, these build upon cart/ecs_bench_suite@377e96e, for which we have no license. 
- This has been verified by looking in the Cargo.toml, the root folder and the readme, none of which mention a license. Additionally, the string "license" [doesn't appear](https://github.com/rust-gamedev/ecs_bench_suite/search?q=license) in the repository.
- This means the code is all rights reserved.
- (The author of these commits also hasn't commented in bevyengine#2373, though even if they had, it would be legally *dubious* to rely on that to license any code they ever wrote)
- (Note that the latest commit on the head at https://github.com/rust-gamedev/ecs_bench_suite hasn't had a license added either.)
- We are currently incorrectly claiming to be able to give an MIT/Apache 2.0 license to this code.

## Solution

- Revert it
aevyrie pushed a commit to aevyrie/bevy that referenced this pull request Jun 7, 2022
# Objective

- Benchmarks are good.
- Licensing situation appears to be [cleared up](bevyengine#4225 (comment)).

## Solution

- Add the benchmark suite back in
- Suggested PR title: "Revert "Revert "Add cart's fork of ecs_bench_suite (bevyengine#4225)" (bevyengine#4252)"


Co-authored-by: Daniel McNab <36049421+DJMcNab@users.noreply.github.com>
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective
Better benchmarking for ECS. Fix bevyengine#2062.

## Solution
Port @cart's fork of ecs_bench_suite to the official bench suite for bevy_ecs, replace cgmath with glam, update to latest bevy.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
…gine#4252)

This reverts commit 08ef2f0.

# Objective

- bevyengine#4225 was merged without considering the licensing considerations.
- It merges in code taken from https://github.com/cart/ecs_bench_suite/tree/bevy-benches/src/bevy.
- We can safely assume that we do have a license to cart's contributions. However, these build upon cart/ecs_bench_suite@377e96e, for which we have no license. 
- This has been verified by looking in the Cargo.toml, the root folder and the readme, none of which mention a license. Additionally, the string "license" [doesn't appear](https://github.com/rust-gamedev/ecs_bench_suite/search?q=license) in the repository.
- This means the code is all rights reserved.
- (The author of these commits also hasn't commented in bevyengine#2373, though even if they had, it would be legally *dubious* to rely on that to license any code they ever wrote)
- (Note that the latest commit on the head at https://github.com/rust-gamedev/ecs_bench_suite hasn't had a license added either.)
- We are currently incorrectly claiming to be able to give an MIT/Apache 2.0 license to this code.

## Solution

- Revert it
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

- Benchmarks are good.
- Licensing situation appears to be [cleared up](bevyengine#4225 (comment)).

## Solution

- Add the benchmark suite back in
- Suggested PR title: "Revert "Revert "Add cart's fork of ecs_bench_suite (bevyengine#4225)" (bevyengine#4252)"


Co-authored-by: Daniel McNab <36049421+DJMcNab@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Performance A change motivated by improving speed, memory usage or compile times S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move Bevy's fork of the ecs_bench_suite to the appropriate folder
7 participants