Skip to content

Commit

Permalink
Have extern::generator_send directly produce TypeIds for product types (
Browse files Browse the repository at this point in the history 
#7318)

### 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.
  • Loading branch information
Stu Hood committed Mar 6, 2019
1 parent f101854 commit f376c32
Show file tree
Hide file tree
Showing 4 changed files with 36 additions and 49 deletions.
13 changes: 1 addition & 12 deletions src/python/pants/engine/native.py
View file
Expand Up @@ -276,16 +276,6 @@ def extern_log(self, context_handle, level, msg_ptr, msg_len):
msg = bytes(self._ffi.buffer(msg_ptr, msg_len)).decode('utf-8')
logger.log(level, msg)

@_extern_decl('TypeId', ['ExternContext*', 'Handle*'])
def extern_product_type(self, context_handle, val):
"""Return a TypeId for the given Handle, which must be a `type`."""
c = self._ffi.from_handle(context_handle)
obj = self._ffi.from_handle(val[0])
# TODO: determine if this assertion has any performance implications.
assert isinstance(obj, type)
tid = c.to_id(obj)
return TypeId(tid)

@_extern_decl('TypeId', ['ExternContext*', 'Handle*'])
def extern_get_type_for(self, context_handle, val):
"""Return a representation of the object's type."""
Expand Down Expand Up @@ -457,7 +447,7 @@ def extern_generator_send(self, context_handle, func, arg):
return (
tag,
c.vals_buf([c.to_value(v) for v in values]),
c.vals_buf([c.to_value(v) for v in products])
c.type_ids_buf([TypeId(c.to_id(t)) for t in products])
)

@_extern_decl('PyResult', ['ExternContext*', 'Handle*', 'Handle**', 'uint64_t'])
Expand Down Expand Up @@ -633,7 +623,6 @@ def init_externs():
self.ffi_lib.extern_call,
self.ffi_lib.extern_generator_send,
self.ffi_lib.extern_eval,
self.ffi_lib.extern_product_type,
self.ffi_lib.extern_get_type_for,
self.ffi_lib.extern_identify,
self.ffi_lib.extern_equals,
Expand Down
38 changes: 28 additions & 10 deletions src/rust/engine/src/externs.rs
View file
Expand Up @@ -20,10 +20,6 @@ pub fn eval(python: &str) -> Result<Value, Failure> {
with_externs(|e| (e.eval)(e.context, python.as_ptr(), python.len() as u64)).into()
}

pub fn product_type(val: &Value) -> TypeId {
with_externs(|e| (e.product_type)(e.context, val as &Handle))
}

pub fn get_type_for(val: &Value) -> TypeId {
with_externs(|e| (e.get_type_for)(e.context, val as &Handle))
}
Expand Down Expand Up @@ -212,6 +208,11 @@ pub fn call(func: &Value, args: &[Value]) -> Result<Value, Failure> {
.into()
}

///
/// TODO: If we added support for inserting to the `Interns` using an `Ident`, `PyGeneratorResponse`
/// could directly return `Idents` during `Get` calls. This would also require splitting its fields
/// further to avoid needing to "identify" the result of a `PyGeneratorResponseType::Break`.
///
pub fn generator_send(generator: &Value, arg: &Value) -> Result<GeneratorResponse, Failure> {
let response =
with_externs(|e| (e.generator_send)(e.context, generator as &Handle, arg as &Handle));
Expand All @@ -223,7 +224,7 @@ pub fn generator_send(generator: &Value, arg: &Value) -> Result<GeneratorRespons
let p = response.products.unwrap_one();
let v = response.values.unwrap_one();
let g = Get {
product: *interns.insert_product(p).type_id(),
product: p,
subject: interns.insert(v),
};
Ok(GeneratorResponse::Get(g))
Expand All @@ -242,7 +243,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,
subject: interns.insert(v),
})
.collect();
Expand Down Expand Up @@ -341,7 +342,6 @@ pub struct Externs {
pub call: CallExtern,
pub generator_send: GeneratorSendExtern,
pub eval: EvalExtern,
pub product_type: ProductTypeExtern,
pub get_type_for: GetTypeForExtern,
pub identify: IdentifyExtern,
pub equals: EqualsExtern,
Expand All @@ -368,8 +368,6 @@ unsafe impl Send for Externs {}

pub type LogExtern = extern "C" fn(*const ExternContext, u8, str_ptr: *const u8, str_len: u64);

pub type ProductTypeExtern = extern "C" fn(*const ExternContext, *const Handle) -> TypeId;

pub type GetTypeForExtern = extern "C" fn(*const ExternContext, *const Handle) -> TypeId;

pub type IdentifyExtern = extern "C" fn(*const ExternContext, *const Handle) -> Ident;
Expand Down Expand Up @@ -479,7 +477,7 @@ pub enum PyGeneratorResponseType {
pub struct PyGeneratorResponse {
res_type: PyGeneratorResponseType,
values: HandleBuffer,
products: HandleBuffer,
products: TypeIdBuffer,
}

#[derive(Debug)]
Expand Down Expand Up @@ -529,7 +527,11 @@ impl HandleBuffer {
})
}

