Mutability of Layout::glyphs()

[Milias]

Hello,

Thank you for your work on this crate, it's really appreciated! I have been using for a hobby project for the past few weeks and it suits my needs perfectly.

I have a small improvement suggestion regarding the method Layout::glyphs(&'a mut self). At the moment, as the function call clearly shows, it requires a mutable reference to the layout. In my use case I store these pre-processed layouts in a HashMap for text caching and then obtain the GlyphPositions by calling this method on the stored layouts.

Since they have been already created, I do not modify them any more, which means that the mutable reference is wholly unnecessary. Would you consider to include two versions of this method? One immutable and one mutable. I believe this is a similar approach to what HashMap does with HashMap::get and HashMap::get_mut.

If I understand from the implementation, the point of having a &mut self is to lazily compute the glyphs's layout. I am not sure what would be the best approach to solve the inability to perform this step in the immutable version. Maybe return an Option<&Vec<GlyphPosition>> that is None if self.glyphs.len() != self.output.len()? You can probably decide better what fits best.

Thank you again for taking your time reading this and working on the crate!

PS. Could you explain why the need for the block unsafe { self.output.set_len(0) } in this method? I'm still fairly new to Rust and I don't see the reason not to use Vec::clear instead, especially since in the next line you call self.output.reserve(self.glyphs.len()).

[mooman219]

Context: So the implementation is lazy because appending text invalidates a lot of state. Recomputing the minimum amount of state was the goal of the API since performance was the theme. The append operation encapsulates all the state that isn't invalided, and the lazy evaluation in glyphs contains all of the state that is invalidated. The glyphs operation implicitly does this because that felt like the right api, since the boilerplate you would have to write would do the same thing.

Answer: I really rather you clone the output of glyphs since that's guaranteed to work. Because the underlying types are copy, a clone is very likely to just be a memcpy which is pretty fast.

Future: It'll probably be more ergonomic for me to just bundle everything up in append and be smarter with the state at the cost of some performance. It's something I'm thinking about.

PS: Clear is fine and you should just use clear, I'm just playing around to get more favorable compiler output in a micro benchmark which doesn't matter. set_len is sound here because the types involved are copy (and also not drop). The compiler can probably figure this out.

[Milias]

Thank you very much for the in-depth explanation!

I will investigate how cloning would work in my use case, since the limitation is that I need a mutable reference to the layout to begin with. The main reason I had in mind against this option (at least originally) was to avoid an unnecessary allocation, however, I don't know how much that would actually affect performance. As you said, indeed the underlying types should make it pretty fast.

At the moment, as a hack, I defined a glyph2 (I know, great name) function that looks like this:

pub fn glyphs2(&self) -> Option<&Vec<GlyphPosition<U>>> {
    if self.glyphs.len() == self.output.len() {
        Some(&self.output)
    } else {
        None
    }
}

For my use case this seems to do the trick, but I'll be paying close attention to your future updates!