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

kernel: optimize LcmInt for small inputs #2159

Merged
merged 1 commit into from
Feb 9, 2018

Conversation

fingolfin
Copy link
Member

@fingolfin fingolfin commented Feb 5, 2018

Motivated by PR #2154 (which made me realize that LcmInt(100, 1) will allocate a temporary T_INTPOS object only to immediately discard it again), this PR gives a tiny speed boost, but more importantly, avoids unnecessary allocation of temporary memory.

Before:

gap> for i in [1..10000000] do LcmInt(2^50*13, 2^50*17); od; time;
2655
gap> for i in [1..10000000] do LcmInt(2^60*13, 2^60*17); od; time;
4450
gap> for i in [1..10000000] do LcmInt(2^70*13, 2^70*17); od; time;
5831

After:

gap> for i in [1..10000000] do LcmInt(2^50*13, 2^50*17); od; time;
2381
gap> for i in [1..10000000] do LcmInt(2^60*13, 2^60*17); od; time;
4505
gap> for i in [1..10000000] do LcmInt(2^70*13, 2^70*17); od; time;
5854

@fingolfin fingolfin added topic: performance bugs or enhancements related to performance (improvements or regressions) topic: kernel labels Feb 5, 2018
@codecov
Copy link

codecov bot commented Feb 5, 2018

Codecov Report

Merging #2159 into master will increase coverage by 1.22%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2159      +/-   ##
==========================================
+ Coverage   68.34%   69.57%   +1.22%     
==========================================
  Files         481      483       +2     
  Lines      254474   256113    +1639     
==========================================
+ Hits       173927   178186    +4259     
+ Misses      80547    77927    -2620
Impacted Files Coverage Δ
src/integer.c 90.31% <100%> (+0.05%) ⬆️
src/gasman.c 78.74% <0%> (-6.18%) ⬇️
src/stats.c 80.1% <0%> (-4.68%) ⬇️
src/permutat.c 81.78% <0%> (-4.04%) ⬇️
hpcgap/lib/hpc/queue.g 66.4% <0%> (-3.2%) ⬇️
src/pperm.c 83.66% <0%> (-1.79%) ⬇️
src/sysfiles.c 35% <0%> (-1.03%) ⬇️
src/hpc/threadapi.c 36.71% <0%> (-0.19%) ⬇️
src/opers.c 81.41% <0%> (ø) ⬆️
lib/debug.g 10.18% <0%> (ø)
... and 153 more

This gives a tiny speed boost, but more importantly, avoids
unnecessary allocation of temporary memory

Before:

gap> for i in [1..10000000] do LcmInt(2^50*13, 2^50*17); od; time;
2655
gap> for i in [1..10000000] do LcmInt(2^60*13, 2^60*17); od; time;
4450
gap> for i in [1..10000000] do LcmInt(2^70*13, 2^70*17); od; time;
5831

After:

gap> for i in [1..10000000] do LcmInt(2^50*13, 2^50*17); od; time;
2381
gap> for i in [1..10000000] do LcmInt(2^60*13, 2^60*17); od; time;
4505
gap> for i in [1..10000000] do LcmInt(2^70*13, 2^70*17); od; time;
5854
@fingolfin
Copy link
Member Author

Just for the kicks, this is what I get in GAP 4.8 (of course most of the speed up is due to @markuspf work on LcmInt, and mine on GMP support in general):

gap> for i in [1..10000000] do LcmInt(2^50*13, 2^50*17); od; time;
5333
gap> for i in [1..10000000] do LcmInt(2^60*13, 2^60*17); od; time;
22452
gap> for i in [1..10000000] do LcmInt(2^70*13, 2^70*17); od; time;
24485

@fingolfin fingolfin merged commit 89c169b into gap-system:master Feb 9, 2018
@fingolfin fingolfin deleted the mh/LcmInt-opt branch February 9, 2018 13:18
@fingolfin fingolfin added the release notes: added PRs introducing changes that have since been mentioned in the release notes label Mar 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: added PRs introducing changes that have since been mentioned in the release notes topic: kernel topic: performance bugs or enhancements related to performance (improvements or regressions)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants