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

Remove duplicate extract_package_and_build_text call #5253

Merged
merged 1 commit into from
Apr 12, 2024

Conversation

kenodegard
Copy link
Contributor

Description

Looking to see just how much of a speedup we get by removing this redundant line.

Extracted from:

Checklist - did you ...

  • Add a file to the news directory (using the template) for the next release's release notes?
  • Add / update necessary tests?
  • Add / update outdated documentation?

@conda-bot conda-bot added the cla-signed [bot] added once the contributor has signed the CLA label Mar 25, 2024
Copy link

codspeed-hq bot commented Mar 25, 2024

CodSpeed Performance Report

Merging #5253 will improve performances by 26.85%

Comparing kenodegard:remove-duplicate (8c64506) with main (6982cbd)

Summary

⚡ 1 improvements
✅ 2 untouched benchmarks

Benchmarks breakdown

Benchmark main kenodegard:remove-duplicate Change
test_pin_subpackage_benchmark 118.7 s 93.5 s +26.85%

@kenodegard kenodegard changed the title Remove redundant duplicate line Remove duplicate extract_package_and_build_text call Mar 25, 2024
@kenodegard
Copy link
Contributor Author

FYI @mbargull

@mbargull
Copy link
Member

Thanks, I wondered too and wanted to do the same, just didn't get to it yet :).
I've pushed the other commit I was interested in (somewhat) isolation.

@mbargull
Copy link
Member

mbargull commented Mar 25, 2024

Benchmark main kenodegard:remove-duplicate Change modified functions
test_pin_subpackage_benchmark 122.9 s 97 s +26.64% build_id
test_pin_subpackage_benchmark 122.9 s 90.9 s +35.22% build_id
get_output_dicts_from_metadata
test_pin_subpackage_benchmark 122.9 s 83.8 s +46.52% build_id
get_output_dicts_from_metadata
_filter_recipe_text
test_pin_subpackage_benchmark 122.8 s 73.5 s +67.07% build_id
get_output_dicts_from_metadata
_filter_recipe_text
_split_line_selector
test_pin_subpackage_benchmark 122.8 s 71.4 s +72.06% build_id
get_output_dicts_from_metadata
_filter_recipe_text
_split_line_selector
select_lines/selector_cache
test_pin_subpackage_benchmark 122.8 s 73.7 s +66.55% build_id
get_output_dicts_from_metadata
_filter_recipe_text
select_lines/namespace hash
test_pin_subpackage_benchmark 122.8 s 71.4 s +72.07% build_id
get_output_dicts_from_metadata
_filter_recipe_text
_split_line_selector
select_lines/selector_cache
regex shortcut
test_pin_subpackage_benchmark 122.8 s 71.3 s +72.16% build_id
get_output_dicts_from_metadata
_filter_recipe_text
_split_line_selector
select_lines/selector_cache
regex shortcut
get_used_loop_vars+get_vars
test_pin_subpackage_benchmark 122.8 s 40.4 s ×3 build_id
get_output_dicts_from_metadata
_filter_recipe_text
_split_line_selector
select_lines/selector_cache
regex shortcut
get_used_loop_vars+get_vars
Config.copy

@mbargull mbargull force-pushed the remove-duplicate branch 2 times, most recently from 9118389 to 0812819 Compare March 25, 2024 20:26
@mbargull
Copy link
Member

test_pin_subpackage_benchmark 122.9 s 97 s +26.64% build_id
test_pin_subpackage_benchmark 122.9 s 90.9 s +35.22% build_id
get_output_dicts_from_metadata
test_pin_subpackage_benchmark 122.9 s 83.8 s +46.52% build_id
get_output_dicts_from_metadata
_filter_recipe_text

It's nice to see that these innocuous changes would yield considerable improvements already.

test_pin_subpackage_benchmark 122.8 s 73.5 s +67.07% build_id
get_output_dicts_from_metadata
_filter_recipe_text
_split_line_selector
test_pin_subpackage_benchmark 122.8 s 71.4 s +72.06% build_id
get_output_dicts_from_metadata
_filter_recipe_text
_split_line_selector
select_lines/selector_cache
test_pin_subpackage_benchmark 122.8 s 73.7 s +66.55% build_id
get_output_dicts_from_metadata
_filter_recipe_text
select_lines/namespace hash

As hoped for in #5237 (comment) , your approach offers at least a much improvements as my more complicated bolted-on version!
It's also nice to see what part contributes the most improvement in the benchmark, i.e., as expected, factoring out _split_line_selector has the most impact.

test_pin_subpackage_benchmark 122.8 s 71.4 s +72.07% build_id
get_output_dicts_from_metadata
_filter_recipe_text
_split_line_selector
select_lines/selector_cache
regex shortcut

This was just to confirm that any impact (if at all) of these micro "only-re.match/re.search-if-needed" optimizations would not be reflected in the current benchmarks.
The test cases carry no script files and the meta.yaml (or lines therein) might be too small for sel_pat.match(line) to matter; plus, due to the memoization the latter might not make much of difference anymore anyway (which would be the desired outcome, of course!).

@kenodegard
Copy link
Contributor Author

@mbargull thanks for doing this! should we split each of the commits into separate PRs now?

@beckermr
Copy link
Contributor

What can I do to get these changes moving towards being included in the next conda-build release?

@kenodegard
Copy link
Contributor Author

All of these improvements have been split off into separate PRs for easier review. Reverting this PR to the first commit.

@kenodegard kenodegard marked this pull request as ready for review April 12, 2024 20:27
@kenodegard kenodegard requested a review from a team as a code owner April 12, 2024 20:27
@kenodegard kenodegard enabled auto-merge (squash) April 12, 2024 20:33
@kenodegard kenodegard merged commit b7ea405 into conda:main Apr 12, 2024
54 checks passed
@kenodegard kenodegard deleted the remove-duplicate branch April 12, 2024 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed [bot] added once the contributor has signed the CLA
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants