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

Use Cow<'static, str> in Named #896

Merged
merged 2 commits into from Aug 23, 2018

Conversation

Projects
None yet
7 participants
@randomPoison
Copy link
Member

commented Aug 22, 2018

Switch Named to represent the name using Cow<'static, str> instead of &'static str so that we can use Named for dynamically-generated names and names loaded from data files. The motivator here is that I would like to use Named for loading node names from glTF files, but it would have to support storing the names as String. Using Cow<'static, str> allows us to store String names without requiring an allocation if the users simply wants to use a string constant for the name.

I also added more documentation for Named, including a bunch of examples.


This change is Reviewable

randomPoison added some commits Aug 22, 2018

Use Cow<'static, str> in Named
Also add more documentation for `Named`.
@OvermindDL1

This comment has been minimized.

Copy link

commented Aug 22, 2018

Just as a note, in my C++ system I kept my 'named' style components as having a built in array of 16 to allow for a maximum of 16 character names (though I always kept it to 15 or less so I could encode a null and I never tested that I covered all 16-length cases so eh). This meant I could serialize out the entire storage for it (I used a single-allocation set with gravestones) with a single memcpy call. I always tried to keep all my storage tables as serializeable with single memcpy calls and overall I was pretty successful in 95% of cases.

@Moxinilian
Copy link
Member

left a comment

Excellent usage of Cow.

@Moxinilian

This comment has been minimized.

Copy link
Member

commented Aug 22, 2018

@OvermindDL1 Why would you want to serialize the named storage in an extremely fast manner like that?
Also, the 16 characters limitation isn't so great.

@OvermindDL1

This comment has been minimized.

Copy link

commented Aug 22, 2018

Not just the named storage, all storage's possible.

And I always found 16 fine, enough to encode some unique entity names, plus it keeps it short enough to keep the editor display's short and keep them easily typeable.

The purpose of fast serialization was for initial network transfers and save/load. I had near imperceptable save/load times even with hundreds of thousands of entities with a couple hundred component types. :-)

@randomPoison

This comment has been minimized.

Copy link
Member Author

commented Aug 22, 2018

It's worth noting that Unity's ECS doesn't allow allocations in components for effectively the same reason. I don't think such a restriction makes a ton of sense for Amethyst at this point, but having no indirection in component data is a useful property when you're trying to maximize performance.

@Moxinilian
Copy link
Member

left a comment

This might need more discussion then.

@Moxinilian
Copy link
Member

left a comment

Okay that's how you cancel a review.

@Moxinilian

This comment has been minimized.

Copy link
Member

commented Aug 22, 2018

I think having a constant size pre-allocated string might indeed be a better solution.
The networking will indeed need to serialize fast, so we can't just use allocation like this.

@jojolepro
Copy link
Member

left a comment

Perfect! Thanks a lot!

@OvermindDL1

This comment has been minimized.

Copy link

commented Aug 22, 2018

Approved now? I don't see any changes? o.O

@Moxinilian

This comment has been minimized.

Copy link
Member

commented Aug 22, 2018

We decided that if it becomes a problem in the future, it will be easy to change.

@randomPoison

This comment has been minimized.

Copy link
Member Author

commented Aug 22, 2018

@OvermindDL1 Ah sorry, I should clarify that I chatted briefly about this on Discord with @Moxinilian. The concerns I brought up were:

  • It's better to use Cow<'static, str> as a first pass for usability.
  • We don't yet know how Named is going to be used, so trying to optimize it at this point is a bit premature.
  • We don't yet know what Amethyst's networking will look like, so we don't yet know if it's even necessary to regularly serialize all names (i.e. it may well be the case that you only send names infrequently if ever).
  • @jojolepro also brought up the point that Named is meant primarily for debugging purposes, so performance is less of a concern than usability.

He agreed to go with Cow<'static, str> for now, though I think it would be useful to discuss alternatives in chat or in an issue here 😄

@OvermindDL1

This comment has been minimized.

Copy link

commented Aug 22, 2018

Heh, well let me just remain as an oppose. I highly am against random allocations of tiny chunks of data like that with indirections scattered around that destroy the ability to memcpy (or whatever Rust's equivalent is, I still need to use it more...). For an efficient ECS with the ability to save/load quickly being able to memcpy entire tables is beyond useful over almost anything else. The only things that I don't both enforcing memcpy in on my C++ version are things that are @Transiant and not ever serialized like active asset handles or the scene octree or so. Being able to serialize, quite literally, 500k (that's 500,000 or 500.000 depending on your localization) entities with over 200 distinct serialized table types in <1second is invaluable. Especially when you are auto-saving once a minute or so.

@jojolepro

This comment has been minimized.

Copy link
Member

commented Aug 22, 2018

We can't really force everyone to limit their entity names to 16 characters, and prevent them from loading those from files. Its just too much of a usability limitation for the sake of performance. If we want we can even strip out Named in release build, if they are not being used as identifiers (which they probably shouldn't, except in rare cases)

@OvermindDL1

This comment has been minimized.

Copy link

commented Aug 22, 2018

In my engine I tended to 'name' things like Player0/Player1/etc... for quick access, in addition to any special links between things and more. I used it quite excessively in production.

@Xaeroxe

This comment has been minimized.

Copy link
Member

commented Aug 22, 2018

Data indirection is a necessary evil for a flexible powerful engine imo. We try and reduce allocations as much as we can, but to say none at all is far too restrictive.

@Xaeroxe
Copy link
Member

left a comment

Thanks!

@OvermindDL1

This comment has been minimized.

Copy link

commented Aug 22, 2018

It was never too much for me, and I had indirection in many places (especially with the scripting related systems, but in general I tried to always keep tables as PoD, no pointers, handles sure, but no pointers.

@Xaeroxe

This comment has been minimized.

Copy link
Member

commented Aug 22, 2018

Seems like premature optimization to be honest. I'd have just used a String and called it good personally.

If we really want there to be no pointers in components we'd have to completely overhaul several systems. We're more likely to introduce overhead than to eliminate it. Of course we only use these things when we have no better alternative but requiring a fixed size for all data types just gets out of control in a hurry.

@jojolepro

This comment has been minimized.

Copy link
Member

commented Aug 23, 2018

Thanks!
bors r+

bors bot added a commit that referenced this pull request Aug 23, 2018

Merge #896
896: Use Cow<'static, str> in Named r=jojolepro a=randomPoison

Switch `Named` to represent the name using `Cow<'static, str>` instead of `&'static str` so that we can use `Named` for dynamically-generated names and names loaded from data files. The motivator here is that I would like to use `Named` for loading node names from glTF files, but it would have to support storing the names as `String`. Using `Cow<'static, str>` allows us to store `String` names without requiring an allocation if the users simply wants to use a string constant for the name.

I also added more documentation for `Named`, including a bunch of examples.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/amethyst/amethyst/896)
<!-- Reviewable:end -->


Co-authored-by: David LeGare <excaliburhissheath@gmail.com>
@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2018

@bors bors bot merged commit 2c28de1 into amethyst:develop Aug 23, 2018

3 checks passed

bors Build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@OvermindDL1

This comment has been minimized.

Copy link

commented Aug 27, 2018

We're more likely to introduce overhead than to eliminate it.

I'm curious as to what overhead you are referencing? I never experienced any inefficiency from it other than occasionally a handle indirect (into trivially cacheable memory), and even then that is when a pointer would have normally been needed anyway?

@Xaeroxe

This comment has been minimized.

Copy link
Member

commented Aug 27, 2018

Mostly RAM usage. Our best example of where we really need this is in the 3D animation systems. A rig can have a wide variety of complexity, maybe for one entity we only need 5 joints but for another we need 70 joints.

If we're not allowed to use pointers that means we lose access to Vecs. So our only way around this is a fixed allocation for the rig asset. That means all of our entities now need storage for 100 joints when really we're only using the first 5. All in the name of preventing a pointer. Of course there are ways to optimize this, but most of them more or less involve building your own memory allocator for memory you've already requested from the OS.

Could work, but is likely to make the application a RAM hog. We also haven't encountered a scenario that proves this kind of optimization and expenditure of developer time is necessary.

@OvermindDL1

This comment has been minimized.

Copy link

commented Aug 27, 2018

Visual things like that I would never serialize, but rather build them back up based on other data (like what determines how they should be positioned and their timestep and so forth).

When I speak of things being POD for ease of memcpy'ing, I mean things that only need to be serialized, either via save/load or over a network, and I never ever serialized an animation state.

However, just playing devils advocate, if you wanted to serialize something like that, think a set of tag's on an object, which could in most cases be 0 in length, or could be 400 or whatever in between), that is when I use either a map structure designed for single allocation (C++ has quite a variety of choose from with different trade-offs), or a Handle style mapping from the entity ID's to a large allocated array of all values that index into parts of itm skipping and leaving marked gravestones when an entry increases until either a kind of GC sweep runs over it based on how fragmented it becomes, or they get reused by something small enough to fit within (a simple Bag style list of unused ranges is nice and fast for that).

But still the point stands, for ~15 years of the life of my old engine I rarely ever have required anything more fancy than POD memcpy for serialization of component tables (and the few that I couldn't was generally because of interaction with external libraries that I required just to be able to build back up the mapping state, usually physics engines and such annoyances), without large amounts of RAM usage and with near as-perfect-as-I-could-reasonably-get-it efficiency. :-)

@randomPoison randomPoison deleted the randomPoison:named-cow branch Aug 28, 2018

@karroffel

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2018

Nobody ever said no allocations, what was mentioned though are handles. Your animation system example nicely shows that it actually is totally doable without references and pointers.

Say you have something like this

struct Skeleton {
    bone_parents: Vec<i32>;
    bone_transforms: Vec<Transform>;
}

struct ResourceServer {
    // many fields here..
    skeletons: Slab<Skeleton>;
}

struct SkeletonHandle(i32);

impl ResourceServer {
    fn skeleton_create(&mut self) -> SkeletonHandle {
        let idx = self.skeletons.insert(Skeleton::new());
        // do stuff with new skeleton
        SkeletonHandle(idx)
    }

    fn skeleton_set_bone_pose(&self, handle: SkeletonHandle, bone_idx: i32, pose: Transform) {
        if !self.skeletons.contains(handle.0) { panic!("at the disco"); }
        let skeleton = &mut self.skeletons[handle.0];
        skeleton.bone_transforms[bone_idx] = pose;
    }
}

Then whenever someone has a SkeletonHandle in a component, it is safe to "memcpy" the component without doing any further lifetime management or any new allocations etc.

@OvermindDL1

This comment has been minimized.

Copy link

commented Aug 28, 2018

Yes the component itself is, but now the state data for the given skeleton is stored outside of the component table and is not memcpy'able, and if that information is required to be serialized/deserialized then that ability is lost regardless.

@OvermindDL1

This comment has been minimized.

Copy link

commented Aug 28, 2018

The important aspect is for all data that needs to be serialized (for whatever reason, save/load, network, script marshalling, etc...) should be serializeable as efficiently and quickly as possible, which is insanely important on things like phones for save/load, preventing hitches in the game for a split second every 2 minutes or so when autosaving, for serializing up the game state for a new network connection, etc... etc...

So it dosen't matter that the component itself is memcpy'able if the data it is referencing is still not. All non-transient game state data needs be as efficiently serializeable as possible.

@jojolepro

This comment has been minimized.

Copy link
Member

commented Aug 28, 2018

Or you can just memcopy the data in ram and write it in a separate thread. :)
(and not save everything at once if necessary)

@OvermindDL1

This comment has been minimized.

Copy link

commented Aug 28, 2018

Or you can just memcopy the data in ram and write it in a separate thread. :)
(and not save everything at once if necessary)

That is indeed what I generally do yep, but you still need to pause the game world to serialize it out so it doesn't change underneath you, which a memcpy allows for.

And no you don't have to save everything at once, but you still need a consistent snapshot of the world at the time of saving so it still has to be frozen long enough for the serialization to happen, whether that's a quick memcpy (that could then be written out on another thread) or whether it's a slow pointer-hopping tree-walking serialization horror, that part has to complete before the game world can continue.

@Xaeroxe

This comment has been minimized.

Copy link
Member

commented Aug 28, 2018

I made the following:

use std::time::Instant;

const SIZE: usize = 100000;

fn main() {
    {
        let data_a = vec!["abcdefghijklmnopqrstuvwxyz".to_string(); SIZE];
        let mut data_b = Vec::<String>::with_capacity(SIZE);
        println!("Starting.");
        let start = Instant::now();
        data_b.extend(data_a.iter().cloned());
        let duration = start.elapsed();
        println!("Duration: {:?}", duration);
    }
    {
        let mut data_a = String::with_capacity(SIZE * 26);
        for _ in 0..SIZE {
            data_a.push_str("abcdefghijklmnopqrstuvwxyz");
        }
        let mut data_b = String::with_capacity(SIZE * 26);
        println!("Starting.");
        let start = Instant::now();
        data_b.push_str(&data_a);
        let duration = start.elapsed();
        println!("Duration: {:?}", duration);
    }
}

In order to determine just how much following pointers was hurting us. And to be honest, the answer is, quite a lot. I've run this a few times on my machine and the difference was within 1-3 orders of magnitude. Now granted with serializing 100000 strings even the worst option only takes 12 ms. However, in a 60 FPS game that's still about 75% of your frame time. So there we have it. Proof that avoiding pointers for components we want to serialize might actually be worth our time.

@Xaeroxe

This comment has been minimized.

Copy link
Member

commented Aug 28, 2018

I also made this version:

{
        let data_a = vec!["abcdefghijklmnopqrstuvwxyz".to_string(); SIZE];
        let mut data_b = vec![String::with_capacity(26); SIZE];
        println!("Starting.");
        let start = Instant::now();
        for i in 0..SIZE {
            data_b[i].push_str(&data_a[i]);
        }
        let duration = start.elapsed();
        println!("Duration: {:?}", duration);
    }

I thought this would give an advantage to the pointers version. It helped, but there's still at least one order of magnitude difference.

@Xaeroxe

This comment has been minimized.

Copy link
Member

commented Aug 28, 2018

With this in mind I propose we use this crate for the name component: https://crates.io/crates/inlinable_string

@randomPoison

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2018

Hmmmm, I agree we should strive to avoid allocations in components we want to serialize (especially if we're going to be serializing 100,000 of them in a frame), but I don't know that we necessarily should be targeting Named for that use case, at least not yet. We still don't get a good sense of how Named is going to be used (@jojolepro has argued that it be used purely for debugging purposes), so optimizing it for a use case that we don't currently have seems premature to me. I'm also worried about the general case of how we avoid allocation in components that need to be serialized; I don't expect storing data inline is always going to be a viable solution, so I think we'd be better served exploring other approaches before we try to optimize Named any further.

@Xaeroxe

This comment has been minimized.

Copy link
Member

commented Aug 28, 2018

Oh wait a minute. I just thought of something. We aren't using String, we're using Cow.

    {
        let data_a = vec![Cow::from("abcdefghijklmnopqrstuvwxyz"); SIZE];
        let mut data_b = Vec::with_capacity(SIZE);
        println!("Starting.");
        let start = Instant::now();
        data_b.extend(data_a.iter().cloned());
        let duration = start.elapsed();
        println!("Duration: {:?}", duration);
    }

This is just as fast as the inline data version, because a Cow clone doesn't actually do a mem copy. And, names rarely change.

@randomPoison

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2018

Well, that's true so long as all your names are created from string literals. But if you were to serialize all those string literal names and send them to a remote client, it would have to deserialize all the names as Strings. Whether or not that actually matters depends on if that client will then need to serialize the names again, hence why I feel like we really need some more concrete use cases to inform how we try to optimize serialization.

@OvermindDL1

This comment has been minimized.

Copy link

commented Aug 28, 2018

Yeah my strings definitely would not always be literals, plus that's still following a pointer. ^.^;

@Xaeroxe Thanks for those tests though! I was curious if Rust was able to help the pointer indirection much but it seems to be about as identical as I experienced in C++ a decade and a half ago when I made my requirement for everything possible to be memcpy'able. :-)

@OvermindDL1

This comment has been minimized.

Copy link

commented Aug 28, 2018

As an aside, I do want to point out again that for at least this specific component if I re-made it in my C++ engine nowadays I would use my Atom type, it's a 64-bit integer that can pack a string long enough to be descriptive (10 chars for case-sensitive or 12 chars for case insensitive, or another character or two if you constrain the chars further and change the encoding), entirely reverseable, and can happen at compile or run time very cheaply, think of them as a global enumeration, every string of the max length maps to a valid enumeration and vice-versa, which is fantastic for naming things for either debugging or level identification but they remain super fast and easy to reason about. I've not seen an atom-like type in Rust yet but a library for it would be awesome (there are a few variations of it in C++, but mine was highly customized for my use-case as I wanted perfect reverseability and not just simple compile-time collideable hashing).

@jojolepro

This comment has been minimized.

Copy link
Member

commented Aug 29, 2018

@Xaeroxe wouldn't your first test actually cause a reallocation of the String internal vec a couple of times also, since its basically appending one string slice at a time? I checked the rust std source and it seems to just foreach over a vec.extend_from_slice.

@Xaeroxe

This comment has been minimized.

Copy link
Member

commented Aug 29, 2018

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.