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

Small integer preallocation optimization #825

Merged
merged 136 commits into from Jun 15, 2018

Conversation

Projects
None yet
@patiences
Copy link
Contributor

commented May 29, 2018

This PR implements the preallocation and initialization of the integers [-5, 257) so that they can be reused. In general, this means that when the python code is parsed and an integer object is detected, if it is in this range we will not create a new Integer object but rather use the preallocated one.

Object construction is expensive memory and time-wise. We can see a performance gain even running a simple performance test like this one:

Pre-optimization

Running test_small_integers ...
  Elapsed time:  33.07889499713201  ms
  CPU process time:  289.66  ms

With optimization

Running test_small_integers ...
  Elapsed time:  23.559533001796808  ms
  CPU process time:  240.0899999999998  ms

(Of course there is always some variance between different runs, but from what I've seen there is consistent improvement.)

@patiences

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2018

Some updated numbers:

Running test_small_integers
Elapsed time: 122.01105412701145 sec
CPU process time: 0.0006999999999999784 sec

Running test_small_integers
Elapsed time: 128.66034104398568 sec
CPU process time: 0.0008010000000000517 sec

Running test_small_integers
Elapsed time: 122.50824150099652 sec
CPU process time: 0.00048599999999998644 sec

Running test_small_integers
Elapsed time: 122.89947666198714 sec
CPU process time: 0.0008429999999999827 sec

Running test_small_integers
Elapsed time: 122.97956180298934 sec
CPU process time: 0.00045000000000000595 sec

@freakboy3742

This comment has been minimized.

Copy link
Member

commented Jun 7, 2018

@patiences Do you have baseline numbers for the benchmark? The ticket only has the "progress" numbers...

@patiences

This comment has been minimized.

Copy link
Contributor Author

commented Jun 8, 2018

@freakboy3742 Yes, of course! Here we go:

Pre-optimization
Running test_small_integers
Elapsed time: 144.74749750000774 sec
CPU process time: 0.00047200000000002795 sec

Running test_small_integers
Elapsed time: 143.55906505999155 sec
CPU process time: 0.0010949999999999571 sec

Running test_small_integers
Elapsed time: 149.2436827890051 sec
CPU process time: 0.0008159999999999834 sec

Running test_small_integers
Elapsed time: 146.150813241984 sec
CPU process time: 0.0008259999999999934 sec

Looks like about a 15% improvement.

@freakboy3742
Copy link
Member

left a comment

This looks great! There's a merge conflict that needs to be resolved (as a result of the Bool optimisation landing); python/testfile.class has also been inadvertently added.

That file needs to be stripped out (ideally, rolling back the commit where it was added; if you just delete the file, it will still be in the commit history, and so every person who clones the repo will download the file, even though it doesn't exist any more).

We should also modify .gitignore to make sure we can't add .class files in future (or, at least, not easily :-)

@patiences

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2018

python/testfile.class has also been inadvertently added.

That file needs to be stripped out (ideally, rolling back the commit where it was added; if you just >delete the file, it will still be in the commit history, and so every person who clones the repo will >download the file, even though it doesn't exist any more).

@freakboy3742 Really? That is so weird. Is that the case with .py files too (some of those got added and deleted at other points too, oops)? I actually am not sure how to revert just the specific commit (bb40b5b) -- I tried to rebase it without that commit locally but I'm not sure that's going to work...

Also, the performance benchmarking tests (#824) could use a look as well :-)

@freakboy3742
Copy link
Member

left a comment

The duplication of Int constants (NSMALLNEGINTS and NSMALLPOSINTS) can be deleted, but other than that this is good to merge!

# The number of negative and positive small org/python/types/Int objects that are preallocated
# [-NSMALLNEGINTS, NSMALLPOSINTS)
NSMALLNEGINTS = 5
NSMALLPOSINTS = 257

This comment has been minimized.

Copy link
@freakboy3742

freakboy3742 Jun 14, 2018

Member

I don't think this is needed anymore

@patiences patiences force-pushed the patiences:smallint-preallocation branch from 9007a0b to aba0292 Jun 14, 2018

patiences added some commits Jun 14, 2018

@freakboy3742 freakboy3742 merged commit 89393a3 into beeware:master Jun 15, 2018

5 checks passed

beekeeper:0/beefore:javacheckstyle Java lint checks passed.
Details
beekeeper:0/beefore:pycodestyle Python lint checks passed.
Details
beekeeper:1/smoke-test Smoke build (Python 3.4) passed.
Details
beekeeper:2/full-test:py3.5 Python 3.5 tests passed.
Details
beekeeper:2/full-test:py3.6 Python 3.6 tests passed.
Details

@patiences patiences deleted the patiences:smallint-preallocation branch Jul 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.