[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Issue 5344: Avoid repeated calculation of accepted contexts (issue 3

From: nine . fierce . ballads
Subject: Re: Issue 5344: Avoid repeated calculation of accepted contexts (issue 346080043 by address@hidden)
Date: Thu, 14 Jun 2018 19:36:40 -0700

Reviewers: dak,

Thank you for your review.  I disagree with some of your conclusions.
File lily/ (right):
lily/ 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.
lily/ 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
definitions will be done in where duplication is not
an issue
because it has been taken care of manually.

Seeing that 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.
File lily/ (right):
lily/ 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.

\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:  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.


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

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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]