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

Skip memmove in del_at_beg when not needed. #24503

Merged
merged 1 commit into from
Nov 9, 2017
Merged

Skip memmove in del_at_beg when not needed. #24503

merged 1 commit into from
Nov 9, 2017

Conversation

yuyichao
Copy link
Contributor

@yuyichao yuyichao commented Nov 6, 2017

This was likely previously done on linux in the glibc implementation.

Fix #24494

This was likely previously done on linux in the glibc implementation.

Fix #24494
@dhoegh
Copy link
Contributor

dhoegh commented Nov 7, 2017

Is it possible to add some kind of test, to protect against a possible regression in the future, as the regression is massive and apparently snug under the radar in 0.5 and 0.6?

@yuyichao
Copy link
Contributor Author

yuyichao commented Nov 7, 2017

No there's no behavioral difference. You can add something to benchmark though.

@yuyichao
Copy link
Contributor Author

yuyichao commented Nov 7, 2017

(Though nonosoldier won't be able to catch this either......)

@dhoegh
Copy link
Contributor

dhoegh commented Nov 7, 2017

Too bad:( Could the test case in the issue be added as a separate test set, and hence if it regresses the test set run time goes from 0.5s to 300s?

@yuyichao
Copy link
Contributor Author

yuyichao commented Nov 7, 2017

Historically that kind of test always cause more trouble and never catch any regression. There's nothing special about this pattern anyway.

@StefanKarpinski
Copy link
Member

Testing for this kind of thing is exactly why we have BaseBenchmarks – and we do regularly run those tests to make sure things haven't regressed. Please do open a PR to add this test there.

@ararslan ararslan added the potential benchmark Could make a good benchmark in BaseBenchmarks label Nov 7, 2017
@JeffBezanson JeffBezanson merged commit d7a171e into master Nov 9, 2017
@JeffBezanson JeffBezanson deleted the yyc/array branch November 9, 2017 05:42
@dhoegh dhoegh mentioned this pull request Nov 9, 2017
@ararslan
Copy link
Member

ararslan commented Nov 9, 2017

This doesn't backport cleanly to release-0.6 (or at least to my backports branch). Would you be willing to submit a PR to release-0.6 with the correct change, or just push a commit to aa/backports-0.6.2?

yuyichao added a commit that referenced this pull request Nov 14, 2017
This was likely previously done on linux in the glibc implementation.

Fix #24494

(cherry picked from commit d7a171e)
@yuyichao
Copy link
Contributor Author

It's exactly the same code on 0.6, just with different context. Anyway, it's here https://github.com/JuliaLang/julia/compare/aa/backports-0.6.2...yyc/test/release-0.6?expand=1

@ararslan
Copy link
Member

Perfect, thanks so much!

ararslan pushed a commit that referenced this pull request Nov 14, 2017
This was likely previously done on linux in the glibc implementation.

Fix #24494

(cherry picked from commit d7a171e)
ararslan pushed a commit that referenced this pull request Nov 14, 2017
This was likely previously done on linux in the glibc implementation.

Fix #24494

(cherry picked from commit d7a171e)
ararslan pushed a commit that referenced this pull request Nov 21, 2017
This was likely previously done on linux in the glibc implementation.

Fix #24494

(cherry picked from commit d7a171e)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Must go faster potential benchmark Could make a good benchmark in BaseBenchmarks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants