Fractions comparison efficiency #4699
Conversation
libmscore/fraction.cpp
Outdated
| @@ -139,6 +141,7 @@ Fraction& Fraction::operator*=(const Fraction& val) | |||
| { | |||
| _numerator *= val._numerator; | |||
| _denominator *= val._denominator; | |||
| if (val._denominator!=1) reduce(); // We should be free to fully reduce here | |||
There was a problem hiding this comment.
space around operators again
libmscore/fraction.cpp
Outdated
| @@ -156,6 +159,8 @@ Fraction& Fraction::operator/=(const Fraction& val) | |||
| { | |||
| _numerator *= val._denominator; | |||
| _denominator *= val._numerator; | |||
| if (_denominator<0) { _denominator *= -1; _numerator *= -1; } | |||
| if (val._numerator!=1) reduce(); | |||
There was a problem hiding this comment.
space around operators again
libmscore/fraction.cpp
Outdated
| if (_denominator == val._denominator) _numerator += val._numerator; // Common enough use case to be handled separately for efficiency | ||
| else { | ||
| int l = lcm(_denominator,val._denominator); | ||
| _numerator = _numerator * (l/_denominator) + val._numerator * (l/val._denominator); |
There was a problem hiding this comment.
space around operators again
libmscore/fraction.cpp
Outdated
| { | ||
| const auto product = static_cast<int_least64_t>(a) * b; | ||
| return static_cast<int>(product / gcd(a, b)); | ||
| return (a/gcd(a, b)) * b; // Divide first to minimize overflow risk |
There was a problem hiding this comment.
Needs space around operators, as per MuseScore style guidelines.
libmscore/fraction.cpp
Outdated
| return a < 0 ? -a : a; | ||
| return gcd(b, a % b); | ||
| int bp; | ||
| while(b!=0) { |
There was a problem hiding this comment.
Needs space around operators, as per MuseScore style guidelines.
libmscore/fraction.cpp
Outdated
| @@ -21,22 +21,24 @@ namespace Ms { | |||
| // greatest common divisor | |||
| //--------------------------------------------------------- | |||
|
|
|||
| static int gcd(int a, int b) | |||
| static inline int gcd(int a, int b) | |||
There was a problem hiding this comment.
inline doesn't make sense here actually. This keyword does not influcence function call inlining by a compiler, and actual inlining depends on compiler settings. A compiler should anyway be able to inline this function calls since it is defined in the same translation unit as those functions that use it (and before them).
There was a problem hiding this comment.
But would it make sense, and if so would you recommend it, for ticks()?
|
To get the real speed increase you should try moving all definitions into |
velochy
force-pushed
the
FractionPerformance
branch
5 times, most recently
from
f6cc513
to
ffb9cb5
Compare
…ns more efficient
velochy
force-pushed
the
FractionPerformance
branch
from
ffb9cb5
to
70868cd
Compare
Benchmarks:Hardware2015 MacBook Pro with Retina display. (specs)
SoftwareMuseScore AppImage nightly builds:
InputI used the OpenScore Edition of Mozart's Jupiter Symphony.
The score was created in MuseScore 2, so I converted it to MuseScore 3 format before running these tests. MethodEach test is the time (from Bash Results
Notes:
InterpretationOverall, unoptimised Fractions led to around a 5% slowdown compared to integer ticks, but @velochy's optimisations claw back about a third of this penalty. A pretty good result then! The optimizations were definitely worth it in this case, but we should remember that this is only because Fractions are used literally everywhere in the code! For most other things it is far more important to write clean code that is easy to maintain than to spend time worrying about data types and optimizations. In particular, the trick of putting everything in the |
|
I can also confirm that with the exception of the PDFs, all files created by each version of MuseScore were byte-for-byte identical. Even the WAV files were identical, so I guess we will need a file with some extreme tuplets to see a difference there. I thought the difference between the PDF files was down the them just containing a timestamp of when the file was created, but looking at the output of |
|
I was asked to update the benchmark to include MuseScore 2.3.2 for comparison. Unfortunately there isn't room for another column in the original table, so here is a new table with the ticks and optimised fractions columns copied from the previous one. Codefor fmt in mscx mid pdf svg wav; do
echo "#### Format: $fmt ####"
for n in {1..3}; do
sleep 2 # allow time to recover from previous run
time MuseScore*.AppImage Jupiter.mscz -o ms2.$fmt 2>/dev/null
done
ls -l ms2.$fmt
md5sum ms2.$fmt
doneResults
InterpretationMuseScore 3 is significantly faster than MuseScore 2, regardless of whether MuseScore 3 uses fractions or integer ticks. This will be due to MuseScore 3 using more efficient algorithms for expensive operations like layout. The benefits of small scale optimizations to data types pale in comparison to these changes. This demonstrates that it is more important to ensure low level classes are easy to use than to optimize them for speed, as the penalty of using them incorrectly will be much greater. Edit: Naturally, the above interpretation is only valid for converting files on the command line. It does not necessarily apply to the GUI, though we know that the layout improvements in MuseScore 3 have made the GUI much faster for large scores. However, it is possible that regressions elsewhere have led to slower performance with smaller scores where layout is less of a burden. |
|
Thank you @shoogle. I think we need more benchmarks for common operations like inserting notes to a big score, deleting measures, etc. @dmitrio95's script tests could be used for such benchmarks. It is interesting to compare 2.3.2 and current master. |
|
Here is how the binary sizes compare.
|
| Version | Build | Size (bytes) | Size (MB) | Change |
|---|---|---|---|---|
| 2.3.2 | Integer ticks | 24674456 | 24.7 | -7.71% |
| 3.0 nightly | Integer ticks | 26737664 | 26.7 | (reference) |
| 3.0 nightly | Unoptimized fractions | 26844160 | 26.8 | +0.398% |
| 3.0 nightly | Optimized fractions | 26958848 | 27.0 | +0.827% |
Switching to fractions increased MuseScore 3's size by less than half a percent. Optimizing the fractions (which potentially involves the compiler inlining the functions in fraction.h) led to a similar increase in size, but improved speed by 2% or better compared to unoptimized fractions. The overall size increase is less than 1%.
MuseScore 3 is bigger than MuseScore 2 due to it having more features. Resources embedded via the Qt Resource System also contribute to the size of the executable, so any additional resources would also lead to an increase in size (and therefore memory footprint).
Size of AppImage
Note: Type 1 AppImages are compressed ISO 9660 filesystems.
| Version | Build | Size (bytes) | Size (MB) | Change |
|---|---|---|---|---|
| 2.3.2 | Integer ticks | 122748928 | 122.75 | -16.8% |
| 3.0 nightly | Integer ticks | 147587072 | 147.59 | (reference) |
| 3.0 nightly | Unoptimized fractions | 147652608 | 147.65 | +0.044% |
| 3.0 nightly | Optimized fractions | 147718144 | 147.72 | +0.089% |
In terms of switching to fractions, the AppImage compression has reduced the size differences by a factor of 10. This means that the size of the file that users actually download and that takes up disk space on their system has increased by less than 0.1%.
As well as the binary executable, the AppImage also contains all libraries, templates, plugins, soundfonts and icons. These accompanying resources are responsible for most of the increase in size of MuseScore 3's AppImage compared to MuseScore 2's. MuseScore 3 uses a newer version of Qt, for example.
Unrolled the recursion in gcd and inlined both gcd and lcm.
Handled special case where denominators match.
Any comments welcome.