Further efficiency of horizontal spacing (faster padding table)

[mike-spa]

When computing horizontal spacing, we are looking into this table tens of thousands of times. I had originally implemented it as a nested map, now I've re-implemented it as a 2D array, which is much faster to read into. On the scores I've tried, this ends up saving approx. 8% on the total layout time of a score, not too bad.

[cbjeukendrup]

Would it also be possible to define PaddingVector like this:

template<typename T>
using PaddingVector = std::array<T, TOT_ELEMENT_TYPES>;

[mike-spa]

The main thing I need here is to overload the operator [] and the .at() members, cause I want to maintain the ability to access the table as paddingTable[ElementType::type1][ElementType::type2] (which is quite elegant I think, and I've used it all over the the codebase at this point). The overload needs to be explicitely defined cause the enum type isn't implicitely converted to its underlying int (I've tried).

[cbjeukendrup]

Ah, I see, that's what I had missed :)

[mike-spa]

I found a way to do this by inheriting from the std::array though, instead of wrapping a class around it, which makes things more elegant. Gonna push it soon :)

[RomanPudashkin]

Do we really need to inherit something or add our own containers? Can we just use something like std::unordered_map instead?

[mike-spa]

The current implementation is an std::map. The type PaddingTable is currently defined as:

using PaddingTable = std::map<ElementType, <std::map<ElementType, double>>.

This is elegant code-wise, because I can access the map by:

_paddingTable[ElementType::type1][ElementType::type2]

(and at this point it is used everywhere in the codebase, so I would prefer not to change it).
But reading from a map is much slower than reading from an array (and we are reading into this map thousands of times, so it makes a big difference). So the idea is that I redefine the PaddingTable as

using PaddingTable = MyVector<MyVector<double>>

MyVector is essentially built around an std::array, but I want to overload the operator [] so I can still access the table by:

_paddingTable[ElementType::type1][ElementType::type2]

That way, I don't need to change all the instances where the container is used and I keep it elegant, but it works much faster. Hope that makes sense :)

[RomanPudashkin]

using PaddingTable = std::unordered_map <ElementType, <std::unordered_map<ElementType, double>>

[RomanPudashkin]

Also, it is very interesting why we need to refill this table (or look into it) "tens of thousands of times"

[mike-spa]

Every combination of [TypeA]-[TypeB] items has a minimum horizontal distance, specified by @oktophonie, which we call "padding". All paddings are very specific to each individual [TypeA]-[TypeB] combination (for example, BarLine-Note padding is different from BarLine-Accidental padding), and also they are not symmetric (for example, Note-BarLine padding is different from BarLine-Note padding). So we need a data structure to store these values, and it needs to be some form of 2D table. This table (i.e. the PaddingTable) is written only once, when the score is created, but is read a lot of times (every time one item has to decide the distance from the following item, it needs to read the appropriate padding value from the table). A big score can easily have tens of thousands of items, that's why I said the table could be accessed tens of thousands of times.

So, I did some tests on a big score, and here's what I got.

Current master, which uses std::map:

Different implementation, using std::unordered_map (3.8% faster)

This PR, using the custom container based on std::array (9.5% faster)

[cbjeukendrup]

If you decide to use my suggested definition of PaddingVector, then you could use the fill member of std::array: https://en.cppreference.com/w/cpp/container/array/fill

_paddingTable[ElementType::AMBITUS].fill(styleMM(Sid::ambitusMargin));

(in some other places below too).

Advantages:

• less typing/reading
• styleMM(Sid::ambitusMargin) is computed only once, instead of for every iteration

[cbjeukendrup]

I believe that std::array actually wants size_t instead of int. So TOT_ELEMENT_TYPES should then be size_t too.