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

Have extern::generator_send directly produce TypeIds for product types #7318

Conversation

Projects
None yet
3 participants
@stuhood
Copy link
Member

commented Mar 6, 2019

Problem

#7114 set us up to use lighter-weight identification of product types in externs::generator_send. We can additionally avoid calling back to python to memoize product types by having methods return TypeIds rather than Values representing types.

Solution

Rather than a ValueBuffer containing type instances, memoize the types and return a TypeIdBuffer.

Result

Fewer calls back to python.

Have extern::generator_send directly produce TypeIds for product type…
…s, which avoids calling back to python to identify types.
@cosmicexplorer
Copy link
Contributor

left a comment

Burn all the externs!!! I've been thinking about the text in interning.rs about how values can never be collected -- no ideas yet.

@@ -555,6 +548,18 @@ impl TypeIdBuffer {
pub fn to_vec(&self) -> Vec<TypeId> {
with_vec(self.ids_ptr, self.ids_len as usize, |vec| vec.clone())
}

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 6, 2019

Contributor

I might do something like:

pub trait FFIBuffer<T, E> {
  fn to_vec(&self) -> Vec<T>;
  fn unwrap_one(&self) -> Result<T, E>;
}

impl FFIBuffer<Value, String> for HandleBuffer { ... }

impl FFIBuffer<TypeId, String> for TypeIdBuffer { ... }

but there might be a better way to split that off e.g. requiring each type impl *_ptr() and *_len() methods, and then implementing to_vec() and unwrap_one() generically, but that might be too far for right now.

This comment has been minimized.

Copy link
@stuhood

stuhood Mar 6, 2019

Author Member

I'm not sure the generics are worth it in this case... if we grow a third copy of this method we can dive in there =)

This comment has been minimized.

Copy link
@stuhood

stuhood Mar 6, 2019

Author Member

Added a comment to this effect.

lazy_static! {
static ref PRODUCT_TYPE_ID_HASH_BUILDER: FNV = FNV::default();
}

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 6, 2019

Contributor

Thank you for removing this nonsense!

@@ -242,7 +238,7 @@ pub fn generator_send(generator: &Value, arg: &Value) -> Result<GeneratorRespons
.into_iter()
.zip(values.into_iter())
.map(|(p, v)| Get {
product: *interns.insert_product(p).type_id(),
product: p,

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 6, 2019

Contributor

I might add a TODO here or on the python side to do more interning in extern_generator_send() in python so that we can centralize the process of interning in one place (I don't know how to do this yet given your comment on Breaks at #7114 (comment), but it seems good to note maybe).

@illicitonion
Copy link
Contributor

left a comment

Beautiful :) Thanks!

@stuhood stuhood merged commit f376c32 into pantsbuild:master Mar 6, 2019

1 check passed

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

@stuhood stuhood deleted the twitter:stuhood/generator-send-produces-typeids-for-products branch Mar 6, 2019

stuhood added a commit that referenced this pull request Apr 10, 2019

Improve Get errors for unhashable or mismatched types (#7502)
### Problem

The error for unhashable types used as `Get` params is unfortunate, because the call to `identify` fails and returns a null pointer to rust, which triggers an error... somewhere else. See #7499.

### Solution

As suggested in #7499, have `extern_generator_send` directly `identify` `Get` params, meaning that failures to hash take advantage of the fact that `extern_generator_send` is already fallible. This avoids a significant number of calls back to python to identify `Values` (an optimization discussed in #7318 and #7114).

### Result

Improved usability. Fixes #7499.
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.