[engine] Remove type_id field from Value #4274

Merged
merged 2 commits into from Feb 21, 2017

Conversation

Projects
None yet
4 participants
@stuhood
Member

stuhood commented Feb 20, 2017

Problem

The type_id field on Value began as a convenience to avoid needing to call back to python for satisfied_by checks. But the vast majority of Values do not participate in those checks, and computing ids for things is not cheap (in particular, post #4199 it dominates the runtime of Context.to_value).

Solution

Remove type_id from Value, and split out a satisfied_by_type extern from satisfied_by, to allow for executing satisfied_by for Values without creating keys for them.

Result

Total runtime for ./pants list is reduced by 10-15%.

@stuhood stuhood requested review from baroquebobcat and peiyuwang Feb 20, 2017

@stuhood stuhood requested review from JieGhost and kwlzn Feb 20, 2017

@kwlzn

kwlzn approved these changes Feb 21, 2017

lgtm

@baroquebobcat

lgtm.

@stuhood stuhood merged commit 158b868 into pantsbuild:master Feb 21, 2017

1 check passed

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

@stuhood stuhood deleted the twitter:stuhood/remove-typeid-from-value branch Feb 21, 2017

lenucksi added a commit to lenucksi/pants that referenced this pull request Apr 25, 2017

[engine] Remove type_id field from Value (#4274)
### Problem

The `type_id` field on `Value` began as a convenience to avoid needing to call back to python for `satisfied_by` checks. But the vast majority of `Value`s do not participate in those checks, and computing ids for things is not cheap (in particular, post #4199 it dominates the runtime of `Context.to_value`).

### Solution

Remove `type_id` from `Value`, and split out a `satisfied_by_type` extern from `satisfied_by`, to allow for executing `satisfied_by` for `Value`s without creating keys for them.

### Result

Total runtime for `./pants list` is reduced by 10-15%.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment