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

Merged
Changes from 1 commit
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -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."""
@@ -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'])
@@ -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,
@@ -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))
}
@@ -223,7 +219,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))
@@ -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).

subject: interns.insert(v),
})
.collect();
@@ -341,7 +337,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,
@@ -368,8 +363,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;
@@ -479,7 +472,7 @@ pub enum PyGeneratorResponseType {
pub struct PyGeneratorResponse {
res_type: PyGeneratorResponseType,
values: HandleBuffer,
products: HandleBuffer,
products: TypeIdBuffer,
}

#[derive(Debug)]
@@ -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.

/// Asserts that the TypeIdBuffer contains one TypeId, and returns it.
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(
@@ -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};

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

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!

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
@@ -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
@@ -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};
@@ -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,
@@ -130,7 +129,6 @@ pub extern "C" fn externs_set(
call,
generator_send,
eval,
product_type,
get_type_for,
identify,
equals,
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.