Skip to content

Move BorrowValue to rustpython-common, add BorrowedValue enum #2228

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.

Sign up for GitHub

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

Merged
coolreader18 merged 1 commit into from
Sep 23, 2020
+166 −7
Merged

Move BorrowValue to rustpython-common, add BorrowedValue enum #2228

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions common/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,3 +18,4 @@ cfg-if = "0.1"
once_cell = "1.4.1"
siphasher = "0.3"
rand = "0.7.3"
derive_more = "0.99.9"
66 changes: 66 additions & 0 deletions common/src/borrow.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
use crate::cell::{
PyMappedMutexGuard, PyMappedRwLockReadGuard, PyMappedRwLockWriteGuard, PyMutexGuard,
PyRwLockReadGuard, PyRwLockWriteGuard,
};
use std::ops::{Deref, DerefMut};

pub trait BorrowValue<'a> {
type Borrowed: 'a + Deref;
fn borrow_value(&'a self) -> Self::Borrowed;
}

#[derive(Debug, derive_more::From)]
pub enum BorrowedValue<'a, T: ?Sized> {
Copy link
Member

@youknowone youknowone Sep 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this better to be an enum rather than concrete type or boxed dyn?
I thought buffer protocol help to avoid this situation.

Copy link
Member Author

@coolreader18 coolreader18 Sep 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could probably add a Dyn variant that's just Box<dyn Deref> if eventually needed, but as is these are the most common forms of borrowing, and they all just have the underlying borrow stored as a reference/pointer, so no function calls are necessary to Deref it (unlike a dyn Deref)

Copy link
Member

@youknowone youknowone Sep 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I don't know rust pratice well, so this is trying discussion.
When I am seeing BorrowValue for specific type like PyBytesLike, it doesn't use all the enum fields but its borrowed type is BorrowedValue. I think this is giving unnessassary cost.
using enum for each case is ok - because it is what it need to be. But BorrowedValue itself as enum is hiding somewhat not expected to be hided.
my suggestion is, defining specific type for each borrowed types. BorrowedBytesLike or BorrowedRwBytes. And BorrowedValue itself as a trait (if it is needed). We can find better abstraction or we can use dyn BorrowedValue in future complicated scenarios.

Copy link

@BenLewis-Seequent BenLewis-Seequent Sep 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is good because with the buffer protocol PyBytesLike and PyRwBytesLike will become extensible and having a flexible return type from borrow_value will make it easier for implementer's of the buffer protocol. I don't this will actually have much cost as all variants probably will optimize down to a simply deref anyways.

Ref(&'a T),
MuLock(PyMutexGuard<'a, T>),
MappedMuLock(PyMappedMutexGuard<'a, T>),
ReadLock(PyRwLockReadGuard<'a, T>),
MappedReadLock(PyMappedRwLockReadGuard<'a, T>),
}

impl<T: ?Sized> Deref for BorrowedValue<'_, T> {
type Target = T;
fn deref(&self) -> &T {
match self {
Self::Ref(r) => r,
Self::MuLock(m) => &m,
Self::MappedMuLock(m) => &m,
Self::ReadLock(r) => &r,
Self::MappedReadLock(m) => &m,
}
}
}

#[derive(Debug, derive_more::From)]
pub enum BorrowedValueMut<'a, T: ?Sized> {
RefMut(&'a mut T),
MuLock(PyMutexGuard<'a, T>),
MappedMuLock(PyMappedMutexGuard<'a, T>),
WriteLock(PyRwLockWriteGuard<'a, T>),
MappedWriteLock(PyMappedRwLockWriteGuard<'a, T>),
}

impl<T: ?Sized> Deref for BorrowedValueMut<'_, T> {
type Target = T;
fn deref(&self) -> &T {
match self {
Self::RefMut(r) => r,
Self::MuLock(m) => &m,
Self::MappedMuLock(m) => &m,
Self::WriteLock(w) => &w,
Self::MappedWriteLock(w) => &w,
}
}
}

impl<T: ?Sized> DerefMut for BorrowedValueMut<'_, T> {
fn deref_mut(&mut self) -> &mut T {
match self {
Self::RefMut(r) => r,
Self::MuLock(m) => &mut *m,
Self::MappedMuLock(m) => &mut *m,
Self::WriteLock(w) => &mut *w,
Self::MappedWriteLock(w) => &mut *w,
}
}
}
59 changes: 57 additions & 2 deletions common/src/cell.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ pub use once_cell::sync::{Lazy, OnceCell};
pub use once_cell::unsync::{Lazy, OnceCell};
#[cfg(feature = "threading")]
use parking_lot::{
MappedRwLockReadGuard, MappedRwLockWriteGuard, Mutex, MutexGuard, RwLock, RwLockReadGuard,
RwLockWriteGuard,
MappedMutexGuard, MappedRwLockReadGuard, MappedRwLockWriteGuard, Mutex, MutexGuard, RwLock,
RwLockReadGuard, RwLockWriteGuard,
};
#[cfg(not(feature = "threading"))]
use std::cell::{Ref, RefCell, RefMut};
Expand All @@ -15,6 +15,7 @@ cfg_if::cfg_if! {
if #[cfg(feature = "threading")] {
type MutexInner<T> = Mutex<T>;
type MutexGuardInner<'a, T> = MutexGuard<'a, T>;
type MappedMutexGuardInner<'a, T> = MappedMutexGuard<'a, T>;
const fn new_mutex<T>(value: T) -> MutexInner<T> {
parking_lot::const_mutex(value)
}
Expand All @@ -24,6 +25,7 @@ cfg_if::cfg_if! {
} else {
type MutexInner<T> = RefCell<T>;
type MutexGuardInner<'a, T> = RefMut<'a, T>;
type MappedMutexGuardInner<'a, T> = RefMut<'a, T>;
const fn new_mutex<T>(value: T) -> MutexInner<T> {
RefCell::new(value)
}
Expand Down Expand Up @@ -63,6 +65,39 @@ impl<T: ?Sized> DerefMut for PyMutexGuard<'_, T> {
self.0.deref_mut()
}
}
impl<'a, T: ?Sized> PyMutexGuard<'a, T> {
#[inline]
pub fn map<U: ?Sized, F>(s: Self, f: F) -> PyMappedMutexGuard<'a, U>
where
F: FnOnce(&mut T) -> &mut U,
{
PyMappedMutexGuard(MutexGuardInner::map(s.0, f))
}
}

#[derive(Debug)]
#[repr(transparent)]
pub struct PyMappedMutexGuard<'a, T: ?Sized>(MappedMutexGuardInner<'a, T>);
impl<T: ?Sized> Deref for PyMappedMutexGuard<'_, T> {
type Target = T;
fn deref(&self) -> &T {
self.0.deref()
}
}
impl<T: ?Sized> DerefMut for PyMappedMutexGuard<'_, T> {
fn deref_mut(&mut self) -> &mut T {
self.0.deref_mut()
}
}
impl<'a, T: ?Sized> PyMappedMutexGuard<'a, T> {
#[inline]
pub fn map<U: ?Sized, F>(s: Self, f: F) -> PyMappedMutexGuard<'a, U>
where
F: FnOnce(&mut T) -> &mut U,
{
PyMappedMutexGuard(MappedMutexGuardInner::map(s.0, f))
}
}

cfg_if::cfg_if! {
if #[cfg(feature = "threading")] {
Expand Down Expand Up @@ -127,6 +162,7 @@ impl<T: ?Sized> Deref for PyRwLockReadGuard<'_, T> {
}
}

#[derive(Debug)]
#[repr(transparent)]
pub struct PyMappedRwLockReadGuard<'a, T: ?Sized>(MappedRwLockReadInner<'a, T>);
impl<T: ?Sized> Deref for PyMappedRwLockReadGuard<'_, T> {
Expand All @@ -145,6 +181,15 @@ impl<'a, T: ?Sized> PyRwLockReadGuard<'a, T> {
PyMappedRwLockReadGuard(RwLockReadInner::map(s.0, f))
}
}
impl<'a, T: ?Sized> PyMappedRwLockReadGuard<'a, T> {
#[inline]
pub fn map<U: ?Sized, F>(s: Self, f: F) -> PyMappedRwLockReadGuard<'a, U>
where
F: FnOnce(&T) -> &U,
{
PyMappedRwLockReadGuard(MappedRwLockReadInner::map(s.0, f))
}
}

#[derive(Debug)]
#[repr(transparent)]
Expand All @@ -161,6 +206,7 @@ impl<T: ?Sized> DerefMut for PyRwLockWriteGuard<'_, T> {
}
}

#[derive(Debug)]
#[repr(transparent)]
pub struct PyMappedRwLockWriteGuard<'a, T: ?Sized>(MappedRwLockWriteInner<'a, T>);
impl<T: ?Sized> Deref for PyMappedRwLockWriteGuard<'_, T> {
Expand All @@ -184,3 +230,12 @@ impl<'a, T: ?Sized> PyRwLockWriteGuard<'a, T> {
PyMappedRwLockWriteGuard(RwLockWriteInner::map(s.0, f))
}
}
impl<'a, T: ?Sized> PyMappedRwLockWriteGuard<'a, T> {
#[inline]
pub fn map<U: ?Sized, F>(s: Self, f: F) -> PyMappedRwLockWriteGuard<'a, U>
where
F: FnOnce(&mut T) -> &mut U,
{
PyMappedRwLockWriteGuard(MappedRwLockWriteInner::map(s.0, f))
}
}
1 change: 1 addition & 0 deletions common/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
//! A crate to hold types and functions common to all rustpython components.

pub mod borrow;
pub mod cell;
pub mod float_ops;
pub mod hash;
Expand Down
27 changes: 27 additions & 0 deletions vm/src/byteslike.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use crate::common::borrow::{BorrowedValue, BorrowedValueMut};
use crate::common::cell::{PyRwLockReadGuard, PyRwLockWriteGuard};
use crate::obj::objbytearray::{PyByteArray, PyByteArrayRef};
use crate::obj::objbytes::{PyBytes, PyBytesRef};
use crate::pyobject::PyObjectRef;
Expand Down Expand Up @@ -26,6 +28,19 @@ impl TryFromObject for PyBytesLike {
}
}

impl<'a> BorrowValue<'a> for PyBytesLike {
type Borrowed = BorrowedValue<'a, [u8]>;
fn borrow_value(&'a self) -> Self::Borrowed {
match self {
Self::Bytes(b) => b.borrow_value().into(),
Self::Bytearray(b) => {
PyRwLockReadGuard::map(b.borrow_value(), |b| b.elements.as_slice()).into()
}
Self::Array(a) => a.get_bytes().into(),
}
}
}

impl PyBytesLike {
pub fn len(&self) -> usize {
match self {
Expand Down Expand Up @@ -73,6 +88,18 @@ impl TryFromObject for PyRwBytesLike {
}
}

impl<'a> BorrowValue<'a> for PyRwBytesLike {
type Borrowed = BorrowedValueMut<'a, [u8]>;
fn borrow_value(&'a self) -> Self::Borrowed {
match self {
Self::Bytearray(b) => {
PyRwLockWriteGuard::map(b.borrow_value_mut(), |b| b.elements.as_mut_slice()).into()
}
Self::Array(a) => a.get_bytes_mut().into(),
}
}
}

impl PyRwBytesLike {
pub fn len(&self) -> usize {
match self {
Expand Down
7 changes: 2 additions & 5 deletions vm/src/pyobject.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ use crate::vm::VirtualMachine;
use rustpython_common::cell::{PyRwLock, PyRwLockReadGuard};
use rustpython_common::rc::PyRc;

pub use crate::common::borrow::BorrowValue;

/* Python objects and references.

Okay, so each python object itself is an class itself (PyObject). Each
Expand Down Expand Up @@ -1175,11 +1177,6 @@ pub trait PyValue: fmt::Debug + PyThreadingConstraint + Sized + 'static {
}
}

pub trait BorrowValue<'a>: PyValue {
type Borrowed: 'a + Deref;
fn borrow_value(&'a self) -> Self::Borrowed;
}

pub trait PyObjectPayload: Any + fmt::Debug + PyThreadingConstraint + 'static {
fn as_any(&self) -> &dyn Any;
}
Expand Down