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

Rename and simplify store_list #5751

Merged
merged 1 commit into from Apr 26, 2018

Conversation

Projects
None yet
2 participants
@stuhood
Copy link
Member

stuhood commented Apr 26, 2018

Problem

  1. store_list creates tuples rather than lists
  2. It has an unused merge parameter (from when we used to use store_list to flatten lists).
  3. It requires more allocations than are strictly necessary because it takes a Vec and then copies it to change its shape.

Solution

Rename to store_tuple, remove the merge parameter, and change the signature to taking a &[Value], which allows for use of arrays on the stack, as well as direct usage of Vec<Value>, which we have in most (all?) cases.

Result

Fewer allocations, simpler API.

@stuhood stuhood requested review from jsirois and illicitonion Apr 26, 2018

@illicitonion
Copy link
Contributor

illicitonion left a comment

Nice!

@stuhood stuhood force-pushed the twitter:stuhood/reduce-allocation-in-store-list branch from 92e2501 to bb59a27 Apr 26, 2018

@stuhood stuhood merged commit a8548cc into pantsbuild:master Apr 26, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@stuhood stuhood deleted the twitter:stuhood/reduce-allocation-in-store-list branch Apr 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment