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

PERF Only call malloc twice in liblinear to_sparse helpers #14170

Merged
merged 1 commit into from Jul 5, 2019

Conversation

@alexhenrie
Copy link
Contributor

alexhenrie commented Jun 23, 2019

By my benchmarks, this way takes 1% less time and 1% less memory. Plus, it makes error handling a lot easier.

@alexhenrie alexhenrie force-pushed the alexhenrie:prealloc branch from fdb1164 to ac68aef Jun 24, 2019
@alexhenrie alexhenrie changed the title PERF Only call malloc once in liblinear to_sparse helpers (#16148) PERF Only call malloc twice in liblinear to_sparse helpers (#16148) Jun 24, 2019
@alexhenrie alexhenrie changed the title PERF Only call malloc twice in liblinear to_sparse helpers (#16148) PERF Only call malloc twice in liblinear to_sparse helpers Jun 25, 2019
@alexhenrie alexhenrie force-pushed the alexhenrie:prealloc branch from ac68aef to c733942 Jun 25, 2019
@jnothman

This comment has been minimized.

Copy link
Member

jnothman commented Jun 26, 2019

@alexhenrie

This comment has been minimized.

Copy link
Contributor Author

alexhenrie commented Jul 1, 2019

@rth Could you merge this please? Feel free to run your own benchmarks.

@rth

This comment has been minimized.

Copy link
Member

rth commented Jul 3, 2019

Thanks @alexhenrie , I'm just not too confident in my ability to review C code.

Maybe @jeremiedbb or @pierreglaser would be interested in doing a second review?

@jeremiedbb

This comment has been minimized.

Copy link
Member

jeremiedbb commented Jul 3, 2019

We looked at it with pierre and it seems fine.
I agree with joel that you should add a comment in the code of free_problem to explain why you only need to free problem[0].

There's one thing not related to this PR still. When the malloc fails, the error is not handled anywhere after in liblinear.pyx (unlike for libswm which handles the error in libsvm.pyx). We should handle it when calling set_problem in liblinear also.

@rth

This comment has been minimized.

Copy link
Member

rth commented Jul 3, 2019

Thanks @jeremiedbb , feel free to merge then )

@alexhenrie

This comment has been minimized.

Copy link
Contributor Author

alexhenrie commented Jul 3, 2019

@jeremiedbb Thanks for reviewing this! In his comment above, @jnothman said that he just wanted to know what the reason was and he did not think a comment is necessary in the code. If you want a C comment explaining free(problem->x[0]) I'm happy to add one, just let me know.

@jeremiedbb jeremiedbb merged commit 0f7e470 into scikit-learn:master Jul 5, 2019
16 checks passed
16 checks passed
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No code changes detected
Details
LGTM analysis: C/C++ No new or fixed alerts
Details
ci/circleci: deploy Your tests passed on CircleCI!
Details
ci/circleci: doc Your tests passed on CircleCI!
Details
ci/circleci: doc-min-dependencies Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
codecov/patch Coverage not affected when comparing 1dc7b1d...c733942
Details
codecov/project 96.82% (-0.01%) compared to 1dc7b1d
Details
scikit-learn.scikit-learn Build #20190625.27 succeeded
Details
scikit-learn.scikit-learn (Linux py35_conda_openblas) Linux py35_conda_openblas succeeded
Details
scikit-learn.scikit-learn (Linux py35_np_atlas) Linux py35_np_atlas succeeded
Details
scikit-learn.scikit-learn (Linux pylatest_conda) Linux pylatest_conda succeeded
Details
scikit-learn.scikit-learn (Windows py35_32) Windows py35_32 succeeded
Details
scikit-learn.scikit-learn (Windows py37_64) Windows py37_64 succeeded
Details
scikit-learn.scikit-learn (macOS pylatest_conda) macOS pylatest_conda succeeded
Details
@jeremiedbb

This comment has been minimized.

Copy link
Member

jeremiedbb commented Jul 5, 2019

it's fine. It's not like we modify this code very often :)

Thanks @alexhenrie !

@alexhenrie

This comment has been minimized.

Copy link
Contributor Author

alexhenrie commented Jul 5, 2019

Thank you!

@alexhenrie alexhenrie deleted the alexhenrie:prealloc branch Jul 8, 2019
koenvandevelde added a commit to koenvandevelde/scikit-learn that referenced this pull request Jul 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.