Re: Issue 5336: Remove downcasting methods from Grob_array and Grob_info (issue 344010043 by nine.fierce.ball...@gmail.com)
On Jun 12, 2018, at 18:18, carl.d.soren...@gmail.com wrote: > > The tradeoff of having people know about dynamic casting and using it > properly needs to be matched with people not needing to know about > dynamic casting and being able to ignore it. Carl, I appreciate hearing your perspective and I’d like to continue this discussion to the point of reaching a consensus on the design. I perceive that we understand each other’s points and simply disagree. There is nothing new I want to counter with. I will just state that if a contributor were made uncomfortable by dynamic_cast, my two-pronged solution would be (1) gently encourage him to educate himself on this fundamental feature of C++, and (2) over time, rework the software to require fewer casts by preserving more type information in the internal interfaces and pushing the casts outward toward the interface with Scheme. > As I said before, I'm not asking for a reversion. I think I just have a > different tradeoff value model than you have. Clearly, and I’m sure David can contribute a third perspective, and I hope he will choose to. This attempted clean-up and some other work in progress has been about as engaging as pounding sand down a rat hole, and I have only persevered in it because I considered it a step in the direction of a cleaner system in the long run. If it is valuable, then I would be willing to continue, but if that is not the general perception, then I would rather take one step back, restore the code to an acceptable state (probably not quite a straight reversion, though), and never touch it again. Thence, back to consensus: From what I’ve seen, the existing approach in many (most?) cases is that any kind of grob can be passed to any function and if it’s the right kind of grob, you get what you asked for, and if it’s the wrong kind of grob, you get something else. I can see that this is necessary at the interface between Scheme and C++, but it doesn’t have to be carried through to all the C++ internals, with each step working to determine the specific type information it requires but neglecting to propagate it to the next step. That loose approach could be continued, but it doesn’t have to be. Should it be continued? Regards, — Dan ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Issue 5336: Remove downcasting methods from Grob_array and Grob_info (issue 344010043 by nine.fierce.ball...@gmail.com)
Dan, Thanks for the feedback. I appreciate it. I'm still not convinced that pulling the dynamic casts out of the getter/setter pair is better. You talk about performance penalties of dynamic casting. But your profiling shows that dynamic casting took about 1% of the processing time. Even if you could reduce dynamic casting by 90% (which seems unlikely), you'd only reduce execution time by 0.9%, which seems pretty insignificant. The tradeoff of having people know about dynamic casting and using it properly needs to be matched with people not needing to know about dynamic casting and being able to ignore it. It seems to me that unless there is a significant mistake that is made by code that doesn't know about the dynamic cast, it's better off to hide it. In fact, it seems to me that it would be possible (and maybe preferable) to put necessary error checking once in the setter/getter, rather than having to recreate it multiple times throughout the code base. If dynamic casting were taking up 50% of the process time, or errors in using dynamic casting were responsible for large numbers of bugs, I might feel different. But to me, the benefits I understand from your explanation seem to be not worth the cost of having dynamic casts show up in 21 different files. As I said before, I'm not asking for a reversion. I think I just have a different tradeoff value model than you have. Thanks for your explanation, Carl https://codereview.appspot.com/344010043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Issue 5336: Remove downcasting methods from Grob_array and Grob_info (issue 344010043 by nine.fierce.ball...@gmail.com)
I remembered another thing about dynamic casting which is not
specifically relevant to the changes in this review, and which should be
pretty well known, but I'll record it here for the sake of completeness.
Wherever there is alternative processing predicated on type, e.g.
if (A *a = dynamic_cast (info.grob ())) { ... }
else if (B *b = dynamic_cast (info.grob ()) { ... }
else if (C *c = dynamic_cast (info.grob ()) { ... }
the reader should stop and consider that perhaps defining a virtual
method would be better. It could be the case that there is only one
block that does something and the others are omitted.
Again, this pattern is more easily recognized with the casts out in the
open than if they were obscured as, info.item(), info.spanner(), etc.
https://codereview.appspot.com/344010043/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: issue #5341: Urgent corrections to stencil-integral.cc (skylines) (issue 341350043 by torsten.haemme...@web.de)
On 2018/06/12 10:49:11, Be-3 wrote: On 2018/06/12 10:05:17, dak wrote: > However, cough cough, all this is an unnecessary complication. Sorry for > overlooking this. Just write > > Stencil q = *s; > […] Just to mention it: How about the definition of ly:stencil-rotate in lily/stencil-scheme.cc? There, we should have the exact same problem, shouldn't we? No. new_s is the return value so the compiler cannot possibly optimize it away. https://codereview.appspot.com/341350043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: issue #5341: Urgent corrections to stencil-integral.cc (skylines) (issue 341350043 by torsten.haemme...@web.de)
On 2018/06/12 10:49:11, Be-3 wrote: On 2018/06/12 10:05:17, dak wrote: > However, cough cough, all this is an unnecessary complication. Sorry for > overlooking this. Just write > > Stencil q = *s; > […] Just to mention it: How about the definition of ly:stencil-rotate in lily/stencil-scheme.cc? There, we should have the exact same problem, shouldn't we? > I'm afraid I'll have to take a much more thorough look at this shit show and > rework its data structures. That'd be great… > I suggest you remove this change (which would not > even help due to the lack of scm_remember_upto_here_1) from this patch set, > retaining the cosmetic parts, and I'll try coming up with something that has a > chance of working regarding the GC parts. Do you mean "remove the grob rotation coding" (Stencil q = *s;). The cosmetic part being just < and <= etc...? Yup Is there a simple (minimal example way) to provoke the garbage collection problem, because in my test cases, it (accidentally) worked. It will work most of the time. Until it doesn't. Basically you need to mess with the garbage collection defaults until you trigger a collection basically on every allocation. That will run slow as anything but has a much higher chance of exposing such problems. https://codereview.appspot.com/341350043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: issue #5341: Urgent corrections to stencil-integral.cc (skylines) (issue 341350043 by torsten.haemme...@web.de)
On 2018/06/12 10:05:17, dak wrote: However, cough cough, all this is an unnecessary complication. Sorry for overlooking this. Just write Stencil q = *s; […] Just to mention it: How about the definition of ly:stencil-rotate in lily/stencil-scheme.cc? There, we should have the exact same problem, shouldn't we? I'm afraid I'll have to take a much more thorough look at this shit show and rework its data structures. That'd be great… I suggest you remove this change (which would not even help due to the lack of scm_remember_upto_here_1) from this patch set, retaining the cosmetic parts, and I'll try coming up with something that has a chance of working regarding the GC parts. Do you mean "remove the grob rotation coding" (Stencil q = *s;). The cosmetic part being just < and <= etc...? Is there a simple (minimal example way) to provoke the garbage collection problem, because in my test cases, it (accidentally) worked. https://codereview.appspot.com/341350043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: issue #5341: Urgent corrections to stencil-integral.cc (skylines) (issue 341350043 by torsten.haemme...@web.de)
https://codereview.appspot.com/341350043/diff/1/lily/stencil-integral.cc File lily/stencil-integral.cc (right): https://codereview.appspot.com/341350043/diff/1/lily/stencil-integral.cc#newcode1132 lily/stencil-integral.cc:1132: SCM new_s = s->smobbed_copy (); This does not work: the compiler will optimize new_s away unless you write scm_remember_upto_here_1 (new_s) at a proper later point of time (after last use). However, cough cough, all this is an unnecessary complication. Sorry for overlooking this. Just write Stencil q = *s; instead of allocating the stencil in some gc-accessible area (of course then also using q.rotate_degrees and q.expr ()). It's a feature of "simple smobs" that they contain all of their referenced SCM values in the structure itself and so can be used as normal automatic variable. You don't want a "smobbed copy" here, an ordinary (and unsmobbed) copy will work fine and save you all the trouble of having to cater for garbage collection: the stack is automatically scanned. Of course this still suffers from the whole "Transform_matrix_and_expression" thing being crap since it does not protect the contained SCM expression, and creating a vector of it is particularly bad mojo since then the data is outside of the stack and no longer scanned for SCM values. The old stencil expression likely got away by being protected in the original data it came from. Your rotated stencil does not have that luxury. So basically the old code tended to usually work by accident and your code does not have the luxury to continue working by accident. I'm afraid I'll have to take a much more thorough look at this shit show and rework its data structures. I suggest you remove this change (which would not even help due to the lack of scm_remember_upto_here_1) from this patch set, retaining the cosmetic parts, and I'll try coming up with something that has a chance of working regarding the GC parts. https://codereview.appspot.com/341350043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
