Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1047)

Issue 359770043: Issue 5368: Reduce allocations in Grob dimension caching (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 month, 2 weeks ago by Dan Eble
Modified:
1 month ago
Reviewers:
dak, haberg-1
CC:
lilypond-devel_gnu.org
Visibility:
Public.

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.

Patch Set 1 #

Total comments: 2

Patch Set 2 : Hide Optional in Dimension_cache and pare it down #

Total comments: 13
Unified diffs Side-by-side diffs Delta from patch set Stats (+61 lines, -98 lines) Patch
D lily/dimension-cache.cc View 1 chunk +0 lines, -68 lines 0 comments Download
M lily/grob.cc View 1 8 chunks +16 lines, -20 lines 6 comments Download
M lily/include/dimension-cache.hh View 1 1 chunk +44 lines, -9 lines 5 comments Download
M lily/include/grob.hh View 1 chunk +1 line, -1 line 2 comments Download

Messages

Total messages: 13
dak
Overall, I consider the problem you are trying to address here real: allocation of the ...
1 month, 2 weeks ago (2018-07-05 12:20:27 UTC) #1
Dan Eble
On 2018/07/05 12:20:27, dak wrote: > I'd like to see some rationale for the amount ...
1 month, 1 week ago (2018-07-05 21:32:25 UTC) #2
dak
On 2018/07/05 21:32:25, Dan Eble wrote: > On 2018/07/05 12:20:27, dak wrote: > > I'd ...
1 month, 1 week ago (2018-07-06 09:12:32 UTC) #3
Dan Eble
On 2018/07/06 09:12:32, dak wrote: > On 2018/07/05 21:32:25, Dan Eble wrote: > > On ...
1 month, 1 week ago (2018-07-06 20:21:50 UTC) #4
haberg-1_telia.com
> On 6 Jul 2018, at 11:12, dak@gnu.org wrote: > > On 2018/07/05 21:32:25, Dan ...
1 month, 1 week ago (2018-07-06 22:02:54 UTC) #5
Dan Eble
On Jul 6, 2018, at 18:02, Hans Åberg <haberg-1@telia.com> wrote: One can do it the ...
1 month, 1 week ago (2018-07-06 22:13:17 UTC) #6
Dan Eble
Hide Optional in Dimension_cache and pare it down
1 month, 1 week ago (2018-07-06 22:16:09 UTC) #7
haberg-1_telia.com
> On 7 Jul 2018, at 00:13, nine.fierce.ballads@gmail.com wrote: > > On Jul 6, 2018, ...
1 month, 1 week ago (2018-07-06 22:19:29 UTC) #8
Dan Eble
On 2018/07/06 22:19:29, haberg-1_telia.com wrote: > The technique is useful for transforming code, though. Into ...
1 month, 1 week ago (2018-07-06 22:26:35 UTC) #9
dak
Hans Åberg <haberg-1@telia.com> writes: >> On 6 Jul 2018, at 11:12, dak@gnu.org wrote: >> >> ...
1 month, 1 week ago (2018-07-07 07:35:11 UTC) #10
dak
LGTM https://codereview.appspot.com/359770043/diff/20001/lily/grob.cc File lily/grob.cc (left): https://codereview.appspot.com/359770043/diff/20001/lily/grob.cc#oldcode325 lily/grob.cc:325: *dim_cache_[a].offset_ += y; This is more an "as ...
1 month, 1 week ago (2018-07-07 11:06:11 UTC) #11
Dan Eble
https://codereview.appspot.com/359770043/diff/20001/lily/grob.cc File lily/grob.cc (left): https://codereview.appspot.com/359770043/diff/20001/lily/grob.cc#oldcode325 lily/grob.cc:325: *dim_cache_[a].offset_ += y; On 2018/07/07 11:06:11, dak wrote: > ...
1 month, 1 week ago (2018-07-07 14:48:27 UTC) #12
dak
1 month, 1 week ago (2018-07-07 15:13:19 UTC) #13
Still LGTM.

https://codereview.appspot.com/359770043/diff/20001/lily/grob.cc
File lily/grob.cc (left):

https://codereview.appspot.com/359770043/diff/20001/lily/grob.cc#oldcode325
lily/grob.cc:325: *dim_cache_[a].offset_ += y;
On 2018/07/07 14:48:27, Dan Eble wrote:
> On 2018/07/07 11:06:11, dak wrote:
> > This is more an "as you like it" suggestion: operator priority of * as
opposed
> > to [] . -> is not always clear to everybody, so *(dim_cache_[a].offset_) +=
y;
> > might be a consideration.  On the other hand, it's ugly.  And it's not like
> 
> I would have no problem adding parentheses, but how do you rate the beauty of
> using value_or() here as in Patch Set 1?

Reads clumsy to me, though it's basically the same as our robust_scm2* .  Let's
just leave this one alone for now.

https://codereview.appspot.com/359770043/diff/20001/lily/grob.cc
File lily/grob.cc (right):

https://codereview.appspot.com/359770043/diff/20001/lily/grob.cc#newcode322
lily/grob.cc:322: if (!dim_cache_[a].offset_.has_value ())
On 2018/07/07 14:48:27, Dan Eble wrote:

> Well, even bool x = 0.0 would cause me to question the state of mind of the
> programmer.

lily/bar-check-iterator.cc:      if (where->main_part_)

Pretty much the same.  We are not using all that many inexact numbers in a
similar manner I guess but numerics used as conditions are not that infrequent
although it's admittedly more integers (like size ()) used in that manner.

https://codereview.appspot.com/359770043/diff/20001/lily/include/dimension-ca...
File lily/include/dimension-cache.hh (right):

https://codereview.appspot.com/359770043/diff/20001/lily/include/dimension-ca...
lily/include/dimension-cache.hh:72: void clear ()
On 2018/07/07 14:48:27, Dan Eble wrote:

> It was already called clear () and I was trying not to change more than I had
> to.
> 
> In this case I don't really like either of those names

So just not wanting to condone either and not touching anything.

Ok with me.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b