Skip to content

perf: avoid method chaining for some shape_generators#3615

Merged
ianna merged 2 commits intoscikit-hep:mainfrom
pfackeldey:avoid_shape_generator_chaining
Aug 26, 2025
Merged

perf: avoid method chaining for some shape_generators#3615
ianna merged 2 commits intoscikit-hep:mainfrom
pfackeldey:avoid_shape_generator_chaining

Conversation

@pfackeldey
Copy link
Copy Markdown
Collaborator

@pfackeldey pfackeldey commented Aug 11, 2025

this PR improves performance by avoiding creation of unnecessary shape_generators where the actual shape is already known. This can become noticeable in cases where trivial getitems (or similar) are chained many times in very large layouts, i.e. coffea's NanoEvents data structure.

@pfackeldey pfackeldey mentioned this pull request Aug 9, 2025
Closed
9 tasks
@codecov
Copy link
Copy Markdown

codecov bot commented Aug 11, 2025 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.49%. Comparing base (b749e49) to head (5268a3e).
⚠️ Report is 401 commits behind head on main.

Additional details and impacted files
Files with missing lines Coverage Δ
src/awkward/_nplikes/array_module.py 95.01% <ø> (+8.63%) ⬆️
src/awkward/_nplikes/virtual.py 91.23% <ø> (ø)

... and 194 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ikrommyd
Copy link
Copy Markdown
Collaborator

ikrommyd commented Aug 11, 2025

Hi @pfackeldey why not use the assert never generator in these cases? On phone so is there something I’m missing?

@pfackeldey
Copy link
Copy Markdown
Collaborator Author

pfackeldey commented Aug 11, 2025

Hi @pfackeldey why not use the assert never generator in these cases? On phone so is there something I’m missing?

assert_never is set implicitly whenever the shape was unknown and then inferred from calling the generator once. If the shape is known already there's no point in setting it to assert_never as we won't hit this anyway. These are the cases of this PR where we have known shapes, so using None (16B) is a bit cheaper than assert_never (160B).
My point is: we shouldn't worry about code paths that are never hit anyway...

@ianna ianna added the pr-next-release Required for the next release label Aug 14, 2025
@pfackeldey pfackeldey requested a review from ianna August 14, 2025 19:48
ianna
ianna approved these changes Aug 26, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@ianna ianna left a comment

Choose a reason for hiding this comment

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

@pfackeldey - Looks great! Thanks for speeding it up!

@ianna ianna enabled auto-merge (squash) August 26, 2025 12:49
@ianna ianna merged commit 46c609b into scikit-hep:main Aug 26, 2025
47 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ianna ianna ianna approved these changes

Assignees

No one assigned

Labels

pr-next-release Required for the next release

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@pfackeldey @ikrommyd @ianna