Re: Issue 5344: Avoid repeated calculation of accepted contexts (issue 346080043 by address@hidden)

[nine . fierce . ballads]

Reviewers: dak,

Message:
Thank you for your review.  I disagree with some of your conclusions.


https://codereview.appspot.com/346080043/diff/1/lily/acceptance-set.cc
File lily/acceptance-set.cc (right):

https://codereview.appspot.com/346080043/diff/1/lily/acceptance-set.cc#newcode34
lily/acceptance-set.cc:34: if (!scm_is_null (default_))
On 2018/06/14 22:53:58, dak wrote:
> Why do you prioritize the default child?

I don't think I can take credit for that, though maybe I can take blame
for misunderstanding Context_def::get_accepted in the old code.  Doesn't
it place the default child at the front of the list that
Context_def::internal_path_to_acceptable_context () considers?  That
would make the default child the preferred alternative when \context
searches for an instantiable child.  At least, that is how I read it.

https://codereview.appspot.com/346080043/diff/1/lily/acceptance-set.cc#newcode50
lily/acceptance-set.cc:50: accepted_ = scm_cons(item, scm_delete_x
(item, accepted_));
On 2018/06/14 22:53:58, dak wrote:
> I'm not overly happy with this kind of O(n^2) behavior.  Most of the
context
> definitions will be done in engraver-init.ly where duplication is not
an issue
> because it has been taken care of manually.

Seeing that engraver-init.ly had at most a couple dozen accepts in a
context definition, my gut feeling was that it would be better to
default to cleanliness rather than worry about scaling, but I have no
problem with going back to allowing duplicate entries and mentioning
it in a comment to discourage the next person who comes along.

https://codereview.appspot.com/346080043/diff/1/lily/context-def.cc
File lily/context-def.cc (right):

https://codereview.appspot.com/346080043/diff/1/lily/context-def.cc#newcode330
lily/context-def.cc:330: Acceptance_set& acc = context->acceptance_;
On 2018/06/14 22:53:58, dak wrote:
> You take the original list without copying and call

context->acceptance_ = acceptance_ (line 328) copies the acceptance set
from the Context_def to the Context and then the following code modifies
the Context's acceptance set.  I could use a named method instead of the
assignment operator to make this clearer.

> Previously,
> \accept/\denies/\defaultchild for better or worse were not interpreted
in \with
> blocks (which is where ops comes from if I remember correctly).

\accepts was. \denies and \defaultchild were not until I added them a
couple weeks ago: https://codereview.appspot.com/346050043/  If any of
that is a problem, let me know and I will prepare a patch to revert what
you please; I don't understand why it would be a problem, though.

Anyway, in the old version of Context::path_to_acceptable_context, these
mods were explicitly looped over, converted from strings to symbols,
passed into Context_def::path_to_acceptable_context and merged into a
fresh copy of the Context_def's list of accepted contexts.  This was
repeated for every call to Context::path_to_acceptable_context, which
occurred potentially multiple times during a find-or-create walk of the
context hierarchy.

In the new code, the Context's acceptance list is computed here, once,
when the context is instantiated.

> lets \with blocks destructively change global context defs.

I do not think this is the case, but I will verify it.

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

Encapsulate the list of accepted contexts into Acceptance_set, which
is owned by both Context_def and Context.  The merging of context mods
that used to occur anew for every path_to_acceptable_context () call
now occurs once when a context is instantiated.

The aspect of this change most in need of review is probably garbage
collection.  Thanks in advance.

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

Affected files (+188, -125 lines):
  A lily/acceptance-set.cc
  M lily/context.cc
  M lily/context-def.cc
  M lily/global-context.cc
  A lily/include/acceptance-set.hh
  M lily/include/context.hh
  M lily/include/context-def.hh