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

Fix performance regression in reductions benchmark #3874

Merged
merged 4 commits into from
Oct 29, 2024

Conversation

jeremiah-corrado
Copy link
Contributor

@jeremiah-corrado jeremiah-corrado commented Oct 25, 2024

Fix a performance regression caused by recent refactoring of ReductionMsg.

The refactor made total reductions (i.e., reducing an array to a single scalar value) return that scalar value via a 1-element array. This resulted in much cleaner code, but also added overhead for the total-reduction case. This PR separates total reductions into separate commands s.t. they can return a scalar directly.

Also fixes a bug in register_commands.py, where any unrecognized return type was being treated as a symbol-table entry.

@ajpotts
Copy link
Contributor

ajpotts commented Oct 28, 2024

It looks like the benchmark is failing b/c prod does not handle overflow. numpy seems to set the overflow result to 0.

I get this print statement from the failing case in ./benchmarks/reduce.py --correctness-only localhost 5555

op
prod

npa
[   1    2    3 ... 9997 9998 9999]

random
False

dtype
int64

SEED
None

npr
0

akr
-9223372036854775808

np.isclose(npr, akr)
False
Traceback (most recent call last):
  File "/home/amandapotts/git/arkouda/./benchmarks/reduce.py", line 176, in <module>
    check_correctness(dtype, args.randomize, args.seed)
  File "/home/amandapotts/git/arkouda/./benchmarks/reduce.py", line 123, in check_correctness
    assert np.isclose(npr, akr)
           ^^^^^^^^^^^^^^^^^^^^
AssertionError


``

@jeremiah-corrado
Copy link
Contributor Author

Thanks for pointing that out! I'll see if I can fix it.

Signed-off-by: Jeremiah Corrado <[email protected]>
@jeremiah-corrado
Copy link
Contributor Author

I'm seeing the following performance for the reduce benchmark (on the same system where the linked graph is generated):

numLocales = 16, N = 1,600,000,000
sum = 1279999999200000000
  sum Average time = 0.0070 sec
  sum Average rate = 1696.42 GiB/sec
prod = 0
  prod Average time = 0.0069 sec
  prod Average rate = 1722.76 GiB/sec
min = 1
  min Average time = 0.0068 sec
  min Average rate = 1748.09 GiB/sec
max = 1599999999
  max Average time = 0.0067 sec
  max Average rate = 1775.76 GiB/sec

This is around 10% lower than the results we've seen historically, which could be attributed to a difference in system configuration or something, but could also indicate that there are some other (more minor) performance fixes needed. I'd propose we merge this PR and see how the nightly performance testing graph responds before looking into this further.

@jeremiah-corrado jeremiah-corrado marked this pull request as ready for review October 28, 2024 17:26
Copy link
Member

@stress-tess stress-tess 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!

Copy link
Contributor

@ajpotts ajpotts left a comment

Choose a reason for hiding this comment

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

Thanks for doing this!

@ajpotts ajpotts added this pull request to the merge queue Oct 29, 2024
Merged via the queue into Bears-R-Us:master with commit 879192f Oct 29, 2024
11 checks passed
@jeremiah-corrado jeremiah-corrado deleted the reduction-perf-fix branch October 30, 2024 15:04
stonea added a commit to chapel-lang/chapel that referenced this pull request Oct 30, 2024
Adds an annotation for a recent Arkouda reduction performance
regression:

- initial regression caused by:
Bears-R-Us/arkouda#3845
- resolved by: Bears-R-Us/arkouda#3874

[Reviewed by nobody; annotations update]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants