Re: Issue 5368: Reduce allocations in Grob dimension caching (issue 359770043 by address@hidden)

[nine . fierce . ballads]

Reviewers: dak,

Message:
On 2018/07/05 12:20:27, dak wrote:
> I'd like to see some rationale for the amount of semi-general-purpose
tooling to
> get there, and more current or future uses of it may go a long way
towards that.

The rationale is that std::optional is fit for this situation and if
LilyPond were built with C++17 I would simply have used it.  The
interface of this Optional is a subset of std::optional, so when the
project leaders determine that it is reasonable to require C++17 to
build LilyPond, discarding this Optional should be straightforward.

> flower/include/optional.hh:30: // type at all times, even when it
presents
> itself as empty.
> The way against that would be to have
> char [sizeof (T)] value_space_;
> and instantiate and destroy it with placement new syntax, right?

Yes, though I haven't studied any existing implementations.  There might
be concerns related to alignment which either require features of newer
C++ or implementation-defined behavior (unions come to mind).  I'm sure
you can imagine my reluctance to try to reproduce that feature of
std::optional without a unit testing framework or routine uses in the
LilyPond code.  If there were a current use case and we could be
reasonably sure that implementation errors would not go undiscovered, I
wouldn't be opposed to looking into the GNU implementation of
std::optional to see what could be borrowed; but I'm not very interested
in spending that time strictly in anticipation of other uses.  I have no
passion to work on Optional for its own sake; it's just that "new Real"
caught my attention and I couldn't let it stand.


https://codereview.appspot.com/359770043/diff/1/flower/include/optional.hh#newcode40
> flower/include/optional.hh:40: typedef const T Optional::*bool_type;
> I don't think we should be dragging an idiom like that into a single
> class/template like this.

> In particular when it comes with additional baggage like the
comparison operator
> business in order to actually work as intended.

This could be dealt with by throwing out the implicit conversion to bool
and implementing has_value() instead.  It would still be a subset of the
interface of std::optional.  Example:

    if (dim_cache_[a].offset_.has_value ())
      return *dim_cache_[a].offset_;


Description:
https://sourceforge.net/p/testlilyissues/issues/5368/

Reduce allocations in Grob dimension caching
Add Optional<T> offering some of the features of C++17 std::optional.

For those who are not familiar with std::optional yet, the most useful
fact to know is that you assign directly to it like a value, but you
read from it with pointer syntax.  I didn't invent it, but go ahead and
blame me for using it, if you feel the urge.

I profiled input/regression/rest-dot-position.ly with callgrind,
collecting data during Book::process, and found that these changes
reduce cycles attributed to malloc and free to about 90% of what they
were.  They reduce overall cycles attributed to Book::process to 99% of
what they were.

I don't know how the use of malloc and free scales in larger scores.

Please review this at https://codereview.appspot.com/359770043/

Affected files (+125, -95 lines):
  A flower/include/optional.hh
  D lily/dimension-cache.cc
  M lily/grob.cc
  M lily/include/dimension-cache.hh
  M lily/include/grob.hh