///
/// Asserts that the HandleBuffer contains one value, and returns it.
///
/// NB: Consider making generic and merging with TypeIdBuffer if we get a third copy.
///
pub fn unwrap_one(&self) -> Value {
assert!(
self.handles_len == 1,
Expand All @@ -555,6 +557,22 @@ impl TypeIdBuffer {
pub fn to_vec(&self) -> Vec<TypeId> {
with_vec(self.ids_ptr, self.ids_len as usize, |vec| vec.clone())
}

///
/// Asserts that the TypeIdBuffer contains one TypeId, and returns it.
///
/// NB: Consider making generic and merging with HandleBuffer if we get a third copy.
///
pub fn unwrap_one(&self) -> TypeId {
assert!(
self.ids_len == 1,
"TypeIdBuffer contained more than one value: {}",
self.ids_len
);
with_vec(self.ids_ptr, self.ids_len as usize, |ids_vec| {
*ids_vec.iter().next().unwrap()
})
}
}

pub type ProjectIgnoringTypeExtern = extern "C" fn(
Expand Down
26 changes: 4 additions & 22 deletions src/rust/engine/src/interning.rs
View file
Expand Up @@ -2,11 +2,9 @@
// Licensed under the Apache License, Version 2.0 (see LICENSE).

use std::collections::HashMap;
use std::hash::{self, BuildHasher, Hash, Hasher};
use std::hash;

use lazy_static::lazy_static;

use crate::core::{Key, TypeId, Value, FNV};
use crate::core::{Key, Value, FNV};
use crate::externs::{self, Ident};

///
Expand Down Expand Up @@ -40,16 +38,13 @@ pub struct Interns {
id_generator: u64,
}

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

impl Interns {
pub fn new() -> Interns {
Interns::default()
}

fn perform_insert(&mut self, v: Value, hash: i64, type_id: TypeId) -> Key {
pub fn insert(&mut self, v: Value) -> Key {
let Ident { hash, type_id } = externs::identify(&v);
let mut inserted = false;
let id_generator = self.id_generator;
let key = *self
Expand All @@ -66,19 +61,6 @@ impl Interns {
key
}

pub fn insert_product(&mut self, v: Value) -> Key {
let type_id = externs::product_type(&v);
let mut hasher = PRODUCT_TYPE_ID_HASH_BUILDER.build_hasher();
type_id.hash(&mut hasher);
let hash: i64 = hasher.finish() as i64;
self.perform_insert(v, hash, type_id)
}

pub fn insert(&mut self, v: Value) -> Key {
let Ident { hash, type_id } = externs::identify(&v);
self.perform_insert(v, hash, type_id)
}

pub fn get(&self, k: &Key) -> &Value {
self
.reverse
Expand Down
8 changes: 3 additions & 5 deletions src/rust/engine/src/lib.rs
View file
Expand Up @@ -58,9 +58,9 @@ use crate::core::{Function, Key, Params, TypeId, Value};
use crate::externs::{
Buffer, BufferBuffer, CallExtern, CloneValExtern, CreateExceptionExtern, DropHandlesExtern,
EqualsExtern, EvalExtern, ExternContext, Externs, GeneratorSendExtern, GetTypeForExtern,
HandleBuffer, IdentifyExtern, LogExtern, ProductTypeExtern, ProjectIgnoringTypeExtern,
ProjectMultiExtern, PyResult, StoreBoolExtern, StoreBytesExtern, StoreF64Extern, StoreI64Extern,
StoreTupleExtern, StoreUtf8Extern, TypeIdBuffer, TypeToStrExtern, ValToStrExtern,
HandleBuffer, IdentifyExtern, LogExtern, ProjectIgnoringTypeExtern, ProjectMultiExtern, PyResult,
StoreBoolExtern, StoreBytesExtern, StoreF64Extern, StoreI64Extern, StoreTupleExtern,
StoreUtf8Extern, TypeIdBuffer, TypeToStrExtern, ValToStrExtern,
};
use crate::handles::Handle;
use crate::rule_graph::{GraphMaker, RuleGraph};
Expand Down Expand Up @@ -103,7 +103,6 @@ pub extern "C" fn externs_set(
call: CallExtern,
generator_send: GeneratorSendExtern,
eval: EvalExtern,
product_type: ProductTypeExtern,
get_type_for: GetTypeForExtern,
identify: IdentifyExtern,
equals: EqualsExtern,
Expand All @@ -130,7 +129,6 @@ pub extern "C" fn externs_set(
call,
generator_send,
eval,
product_type,
get_type_for,
identify,
equals,
Expand Down

0 comments on commit f376c32

Please sign in to comment.