Skip to content

Conversation

@kmod
Copy link
Collaborator

@kmod kmod commented Sep 1, 2015

llvm's DenseMap/DenseSet have a hardcoded minimum size of 64 buckets, which I think ends up wasting a lot of memory for us. They also take longer to iterate over (more empty buckets to skip), which is probably not as big a deal in the macrobenchmarks but shows up in some microbenchmarks.

So, fork their implementation and lower the minimum to 8 (parameterizable now). Surprisingly effective:

           django_template3.py             2.8s (2)             2.8s (2)  -0.8%
                 pyxl_bench.py             2.3s (2)             2.3s (2)  -0.1%
     sqlalchemy_imperative2.py             2.8s (2)             2.7s (2)  -4.4%
       django_template3_10x.py            17.5s (2)            17.1s (2)  -2.6%
             pyxl_bench_10x.py            17.5s (2)            17.4s (2)  -0.4%
 sqlalchemy_imperative2_10x.py            21.0s (2)            19.7s (2)  -6.2%
                       geomean                 7.0s                 6.8s  -2.5%

kmod added 6 commits September 1, 2015 23:24
With no modificaions in this commit, so that we can track changes.
Unmodified from llvm rev r230300.
- add note + license
- put in our namespace
- change header guards
- make it match our lint style
- don't apply our formatting
The default is 64, which is quite a lot for our uses.
By switching to our DenseMap/Set fork with that allows parameterizing
on this.

This is the same number that CPython uses.  We were previously
using llvm's default of 64, which is pretty high -- probably fine
for their use cases, but in Python programs with lots of sets/dicts
we spend a lot of time doing malloc() for all those 1kB+ allocations.
This also increases the time it takes to iterate over the dict/set,
since there are more empty buckets that have to be read + skipped.
kmod added a commit that referenced this pull request Sep 2, 2015
Lower initial dict/set size from 64->8
@kmod kmod merged commit 7eec7fa into pyston:master Sep 2, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant