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

Added more build profiles #290

Merged
merged 2 commits into from
Apr 16, 2020
Merged

Added more build profiles #290

merged 2 commits into from
Apr 16, 2020

Conversation

Razican
Copy link
Member

@Razican Razican commented Mar 31, 2020

This will add build profiles for benchmarks and tests. It will also add a thin optimization layer to development builds, and make sure that benchmarks are run with the same build profiles as the release builds.

This will add build profiles for benchmarks and tests. It will also
add a thin optimization layer to development builds, and make sure
that benchmarks are run with the same build profiles as the release
builds.
Cargo.toml Outdated Show resolved Hide resolved
@Razican
Copy link
Member Author

Razican commented Mar 31, 2020

I modified the configuration to only show the changed options from the defaults, and to add some comments on why the defaults were changed.

overflow-checks = true
panic = 'unwind'
# Enables thin local LTO and some optimizations.
opt-level = 1
Copy link
Member

Choose a reason for hiding this comment

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

I’m not sure for dev we want any optimisations, also setting 1 turns off the debug asserts which I think we’re using to get better feedback if something is wrong on dev

Copy link
Member Author

Choose a reason for hiding this comment

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

I was trying to find where it said that this would disable debug assertions, but I couldn't find it.

@Razican
Copy link
Member Author

Razican commented Apr 9, 2020

Let me know if this is good to go, @jasonwilliams

@jasonwilliams
Copy link
Member

I will need to test this with the debugger, having an opt-level of 1 might remove some of the information for the LLDB debugger to show properly

@jasonwilliams jasonwilliams added this to the v0.8.0 milestone Apr 12, 2020
@jasonwilliams
Copy link
Member

Testing this today with the debugger, i can still see all the values i had before, so a level of 1 seems ok, but we still have the debug_assert issue https://doc.rust-lang.org/cargo/reference/profiles.html

@Razican Razican added the performance Performance related changes and issues label Apr 13, 2020
@Razican
Copy link
Member Author

Razican commented Apr 13, 2020

Testing this today with the debugger, i can still see all the values i had before, so a level of 1 seems ok, but we still have the debug_assert issue doc.rust-lang.org/cargo/reference/profiles.html

If I understand the reference properly, the debug-assertions is turned on automatically if the optimization level is 0, but if not, it will check for that configuration option, so it should work as expected.

I just checked by adding a debug_assert!(false) in the main function and running this with this configuration, and it panics as expected. If I run in --release mode, it doesn't panic.

This small optimization level is very fast at compile time, but gives a ton of performance improvements that will be interesting when running a lot of tests, or running example workloads that take long to execute.

@Razican
Copy link
Member Author

Razican commented Apr 16, 2020

Let me know, @jasonwilliams if this is good to go, or if we don't want the optimizations on debug/test builds :)

Copy link
Member

@jasonwilliams jasonwilliams left a comment

Choose a reason for hiding this comment

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

I think this is good to go!

@Razican Razican merged commit 8574ea1 into boa-dev:master Apr 16, 2020
@Razican Razican deleted the new_profiles branch April 16, 2020 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Performance related changes and issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants