Re: Remove ly:grob-properties (issue 549840044 by hanw...@gmail.com)

[dak]

On 2020/04/11 17:44:11, hanwenn wrote:
> On 2020/04/11 17:19:19, dak wrote:
> > > I thought of removing the other function too, and I agree in
principle, but
> it
> > > can only break more user files. As long as we're not considering
> reorganizing
> > > the immutable lists, I think it can stay.
> > 
> > Note that there is no necessity of returning a bonafide part of the
grob
> > structure: one can construct a list of properties on the fly. 
Particularly
> > because this is used read-only and only occasionally.
> 
> I know. But if we make a copy, that will be expensive too.

It's either imprudent user code or code that actually potentially uses
all the properties.  Either way I'd not be worried about the cost.  That
we don't want to use it in the internals for regular use then is
obvious.

https://codereview.appspot.com/549840044/

Re: Remove ly:grob-properties (issue 549840044 by hanw...@gmail.com)

[hanwenn]

On 2020/04/11 17:19:19, dak wrote:
> > I thought of removing the other function too, and I agree in
principle, but it
> > can only break more user files. As long as we're not considering
reorganizing
> > the immutable lists, I think it can stay.
> 
> Note that there is no necessity of returning a bonafide part of the
grob
> structure: one can construct a list of properties on the fly. 
Particularly
> because this is used read-only and only occasionally.

I know. But if we make a copy, that will be expensive too.

Any comment on 

NOSUBMIT: this is an incompatible change; where are the release notes?


https://codereview.appspot.com/549840044/

Re: Remove ly:grob-properties (issue 549840044 by hanw...@gmail.com)

[dak]

On 2020/04/11 16:38:28, hanwenn wrote:
> On 2020/04/11 16:02:31, dak wrote:
> > The description says "[ly:grob-properties] will not return the
properties that
> > were \overridden."
> > 
> > Aren't you confusing this with ly:grob-basic-properties ?  I think
> > ly:grob-properties will actually _only_ return the properties that
were
> > overriden.
> 
> \override pushes data onto the immutable list (or at least, it used to
be like
> that).

In effect you are right (and I misremembered), but override pushes onto
context-maintained lists which are ultimately used as template when
make_grob gets called and _then_ the overriden properties end up on the
immutable list.

> I thought of removing the other function too, and I agree in
principle, but it
> can only break more user files. As long as we're not considering
reorganizing
> the immutable lists, I think it can stay.

Note that there is no necessity of returning a bonafide part of the grob
structure: one can construct a list of properties on the fly. 
Particularly because this is used read-only and only occasionally.

https://codereview.appspot.com/549840044/

Re: Remove ly:grob-properties (issue 549840044 by hanw...@gmail.com)

[thomasmorley65]

> Fixes
> Documentation/snippets/overriding-articulations-of-destinct-type,
> which should be mirrored back into LSR.

Fixed in LSR

https://codereview.appspot.com/549840044/

Re: Remove ly:grob-properties (issue 549840044 by hanw...@gmail.com)

[hanwenn]

Reviewers: dak, thomasmorley651,

Message:
On 2020/04/11 16:02:31, dak wrote:
> The description says "[ly:grob-properties] will not return the
properties that
> were \overridden."
> 
> Aren't you confusing this with ly:grob-basic-properties ?  I think
> ly:grob-properties will actually _only_ return the properties that
were
> overriden.

\override pushes data onto the immutable list (or at least, it used to
be like that). So if you do \override Stem.length = #5 , you won't see
'length in the return value of ly:grob-properties

I thought of removing the other function too, and I agree in principle,
but it can only break more user files. As long as we're not considering
reorganizing the immutable lists, I think it can stay.

Description:
Remove ly:grob-properties

This function is hard to use correctly:

* one cannot set the head of the alist, so it cannot be used for
  modifications.

* it will not return the properties that were \overridden

Instead, ly:grob-property should be used. 

This paves the way to further work on (grob) property storage
mechanisms.

Fixes
Documentation/snippets/overriding-articulations-of-destinct-type,
which should be mirrored back into LSR.

NOSUBMIT: this is an incompatible change; where are the release notes?

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

Affected files (+1, -13 lines):
  M Documentation/snippets/overriding-articulations-of-destinct-type.ly
  M lily/grob-scheme.cc


Index: Documentation/snippets/overriding-articulations-of-destinct-type.ly
diff --git 
a/Documentation/snippets/overriding-articulations-of-destinct-type.ly 
b/Documentation/snippets/overriding-articulations-of-destinct-type.ly
index 
5053be73b1161970f5d7cc47baf729fa92c096bb..525840d718a5f32f0ba4b15b66dc72b550305fbf
 100644
--- a/Documentation/snippets/overriding-articulations-of-destinct-type.ly
+++ b/Documentation/snippets/overriding-articulations-of-destinct-type.ly
@@ -28,7 +28,7 @@ With 2.16.2 it is possible to put the proposed function,
 #(define (custom-script-tweaks ls)
   (lambda (grob)
 (let* ((type (ly:prob-property
-(assoc-ref (ly:grob-properties grob) 'cause)
+(ly:grob-property grob 'cause)
 'articulation-type))
(tweaks (assoc-ref ls type)))
   (if tweaks
Index: lily/grob-scheme.cc
diff --git a/lily/grob-scheme.cc b/lily/grob-scheme.cc
index 
a078e7ac083d237169b39c15f8b39dc3293ee44b..22f89c87e8d48eb6e8db231a3066b2fcfeaddeca
 100644
--- a/lily/grob-scheme.cc
+++ b/lily/grob-scheme.cc
@@ -312,18 +312,6 @@ LY_DEFINE (ly_grob_set_parent_x, "ly:grob-set-parent!",
   return SCM_UNSPECIFIED;
 }
 
-LY_DEFINE (ly_grob_properties, "ly:grob-properties",
-   1, 0, 0, (SCM grob),
-   "Get the mutable properties of @var{grob}.")
-{
-  Grob *g = unsmob (grob);
-
-  LY_ASSERT_SMOB (Grob, grob, 1);
-
-  /* FIXME: uhg? copy/read only? */
-  return g->mutable_property_alist_;
-}
-
 LY_DEFINE (ly_grob_basic_properties, "ly:grob-basic-properties",
1, 0, 0, (SCM grob),
"Get the immutable properties of @var{grob}.")

Re: Remove ly:grob-properties (issue 549840044 by hanw...@gmail.com)

[dak]

On 2020/04/11 16:32:58, thomasmorley651 wrote:
> On 2020/04/11 16:31:48, thomasmorley651 wrote:
> 
> > Btw, it's used in
> > input/regression/multi-measure-rest-reminder.ly
> > input/regression/scheme-text-spanner.ly
> > as well.
> 
> No, that's ly:grob-properties?
> Bad grepping ...

Also icky name overlaps.  ly:grob-properties? was last.  Maybe it should
be ly:context-grob-properties? instead?  It's the thing contexts use for
tracking grob properties.

https://codereview.appspot.com/549840044/

Re: Remove ly:grob-properties (issue 549840044 by hanw...@gmail.com)

[thomasmorley65]

On 2020/04/11 16:31:48, thomasmorley651 wrote:

> Btw, it's used in
> input/regression/multi-measure-rest-reminder.ly
> input/regression/scheme-text-spanner.ly
> as well.

No, that's ly:grob-properties?
Bad grepping ...


https://codereview.appspot.com/549840044/

Re: Remove ly:grob-properties (issue 549840044 by hanw...@gmail.com)

[thomasmorley65]

The change in the snippet is ofcourse nice.

Though, please don't delete ly:grob-properties or
ly:grob-basic-properties.
I use them every day.
They are tools to get insights at the grob at hand.
Don't judge from our code base and the few occurencies there.
Btw, it's used in
input/regression/multi-measure-rest-reminder.ly
input/regression/scheme-text-spanner.ly
as well.

Again, it's in every days use!!!


https://codereview.appspot.com/549840044/diff/567430043/Documentation/snippets/overriding-articulations-of-destinct-type.ly
File Documentation/snippets/overriding-articulations-of-destinct-type.ly
(right):

https://codereview.appspot.com/549840044/diff/567430043/Documentation/snippets/overriding-articulations-of-destinct-type.ly#newcode31
Documentation/snippets/overriding-articulations-of-destinct-type.ly:31:
(ly:grob-property grob 'cause)
Yep, that's better here.

https://codereview.appspot.com/549840044/

Re: Simplify mf-to-table (issue 549840043 by hanw...@gmail.com)

[jonas . hahnfeld]

Thanks for the cleanup, a few nits inline.
(I assume the description wasn't updated according to your local commit
text?)


https://codereview.appspot.com/549840043/diff/553830043/scripts/build/mf-to-table.py
File scripts/build/mf-to-table.py (right):

https://codereview.appspot.com/549840043/diff/553830043/scripts/build/mf-to-table.py#newcode22
scripts/build/mf-to-table.py:22: import getopt
I think this import can be dropped

https://codereview.appspot.com/549840043/diff/553830043/scripts/build/mf-to-table.py#newcode165
scripts/build/mf-to-table.py:165: 
Regular expressions seem a bit overkill here. What about
root = os.path.splitext (name)
global_lisp_nm = root + '.global-lisp'
char_lisp_nm = root + '.lisp'

https://codereview.appspot.com/549840043/

Re: flower: Get rid of libc-extension (issue 553740043 by jonas.hahnf...@gmail.com)

[jonas . hahnfeld]

On 2020/04/11 15:59:32, hahnjo wrote:
> Rename my_round() to round_halfway_up()

As promised, the updated patch keeps the functionality. However I've
renamed the function and added clear documentation that it should not be
used in new code.

https://codereview.appspot.com/553740043/

Re: Remove ly:grob-properties (issue 549840044 by hanw...@gmail.com)

[dak]

The description says "[ly:grob-properties] will not return the
properties that were \overridden."

Aren't you confusing this with ly:grob-basic-properties ?  I think
ly:grob-properties will actually _only_ return the properties that were
overriden.

https://codereview.appspot.com/549840044/

Remove ly:grob-properties (issue 549840044 by hanw...@gmail.com)

[dak]

https://codereview.appspot.com/549840044/diff/567430043/lily/grob-scheme.cc
File lily/grob-scheme.cc (left):

https://codereview.appspot.com/549840044/diff/567430043/lily/grob-scheme.cc#oldcode327
lily/grob-scheme.cc:327: LY_DEFINE (ly_grob_basic_properties,
"ly:grob-basic-properties",
There is really no point in removing ly:grob-properties while retaining
ly:grob-basic-properties as they are a pair of accessors.

https://codereview.appspot.com/549840044/

Re: stale git branches

[Jonas Hahnfeld]

Am Samstag, den 11.04.2020, 14:48 +0200 schrieb Han-Wen Nienhuys:
> On Sat, Apr 11, 2020 at 2:13 PM Jonas Hahnfeld <
> hah...@hahnjo.de
> > wrote:
> > following removal of dev/translation-* branches, I took a closer look
> > at stale branches. I think it would make sense to keep unscoped
> > branches (outside of dev/user/) to a minimum. This should also avoid
> > overlooking old changes that have not been merged yet.
> > The following list is by no means complete, but maybe a good start:
> > 
> > dev/pango contains commits:
> > 53ed2b55e2 Add a RAII wrapper for extracting FT_Face from PangoFcFont
> > c93c477180 Make Pango >= 1.36 mandatory.
> > in master:
> > 9cf8d35e8c Add a RAII wrapper for extracting FT_Face from PangoFcFont
> > 15b7118410 Make Pango >= 1.36 mandatory.
> > I'm fairly certain the branch can be removed.
> 
> This can be removed. It was a staging area when David wanted to revert
> said patch from staging.

Ah, now I remember. Deleted (as David noted, a rebase resulted in empty
commits).


signature.asc
Description: This is a digitally signed message part

Re: Refactor get/set_property to take the item as first argument (issue 573670043 by d...@gnu.org)

[David Kastrup]

Han-Wen Nienhuys  writes:

> On Fri, Apr 10, 2020 at 1:07 PM David Kastrup  wrote:
>> > * memory use: each SCM value takes 8 bytes, and there are 416 grob
>> > properties today, for a total of 3328 bytes. Morgenlied is single page
>> > of music and has 3704 grobs. So storage for the vectors (which will be
>> > mostly empty) will take up 12M / page. This likely will result in
>> > doubling the memory use of LilyPond.
>>
>> You haven't understood the scheme.  A grob only contains the properties
>> seminal to its type.  That's why there is a double indirection.  A full
>> array of 416 entries is needed per grob _type_, not per grob.
>
> I still don't see why this requires an overall overhaul of the
> internal calling convention. You can have a global registry of symbol
> => uint16 (numbering all the symbols),

It would be pointless to have a single registry for music properties,
grob properties, stream event properties, and context properties, thus
significantly blowing up the second-level indexing tables and making for
a large number of cache misses.

> memoize that uint16 like we memoize the symbol today (I think this is
> what you suggested already).  Then use a uint16 => value hashtable in
> the grob/prob/etc. A custom hash implementation can save space by
> storing the uint16 key rather than the 8 byte SCM symbol.
>
> This would be a very conservative, localized change that doesn't
> require setting up machinery to convert types to vectors,

A vector is cheaper to index than a hashtable.  It makes no sense
talking aobut "setting up machinery" with regard to a vector while
taking hash tables for granted.

> and it keeps working if grob types change which grob-interfaces they
> support halfway through.

I don't think there is any point in discussing this patch based on what
you surmise about an ultimate solution.

> There are many types (stem, axis-group, beam, hairpin) that properties
> that are mostly unset. A fixed encoding will waste CPU cache and main
> memory to allow these properties to be set even if it is not
> necessary, so aside from increased complexity, it's not obvious that
> there will be a performance gain.

We can discuss this until the cows go home, but the authoritive answer
will be benchmarks.  They may indicate the necessity for further work,
and doing further work is facilitated by not having to track any added
C++ code accessing properties.  Hence this patch.

I actually expect more gains to be realised by rewriting
break-substitution.cc rather than the actual property access, but having
property access made cheap is certainly not undesirable either.

>> Hashing outside of a closed set still requires storing the actual symbol
>> for corroboration.  Hashing inside of a closed set amounts to the
>> first-stage lookup in my scheme, except that a small number guaranteed
>> to be unique comes out.  This lookup can be memoised and will work
>> permanently as opposed to hashing in a growing set.
>>
>> > We could experiment with this without requiring a giant rewrite of the
>> > source code.
>>
>> What you call a "giant rewrite" is basically this patch.  It's a purely
>> syntactical patch only extending rather than reducing possible
>> approaches and perfectly compatible with different implementations.  The
>> work for that patch is already done, the impact is similar to what
>> happened when changing around the syntax of unsmob a few times ian the
>> past.  It will allow for prolonged work on possibly different
>> implementations without having to constantly deal with merge conflicts.
>>
>> It does not depend on picking a particular implementation.  You
>> expressed interest in what I was planning to do with the opportunity
>> this patch opens, I answered.  I do not, however, see the point in
>> discussing and dissing (partially) unwritten code based on an
>> incomplete understanding in a handwaving discussion.
>
> I think it is not out of the ordinary to discuss plans invasive plans
> before implementing them.

But we are not discussing an invasive plan here.  This here is an
extensive, not an invasive change.  There is no point in dissing
unwritten code that take no project-wide resources.  The point of time
to review code is after it is proposed.

> I think this change is invasive because it changes the idiom across
> the source code, and I disagree that this change is net neutral. C++
> method calls are a more natural idiom plain function calls.

We are not talking about C++ method calls here.  We are talking about
macro calls.  Memoisation of any kind can only be implemented at the
macro call level when diversifying on a string literal since those are
not available as template arguments, otherwise one could transfer the
job to template specialisation like it is done in a few other places.
So the macro needs information to work with.  This change makes what
actually happens more rather than less transparent and idiomatic.

> If we can get a significant performance boost, that may be 

Re: stale git branches

[Urs Liska]

Am 11. April 2020 15:33:06 MESZ schrieb David Kastrup :
>Jonas Hahnfeld  writes:
>
>> Hi all,
>>
>> following removal of dev/translation-* branches, I took a closer look
>> at stale branches. I think it would make sense to keep unscoped
>> branches (outside of dev/user/) to a minimum. This should also avoid
>> overlooking old changes that have not been merged yet.
>> The following list is by no means complete, but maybe a good start:
>>
>> dev/pango contains commits:
>> 53ed2b55e2 Add a RAII wrapper for extracting FT_Face from PangoFcFont
>> c93c477180 Make Pango >= 1.36 mandatory.
>> in master:
>> 9cf8d35e8c Add a RAII wrapper for extracting FT_Face from PangoFcFont
>> 15b7118410 Make Pango >= 1.36 mandatory.
>> I'm fairly certain the branch can be removed.
>
>git rebase origin origin/dev/pango
>
>ends up with no commit on top.  So yes.
>
>> Branches dev/issue3300,
>
>Mine, but actually issue 3330.  Removed.
>
>> dev/issue3330, dev/issue3648 are likely related
>> to the named issues which have status 'Verified'. AFAICS there are
>some
>> additional commits in the branches, could be due to review comments?
>> David, you are probably the best to judge if they are fully merged or
>> some changes could still be relevant, could you take a look?
>>
>> As far as I understand, master now also has the relevant commits from
>> dev/guile-v2-work, dev/guilev2, and dev/guilev21? Can those branches
>be
>> dropped to avoid possible confusion about the current status?
>
>Will followup on all those later.
>
>> Then there are some dev/user/ branches. I consider these relatively
>> "private" to that person and would not propose to delete them on a
>> global basis. Still maybe everyone can take a look and delete unused
>> personal branches on their own?
>
>origin/dev/rune may be considered an epitaph.  I don't think anybody
>ever attempted merging what this was about: maybe it's in the interest
>area of Hans Aberg.  Whether or not somebody does an assessment of it
>at
>one point of time, I think it appropriate to leave it as-is, including
>not rebasing/rewriting any of it in-place.
>
>I'll readily agree that there is a disconcerting large set of other
>apparently semi-dead branches by living people, most of them likely
>unaware of what they left lying there.  There may be some point in
>going
>through and mailing them about what they think best to do here.

I will look into what I have lying around.

Urs

-- 
Diese Nachricht wurde von meinem Android-Gerät mit K-9 Mail gesendet.

Re: stale git branches

[David Kastrup]

Jonas Hahnfeld  writes:

> Hi all,
>
> following removal of dev/translation-* branches, I took a closer look
> at stale branches. I think it would make sense to keep unscoped
> branches (outside of dev/user/) to a minimum. This should also avoid
> overlooking old changes that have not been merged yet.
> The following list is by no means complete, but maybe a good start:
>
> dev/pango contains commits:
> 53ed2b55e2 Add a RAII wrapper for extracting FT_Face from PangoFcFont
> c93c477180 Make Pango >= 1.36 mandatory.
> in master:
> 9cf8d35e8c Add a RAII wrapper for extracting FT_Face from PangoFcFont
> 15b7118410 Make Pango >= 1.36 mandatory.
> I'm fairly certain the branch can be removed.

git rebase origin origin/dev/pango

ends up with no commit on top.  So yes.

> Branches dev/issue3300,

Mine, but actually issue 3330.  Removed.

> dev/issue3330, dev/issue3648 are likely related
> to the named issues which have status 'Verified'. AFAICS there are some
> additional commits in the branches, could be due to review comments?
> David, you are probably the best to judge if they are fully merged or
> some changes could still be relevant, could you take a look?
>
> As far as I understand, master now also has the relevant commits from
> dev/guile-v2-work, dev/guilev2, and dev/guilev21? Can those branches be
> dropped to avoid possible confusion about the current status?

Will followup on all those later.

> Then there are some dev/user/ branches. I consider these relatively
> "private" to that person and would not propose to delete them on a
> global basis. Still maybe everyone can take a look and delete unused
> personal branches on their own?

origin/dev/rune may be considered an epitaph.  I don't think anybody
ever attempted merging what this was about: maybe it's in the interest
area of Hans Aberg.  Whether or not somebody does an assessment of it at
one point of time, I think it appropriate to leave it as-is, including
not rebasing/rewriting any of it in-place.

I'll readily agree that there is a disconcerting large set of other
apparently semi-dead branches by living people, most of them likely
unaware of what they left lying there.  There may be some point in going
through and mailing them about what they think best to do here.

-- 
David Kastrup

Re: Refactor get/set_property to take the item as first argument (issue 573670043 by d...@gnu.org)

[Han-Wen Nienhuys]

On Sat, Apr 11, 2020 at 2:53 PM Jonas Hahnfeld  wrote:
>
> Am Samstag, den 11.04.2020, 14:45 +0200 schrieb Han-Wen Nienhuys:
> > I think it is not out of the ordinary to discuss plans invasive plans
> > before implementing them.
>
> To be fair, David did ask about it two months ago:
> https://lists.gnu.org/archive/html/lilypond-devel/2020-02/msg00668.html
>

I did ask " I'm curious about your plans. Can you say more? "

My question was smushed into the quote in the archive, maybe because
of the vagaries of HTML vs. Text mail.

https://lists.gnu.org/archive/html/lilypond-devel/2020-02/msg00705.html

> (I don't understand this part of LilyPond, so I'll keep out of the
> technical discussion.)

--
Han-Wen Nienhuys - hanw...@gmail.com - http://www.xs4all.nl/~hanwen

Re: Refactor get/set_property to take the item as first argument (issue 573670043 by d...@gnu.org)

[Jonas Hahnfeld]

Am Samstag, den 11.04.2020, 14:45 +0200 schrieb Han-Wen Nienhuys:
> I think it is not out of the ordinary to discuss plans invasive plans
> before implementing them.

To be fair, David did ask about it two months ago:
https://lists.gnu.org/archive/html/lilypond-devel/2020-02/msg00668.html

(I don't understand this part of LilyPond, so I'll keep out of the
technical discussion.)


signature.asc
Description: This is a digitally signed message part

Re: Refactor get/set_property to take the item as first argument (issue 573670043 by d...@gnu.org)

[Han-Wen Nienhuys]

On Sat, Apr 11, 2020 at 2:45 PM Han-Wen Nienhuys  wrote:
> > What you call a "giant rewrite" is basically this patch.  It's a purely
> > syntactical patch only extending rather than reducing possible
> > approaches and perfectly compatible with different implementations.  The
> > work for that patch is already done, the impact is similar to what
> > happened when changing around the syntax of unsmob a few times ian the
> > past.  It will allow for prolonged work on possibly different
> > implementations without having to constantly deal with merge conflicts.
> >
> > It does not depend on picking a particular implementation.  You
> > expressed interest in what I was planning to do with the opportunity
> > this patch opens, I answered.  I do not, however, see the point in
> > discussing and dissing (partially) unwritten code based on an incomplete
> > understanding in a handwaving discussion.
>
> I think it is not out of the ordinary to discuss plans invasive plans
> before implementing them.  I think this change is invasive because it
> changes the idiom across the source code, and I disagree that this
> change is net neutral. C++ method calls are a more natural idiom plain
> function calls. If we can get a significant performance boost, that
> may be worth it, but I think we should first understand if that is the
> case.  As said (see above), I am skeptical.

Also, property access accounts for about 10% of runtime, last time I
looked. This can probably be reduced by some factor, but even if it
were infinitely fast, it is not going to make a huge difference to the
overall runtime.

-- 
Han-Wen Nienhuys - hanw...@gmail.com - http://www.xs4all.nl/~hanwen

Re: stale git branches

[Han-Wen Nienhuys]

On Sat, Apr 11, 2020 at 2:13 PM Jonas Hahnfeld  wrote:
> following removal of dev/translation-* branches, I took a closer look
> at stale branches. I think it would make sense to keep unscoped
> branches (outside of dev/user/) to a minimum. This should also avoid
> overlooking old changes that have not been merged yet.
> The following list is by no means complete, but maybe a good start:
>
> dev/pango contains commits:
> 53ed2b55e2 Add a RAII wrapper for extracting FT_Face from PangoFcFont
> c93c477180 Make Pango >= 1.36 mandatory.
> in master:
> 9cf8d35e8c Add a RAII wrapper for extracting FT_Face from PangoFcFont
> 15b7118410 Make Pango >= 1.36 mandatory.
> I'm fairly certain the branch can be removed.

This can be removed. It was a staging area when David wanted to revert
said patch from staging.

> As far as I understand, master now also has the relevant commits from
> dev/guile-v2-work, dev/guilev2, and dev/guilev21? Can those branches be
> dropped to avoid possible confusion about the current status?

I think so.

-- 
Han-Wen Nienhuys - hanw...@gmail.com - http://www.xs4all.nl/~hanwen

Re: Refactor get/set_property to take the item as first argument (issue 573670043 by d...@gnu.org)

[Han-Wen Nienhuys]

On Fri, Apr 10, 2020 at 1:07 PM David Kastrup  wrote:
> > * memory use: each SCM value takes 8 bytes, and there are 416 grob
> > properties today, for a total of 3328 bytes. Morgenlied is single page
> > of music and has 3704 grobs. So storage for the vectors (which will be
> > mostly empty) will take up 12M / page. This likely will result in
> > doubling the memory use of LilyPond.
>
> You haven't understood the scheme.  A grob only contains the properties
> seminal to its type.  That's why there is a double indirection.  A full
> array of 416 entries is needed per grob _type_, not per grob.

I still don't see why this requires an overall overhaul of the
internal calling convention. You can have a global registry of symbol
=> uint16 (numbering all the symbols), memoize that uint16 like we
memoize the symbol today (I think this is what you suggested already).
Then use a uint16 => value hashtable in the grob/prob/etc. A custom
hash implementation can save space by storing the uint16 key rather
than the 8 byte SCM symbol.

This would be a very conservative, localized change that doesn't
require setting up machinery to convert types to vectors, and it keeps
working if grob types change which grob-interfaces they support
halfway through.

There are many types (stem, axis-group, beam, hairpin) that properties
that are mostly unset. A fixed encoding will waste CPU cache and main
memory to allow these properties to be set even if it is not
necessary, so aside from increased complexity, it's not obvious that
there will be a performance gain.

> Hashing outside of a closed set still requires storing the actual symbol
> for corroboration.  Hashing inside of a closed set amounts to the
> first-stage lookup in my scheme, except that a small number guaranteed
> to be unique comes out.  This lookup can be memoised and will work
> permanently as opposed to hashing in a growing set.
>
> > We could experiment with this without requiring a giant rewrite of the
> > source code.
>
> What you call a "giant rewrite" is basically this patch.  It's a purely
> syntactical patch only extending rather than reducing possible
> approaches and perfectly compatible with different implementations.  The
> work for that patch is already done, the impact is similar to what
> happened when changing around the syntax of unsmob a few times ian the
> past.  It will allow for prolonged work on possibly different
> implementations without having to constantly deal with merge conflicts.
>
> It does not depend on picking a particular implementation.  You
> expressed interest in what I was planning to do with the opportunity
> this patch opens, I answered.  I do not, however, see the point in
> discussing and dissing (partially) unwritten code based on an incomplete
> understanding in a handwaving discussion.

I think it is not out of the ordinary to discuss plans invasive plans
before implementing them.  I think this change is invasive because it
changes the idiom across the source code, and I disagree that this
change is net neutral. C++ method calls are a more natural idiom plain
function calls. If we can get a significant performance boost, that
may be worth it, but I think we should first understand if that is the
case.  As said (see above), I am skeptical.

> It is more efficient to discuss objections of the "you haven't thought
> this through" kind on working code.
>
> Nothing in this patch here enforces picking a particular route.  It is
> just paving the ground for more work, and it does so in a manner that
> isn't so much of an eye sore that it can only be tolerated in
> expectation of immense future gains.

-- 
Han-Wen Nienhuys - hanw...@gmail.com - http://www.xs4all.nl/~hanwen

Re: python error running make doc

[Jonas Hahnfeld]

Am Samstag, den 11.04.2020, 14:33 +0200 schrieb Federico Bruni:
> Fedora 31. /usr/bin/python points to version 3.7.6.
> I'm running `make doc` in the translation branch and I get this error:
> 
> $ make -j3 doc
> GNUmakefile:30: warning: overriding recipe for target 'po-update'
> /home/fede/src/lilypond-translation/stepmake/stepmake/podir-targets.make:14: 
> warning: ignoring old recipe for target 'po-update'
> Making Documentation/po/out-www/messages (hard links)
> Making input/regression/out-www/collated-files.list < 1359 files
> Making input/regression/out-www/version.itexi
> Making input/regression/out-www/weblinks.itexi
> Making input/regression/out-www/AAA-intro-regression.texi < tely
> Making input/regression/out-www/collated-files.tely
> Making input/regression/out-www/collated-files.texi < tely
> Traceback (most recent call last):
>   File "../../scripts/lilypond-book.py", line 69, in 
> import lilylib as ly
>   File "/home/fede/src/lilypond-translation/python/lilylib.py", line 169
> print('command failed:', cmd, file=sys.stderr)
>   ^
> SyntaxError: invalid syntax
> make[3]: *** [../../make/ly-rules.make:25: 
> out-www/AAA-intro-regression.texi] Error 1
> make[3]: *** Waiting for unfinished jobs
> Traceback (most recent call last):
>   File "../../scripts/lilypond-book.py", line 69, in 
> import lilylib as ly
>   File "/home/fede/src/lilypond-translation/python/lilylib.py", line 169
> print('command failed:', cmd, file=sys.stderr)
>   ^
> SyntaxError: invalid syntax

This is odd, this "syntax" works fine for Python 3. However I see the
mentioned error when pasting the line into Python 2. Could you double-
check that your build indeed uses Python 3 (as is required for master)?

> make[3]: *** [../../make/ly-rules.make:36: out-www/collated-files.texi] 
> Error 1
> Traceback (most recent call last):
>   File 
> "/home/fede/src/lilypond-translation/scripts/build/create-weblinks-itexi.py", 
> line 601, in 
> make_download_source("downloadStableSource", VERSION_STABLE, lang)
>   File 
> "/home/fede/src/lilypond-translation/scripts/build/create-weblinks-itexi.py", 
> line 453, in make_download_source
> make_macro(macroLang(name,lang), string)
>   File 
> "/home/fede/src/lilypond-translation/scripts/build/create-weblinks-itexi.py", 
> line 428, in make_macro
> print(string.encode('utf-8'))
> UnicodeDecodeError: 'ascii' codec can't decode byte 0xc3 in position 
> 75: ordinal not in range(128)

Similarly, I remember that calling .encode() was required for Python 3
but didn't work for Python 2 (so I had to do it when switching). Maybe
the same problem as above?

Jonas


signature.asc
Description: This is a digitally signed message part

python error running make doc

[Federico Bruni]

Fedora 31. /usr/bin/python points to version 3.7.6.
I'm running `make doc` in the translation branch and I get this error:

$ make -j3 doc
GNUmakefile:30: warning: overriding recipe for target 'po-update'
/home/fede/src/lilypond-translation/stepmake/stepmake/podir-targets.make:14: 
warning: ignoring old recipe for target 'po-update'

Making Documentation/po/out-www/messages (hard links)
Making input/regression/out-www/collated-files.list < 1359 files
Making input/regression/out-www/version.itexi
Making input/regression/out-www/weblinks.itexi
Making input/regression/out-www/AAA-intro-regression.texi < tely
Making input/regression/out-www/collated-files.tely
Making input/regression/out-www/collated-files.texi < tely
Traceback (most recent call last):
 File "../../scripts/lilypond-book.py", line 69, in 
   import lilylib as ly
 File "/home/fede/src/lilypond-translation/python/lilylib.py", line 169
   print('command failed:', cmd, file=sys.stderr)
 ^
SyntaxError: invalid syntax
make[3]: *** [../../make/ly-rules.make:25: 
out-www/AAA-intro-regression.texi] Error 1

make[3]: *** Waiting for unfinished jobs
Traceback (most recent call last):
 File "../../scripts/lilypond-book.py", line 69, in 
   import lilylib as ly
 File "/home/fede/src/lilypond-translation/python/lilylib.py", line 169
   print('command failed:', cmd, file=sys.stderr)
 ^
SyntaxError: invalid syntax
make[3]: *** [../../make/ly-rules.make:36: out-www/collated-files.texi] 
Error 1

Traceback (most recent call last):
 File 
"/home/fede/src/lilypond-translation/scripts/build/create-weblinks-itexi.py", 
line 601, in 

   make_download_source("downloadStableSource", VERSION_STABLE, lang)
 File 
"/home/fede/src/lilypond-translation/scripts/build/create-weblinks-itexi.py", 
line 453, in make_download_source

   make_macro(macroLang(name,lang), string)
 File 
"/home/fede/src/lilypond-translation/scripts/build/create-weblinks-itexi.py", 
line 428, in make_macro

   print(string.encode('utf-8'))
UnicodeDecodeError: 'ascii' codec can't decode byte 0xc3 in position 
75: ordinal not in range(128)
make[3]: *** 
[/home/fede/src/lilypond-translation/stepmake/stepmake/texinfo-rules.make:126: 
out-www/weblinks.itexi] Error 1

make[3]: *** Deleting file 'out-www/weblinks.itexi'
make[2]: *** 
[/home/fede/src/lilypond-translation/stepmake/stepmake/generic-targets.make:167: 
WWW-1] Error 2
make[1]: *** 
[/home/fede/src/lilypond-translation/stepmake/stepmake/generic-targets.make:167: 
WWW-1] Error 2
make: *** 
[/home/fede/src/lilypond-translation/stepmake/stepmake/generic-targets.make:185: 
doc-stage-1] Error 2

stale git branches

[Jonas Hahnfeld]

Hi all,

following removal of dev/translation-* branches, I took a closer look
at stale branches. I think it would make sense to keep unscoped
branches (outside of dev/user/) to a minimum. This should also avoid
overlooking old changes that have not been merged yet.
The following list is by no means complete, but maybe a good start:

dev/pango contains commits:
53ed2b55e2 Add a RAII wrapper for extracting FT_Face from PangoFcFont
c93c477180 Make Pango >= 1.36 mandatory.
in master:
9cf8d35e8c Add a RAII wrapper for extracting FT_Face from PangoFcFont
15b7118410 Make Pango >= 1.36 mandatory.
I'm fairly certain the branch can be removed.

Branches dev/issue3300, dev/issue3330, dev/issue3648 are likely related
to the named issues which have status 'Verified'. AFAICS there are some
additional commits in the branches, could be due to review comments?
David, you are probably the best to judge if they are fully merged or
some changes could still be relevant, could you take a look?

As far as I understand, master now also has the relevant commits from
dev/guile-v2-work, dev/guilev2, and dev/guilev21? Can those branches be
dropped to avoid possible confusion about the current status?

Then there are some dev/user/ branches. I consider these relatively
"private" to that person and would not propose to delete them on a
global basis. Still maybe everyone can take a look and delete unused
personal branches on their own?

Jonas


signature.asc
Description: This is a digitally signed message part

Re: Reimplement Scheme_hash_table using linear probing. (issue 559790043 by hanw...@gmail.com)

[dak]

On 2020/04/11 11:36:16, hanwenn wrote:
> On Sat, Apr 11, 2020 at 1:05 PM 
wrote:
> >
> > On 2020/04/11 05:37:39, hanwenn wrote:
> > > >  In addition, I don't think that it is used to a degree where it
> > would
> > > significantly affect LilyPond's performance.
> > >
> > > It is not yet.
> > >
> > > My plan is to plugin this into Grob and Prob and see if there is a
> > measurable
> > > speed improvement. If there is none, it's likely that your double
> > indexing
> > > scheme will also not bring much.
> >
> > I'd strongly suggest we have numbers first before introducing ~300
lines
> > for a custom hash implementation. It's good to have the
implementation
> > available early (I haven't looked at it yet), but I think it should
only
> > be merged with strong evidence that it's worth it.
> 
> I'm not opposed to this, but in the same vein, I think we should then
> hold off on merging David's reorganization of the property accesses in
> https://codereview.appspot.com/573670043/
> 
> -- 
> Han-Wen Nienhuys - mailto:hanw...@gmail.com -
http://www.xs4all.nl/~hanwen

As I already said there: that change is purely syntactically as of that
patch, the difference in appearance is not odious, and it allows for
parallel development of one or more different implementations without
ongoing merge conflicts.

It doesn't favor any particular approach, merely reorganises the macro
call in a manner where more information is available to the
implementation of the macro.

This is not "in the same vein" but more like the aorta.

-- 
David Kastrup


https://codereview.appspot.com/559790043/

Re: Reimplement Scheme_hash_table using linear probing. (issue 559790043 by hanw...@gmail.com)

[Han-Wen Nienhuys]

On Sat, Apr 11, 2020 at 1:05 PM  wrote:
>
> On 2020/04/11 05:37:39, hanwenn wrote:
> > >  In addition, I don't think that it is used to a degree where it
> would
> > significantly affect LilyPond's performance.
> >
> > It is not yet.
> >
> > My plan is to plugin this into Grob and Prob and see if there is a
> measurable
> > speed improvement. If there is none, it's likely that your double
> indexing
> > scheme will also not bring much.
>
> I'd strongly suggest we have numbers first before introducing ~300 lines
> for a custom hash implementation. It's good to have the implementation
> available early (I haven't looked at it yet), but I think it should only
> be merged with strong evidence that it's worth it.

I'm not opposed to this, but in the same vein, I think we should then
hold off on merging David's reorganization of the property accesses in
https://codereview.appspot.com/573670043/

-- 
Han-Wen Nienhuys - hanw...@gmail.com - http://www.xs4all.nl/~hanwen

Re: Reimplement Scheme_hash_table using linear probing. (issue 559790043 by hanw...@gmail.com)

[dak]

On 2020/04/11 05:37:39, hanwenn wrote:
> >  In addition, I don't think that it is used to a degree where it
would
> significantly affect LilyPond's performance.
> 
> It is not yet. 
> 
> My plan is to plugin this into Grob and Prob and see if there is a
measurable
> speed improvement.

You cannot just "plug it in" since it doesn't integrate seamlessly into
the system of context-managed grob properties (override/revert).  There
is also lily/break-substitution.cc to deal with.

> If there is none, it's likely that your double indexing
> scheme will also not bring much.

I prefer to judge the viability of my approaches based on my own code,
but thanks for worrying.

>
https://codereview.appspot.com/559790043/diff/563830043/input/regression/scheme-unit-test.ly
> File input/regression/scheme-unit-test.ly (right):
> 
>
https://codereview.appspot.com/559790043/diff/563830043/input/regression/scheme-unit-test.ly#newcode6
> input/regression/scheme-unit-test.ly:6: #(ly:test-scheme-hash-table)
> On 2020/04/10 21:43:28, dak wrote:
> > This seems like a rather opaque way of testing functionality.
> 
> how about criticism that is constructive? What do you suggest instead?

Have the actual functionality tested spelled out in Scheme rather than
calling an opaque C++ block?  It's what we do elsewhere.
> 
>
https://codereview.appspot.com/559790043/diff/563830043/lily/include/scm-hash.hh
> File lily/include/scm-hash.hh (right):
> 
>
https://codereview.appspot.com/559790043/diff/563830043/lily/include/scm-hash.hh#newcode36
> lily/include/scm-hash.hh:36: Entry *table_;
> On 2020/04/10 21:43:28, dak wrote:
> > Any reason this is not a C++ array?  We don't use C style arrays
elsewhere.
> 
> so we can skip the marking for GUILEV2.

So how about making this a regular SCM vector?  That way you can also
skip the individual scm_gc_mark calls in Guilev1 and get everything
initialised to SCM_UNDEFINED.  Another possibility would be to use C++
STL code with a custom allocator.

>
https://codereview.appspot.com/559790043/diff/563830043/lily/scm-hash.cc
> File lily/scm-hash.cc (right):
> 
>
https://codereview.appspot.com/559790043/diff/563830043/lily/scm-hash.cc#newcode59
> lily/scm-hash.cc:59: if (old_table[i].key != NULL)
> On 2020/04/10 21:43:29, dak wrote:
> > NULL is not a valid SCM value, nor is it guaranteed not to overlap
with valid
> > SCM values that may be stored here.  Better use SCM_UNDEFINED here. 
Also SCM
> > values should not be compared numerically but by using scm_is_eq . 
That
> allows
> > making sure that there are no integer/SCM confusion (there is a
#define one
> can
> > set for that purpose) which are a typical source for problems.
> 
> this would create overhead because scm_gc_calloc allocates data filled
with
> zeroes rather than SCM_xxx. I added some more comment about the 
assumptions to
> the header.

So?  SCM values should not be compared numerically.  If you want to rely
on 0 initialization (which seems rather overblown considering the
overall scheme of things) and not want to break the type system, you
could still do if (SCM_UNPACK (old_table[i].key) != 0) (note that SCM
values are not guaranteed to be equivalent to a pointer type so
comparison to NULL is also a bad idea).

> 
>
https://codereview.appspot.com/559790043/diff/563830043/lily/scm-hash.cc#newcode73
> lily/scm-hash.cc:73: uintptr_t x = (uintptr_t) (key);
> On 2020/04/10 21:43:29, dak wrote:
> > For using the numerical value of an SCM, there is SCM_UNPACK. 
Please don't
> use
> > C style casts.  They always succeed, even in cases where they are a
very bad
> > idea.
> 
> Done.
> 
>
https://codereview.appspot.com/559790043/diff/563830043/lily/scm-hash.cc#newcode81
> lily/scm-hash.cc:81: {
> On 2020/04/10 21:43:28, dak wrote:
> > This only works for specific cases (uintptr_t == 4 or 8).  Instead
of creating
> > our own implementation, how about submitting a patch to Guile
upstream?  That
> > way there is better review for Guile-specific problems and we don't
have the
> > maintenance cost in our own code.
> 
> no. we'd only get the benefits when we move to GUILE 3.2, whenever
that is
> released.

Guile development works differently.  Stuff that isn't written by Andy
Wingo is placed directly into the "stable" releases if it is considered
suitable.  Otherwise it is getting ignored.  At any rate, there is no
point in parallel development.
> 
>
https://codereview.appspot.com/559790043/diff/563830043/lily/scm-hash.cc#newcode95
> lily/scm-hash.cc:95: if (table_[i].key != NULL)
> On 2020/04/10 21:43:28, dak wrote:
> > NULL is not a valid SCM value, and SCM values should be compared
using
> scm_is_eq
> > rather than !=.  Using SCM_UNDEFINED is the proper way of doing
things.
> 
> notice that we're not passing the NULL to GUILE anywhere.
> 
>
https://codereview.appspot.com/559790043/diff/563830043/lily/scm-hash.cc#newcode111
> lily/scm-hash.cc:111: *idx = int (hash (key) % cap_);
> On 2020/04/10 21:43:29, dak wrote:
> > If the capacity is going to be one less 

Re: Reimplement Scheme_hash_table using linear probing. (issue 559790043 by hanw...@gmail.com)

[jonas . hahnfeld]

On 2020/04/11 05:37:39, hanwenn wrote:
> >  In addition, I don't think that it is used to a degree where it
would
> significantly affect LilyPond's performance.
> 
> It is not yet. 
> 
> My plan is to plugin this into Grob and Prob and see if there is a
measurable
> speed improvement. If there is none, it's likely that your double
indexing
> scheme will also not bring much.

I'd strongly suggest we have numbers first before introducing ~300 lines
for a custom hash implementation. It's good to have the implementation
available early (I haven't looked at it yet), but I think it should only
be merged with strong evidence that it's worth it.

https://codereview.appspot.com/559790043/

Re: Web: update documentation symlinks, and use https in RewriteRules (issue 551650043 by v.villen...@gmail.com)

[v . villenave]

On 2020/04/09 19:52:47, c_sorensen wrote:
> I'm OK to have you push it, but I'd like to hear from those involved
in the
> release whether or not the steps in the CG were followed.

Well, I introduced new CG additions to that effect a few weeks ago:
https://sourceforge.net/p/testlilyissues/issues/5837/
So, it’s still fairly recent.

V.

https://codereview.appspot.com/551650043/

Re: Fix line comments for new snippets (issue 549820043 by d...@gnu.org)

[dak]

https://codereview.appspot.com/549820043/diff/565880043/scripts/auxiliar/makelsr.py
File scripts/auxiliar/makelsr.py (right):

https://codereview.appspot.com/549820043/diff/565880043/scripts/auxiliar/makelsr.py#newcode38
scripts/auxiliar/makelsr.py:38: new_lys_marker = "%% generated from %s"
% new_lys
Embarrassingly, this needs to be  as well as I discovered right now
before pushing.  I decided not to restart the review for this.

https://codereview.appspot.com/549820043/

PATCHES - Countdown for April 11th

[pkx166h]

Hello,

Here is the current patch countdown list. The next countdown will be on 
April 13th


A quick synopsis of all patches currently in the review process can be
found here:

http://philholmes.net/lilypond/allura/

***


 Push:

5881 Fix line comments for new snippets - David Kastrup
https://sourceforge.net/p/testlilyissues/issues/5881
http://codereview.appspot.com/549820043


 Countdown:

5884 Clean up many unused global variables - Jonas Hahnfeld
https://sourceforge.net/p/testlilyissues/issues/5884
http://codereview.appspot.com/547890043

5883 Web: update documentation symlinks, and use https in RewriteRules - 
Valentin Villenave

https://sourceforge.net/p/testlilyissues/issues/5883
http://codereview.appspot.com/551650043

5882 Refactor get/set_property to take the item as first argument - 
David Kastrup

https://sourceforge.net/p/testlilyissues/issues/5882
http://codereview.appspot.com/573670043


 Review:

5885 python: Compile files without extra copy - Jonas Hahnfeld
https://sourceforge.net/p/testlilyissues/issues/5885
http://codereview.appspot.com/579560057

1204 Documentation and regtests needed for external font files - 
Valentin Villenave

https://sourceforge.net/p/testlilyissues/issues/1204
http://codereview.appspot.com/557640051


 New:

5887 Simplify substitute_object_alist declaration - Han-Wen Nienhuys
https://sourceforge.net/p/testlilyissues/issues/5887
http://codereview.appspot.com/563840043

5886 Reimplement Scheme_hash_table using linear probing. - Han-Wen Nienhuys
https://sourceforge.net/p/testlilyissues/issues/5886
http://codereview.appspot.com/559790043

5771 remove unnecessary (descend-to-context ... 'Score) - Dan Eble
https://sourceforge.net/p/testlilyissues/issues/5771
http://codereview.appspot.com/557440043


 Waiting:

5811 mf: use python scripting for generating Emmentaler fonts - Han-Wen 
Nienhuys

https://sourceforge.net/p/testlilyissues/issues/5811
http://codereview.appspot.com/553580043

5740 Add \post to defer context actions to end of time step - Dan Eble
https://sourceforge.net/p/testlilyissues/issues/5740
http://codereview.appspot.com/581600043


***


Regards,

James

Re: Issue #1204: Document, and add regtest for, external fonts. (issue 557640051 by v.villen...@gmail.com)

[v . villenave]

https://codereview.appspot.com/557640051/diff/565870043/input/regression/font-name-add-files.ly
File input/regression/font-name-add-files.ly (right):

https://codereview.appspot.com/557640051/diff/565870043/input/regression/font-name-add-files.ly#newcode244
input/regression/font-name-add-files.ly:244: #(rmdir dummyfontdir)

So, David ran into a "Directory not empty" error with this regtest, but
I
haven’t been able to reproduce it.
https://sourceforge.net/p/testlilyissues/issues/1204/?page=1#998e/7f0d

What could be the cause?

V.

https://codereview.appspot.com/557640051/