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

Perormance regressions introduced by latest changes to main #886

Closed
diptorupd opened this issue Jan 23, 2023 · 7 comments
Closed

Perormance regressions introduced by latest changes to main #886

diptorupd opened this issue Jan 23, 2023 · 7 comments
Assignees
Labels
user User submitted issue

Comments

@diptorupd
Copy link
Collaborator

diptorupd commented Jan 23, 2023

I also see segfaults, makes me think that something is wrong with local memory.

Originally posted by @fcharras in #816 (comment)

The new problems are not related to #877 but to #876 , reported in #816

Originally posted by @fcharras in #867 (comment)

@fcharras
Copy link

@diptorupd @chudur-budur

We're having conversations about this in multiple separate issues, let's keep it in this dedicated thread ?

I also see segfaults, makes me think that something is wrong with local memory.

The problem most likely is because I without realizing the impact changed the way the global and local range args passed to the __getitem__. We are working on reverting my changes and fixing the issue.

Do you still need a minimal reproducer ? I haven't come up with one yet. Yet again it's not trivial to reproduce, simple kernels still work.

If you have a commit that revert those changes I can easily say if it fixes the issues though. (basically running the benchmark script in https://github.com/soda-inria/sklearn-numba-dpex )

@diptorupd
Copy link
Collaborator Author

@fcharras No we do not need th reproducer. Let @chudur-budur open the PR and then you can test and let us know if the issue is fixed.

@chudur-budur
Copy link
Collaborator

@fcharras Could you please test PR #888 and check if you are seeing any performance issues?

@fcharras
Copy link

fcharras commented Jan 29, 2023

Unfortunately, issues persists in #888 :-/

I just realized that I made a mistake in my bug report that might have misled you. The problems start at the merge commit of #804 not #876 . The merge commits of those two PRs are very close to each other and I missclicked the copy/paste :-/

@chudur-budur
Copy link
Collaborator

chudur-budur commented Feb 2, 2023

@fcharras Could you please test #896 and see if you are getting any slowdown?

@fcharras
Copy link

fcharras commented Feb 3, 2023

@chudur-budur @diptorupd please see #898 that give a minimal reproducer for what caused the regression I was facing, which is more than a performance issue.

However, it appears fixing it will not be the end. After understanding the bug I could test the KMeans in a way that doesn't trigger it (changing some parameters), and I still see a minor slow down. I think the refacto also added some python overhead. But, that should be easier to debug with normal profiling ? maybe the cache keys that are computed are a bit too computationally heavy. There are probably several expensive calls that can be saved. (no need to re-hash the codegen every time for instance)

But~ there are two more important issues to solve first IMO.

@diptorupd
Copy link
Collaborator Author

Closing the issue as the caching related performance regressions have been addressed and the code generation issue in #898 is solved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
user User submitted issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants