-
-
Notifications
You must be signed in to change notification settings - Fork 681
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
Oco
(Owned Clones Once) smart pointer
#1480
Conversation
Can you say more about this one? I’m not familiar with the mechanics of converting |
To think about it, that's an obvious behavior for an I think it would be possible to manually create a special low-level data structure, similar to So the regular |
Owned Clones Once
smart pointerOco
(Owned Clones Once) smart pointer
In the last commits the original /// "Owned Clones Once" - a smart pointer that can be either a reference,
/// an owned value, or a reference counted pointer. This is useful for
/// storing immutable values, such as strings, in a way that is cheap to
/// clone and pass around.
/// The Clone implementation is amortized O(1), and will only clone the
/// [`Oco::Owned`] variant once, while converting it into [`Oco::Counted`].
pub enum Oco<'a, T: ?Sized + ToOwned + 'a> {
/// A static reference to a value.
Borrowed(&'a T),
/// A reference counted pointer to a value.
Counted(Rc<T>),
/// An owned value.
Owned(<T as ToOwned>::Owned),
} After analyzing the possible consequences of the introduction of the |
@DanikVitek I'm not sure I understand what this type does. Could you please help me with a layman's explanation of what this is used for? |
@rakshith-ravi let s1 = Oco::from("hello, world".to_string()); // Oco::<'_, str>::Owned(String)
let s2 = s1.clone(); // value of s1 is cloned into s2 and converted into Oco::<'_, str>::Counted(Rc<str>)
let s3 = s2.clone(); // the same value as in s2, with just the reference counter incremented Also now, if you have a custom type that may be cloned a lot and contains |
Okay, so essentially this clones the first time, but not after that? Why would we need that? Also, wouldn't Sorry I'm poking a lot, but I'm not sure |
@rakshith-ravi |
This is intended primarily as an internal type, used in a few places in the reactive system and DOM renderer to make it much cheaper to clone things like text nodes. This type guarantees that the contents of the string are cloned a maximum of one time, even if you clone and reuse it in many places. Obviously open to name suggestions but I wouldn't anticipate users of the library needing to use this themselves. |
Ahh, got it. If it's not used by the library users a lot, then I suppose it shouldn't be much of an issue.
I'll let you know if I can come up with something |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry it's taken me so long to review this in more depth.
I think this is in good shape overall.
It looks like it needs a rebase as there are some merge conflicts.
docs/book/src/metadata.md
Outdated
@@ -10,7 +10,7 @@ That’s where the [`leptos_meta`](https://docs.rs/leptos_meta/latest/leptos_met | |||
|
|||
`leptos_meta` provides special components that let you inject data from inside components anywhere in your application into the `<head>`: | |||
|
|||
[`<Title/>`](https://docs.rs/leptos_meta/latest/leptos_meta/fn.Title.html) allows you to set the document’s title from any component. It also takes a `formatter` function that can be used to apply the same format to the title set by other pages. So, for example, if you put `<Title formatter=|text| format!("{text} — My Awesome Site")/>` in your `<App/>` component, and then `<Title text="Page 1"/>` and `<Title text="Page 2"/>` on your routes, you’ll get `Page 1 — My Awesome Site` and `Page 2 — My Awesome Site`. | |||
[`<Title/>`](https://docs.rs/leptos_meta/latest/leptos_meta/fn.Title.html) allows you to set the document’s title from any component. It also takes a `formatter` function that can be used to apply the same format to the title set by other pages. So, for example, if you put `<Title formatter=|text| format!("{text} — My Awesome Site").into()/>` in your `<App/>` component, and then `<Title text="Page 1"/>` and `<Title text="Page 2"/>` on your routes, you’ll get `Page 1 — My Awesome Site` and `Page 2 — My Awesome Site`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather not change parts of the API that then require the user to do additional work. For example, the text of the <Title/>
is not something that's going to be cloned frequently—in fact, I don't think the <Title/>
component ever clones it.
meta/src/title.rs
Outdated
@@ -36,11 +36,11 @@ impl std::fmt::Debug for TitleContext { | |||
|
|||
/// A function that is applied to the text value before setting `document.title`. | |||
#[repr(transparent)] | |||
pub struct Formatter(Box<dyn Fn(String) -> String>); | |||
pub struct Formatter(Box<dyn Fn(Oco<'static, str>) -> Oco<'static, str>>); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(See comment above — I'd rather keep this one Fn(String) -> String
, as it ~never needs to be cloned and is additional work for the user)
Ok, I'll revert those changes and merge |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I re-ran the js-framework-benchmark
comparing this branch to current main and found no performance effect, which is perfect: there are no nodes being cloned in the implementation of that benchmark, so no improvement would be expected, and there was no performance regression.
There is a small increase in WASM binary size, about ~8kb in release mode at opt-level = "3"
. This is completely fine: ~1kb or less at opt-level = "z"
and compressed.
It is, of course, a breaking change in a technical sense, but given that it did not require updating even a single example, I think the impact on users should be minimal.
There are a number of places where we do clone View
s, including in the Suspense
/Transition
implementation, so I would expect this to benefit users across the board.
Thank you for all your work on this, and well done! Merging now.
This PR is dedicated to integrating the following type into the Leptos ecosystem:
This smart pointer is designed primarily to replace usages of
Cow<'static, str>
withImmutable<'static, str>
. Leptos uses strings just to pass them later as a&str
to the DOM, so all strings are ultimately immutable. In that case, it makes more sense to be able to refer to the same data when cloning strings, or when working with custom types, that manipulate strings.This was the case for me: in my web app, I'm passing around large strings, that represent images of high resolution.
In total, this should decrease the average memory usage of an application at any given point in time, though some cloning might occur when converting
&str
andString
intoRc<str>
due to the way it's implemented. But the originalString
will be immediately dropped and all further cloning will be essentially no-cost.