Pitch.CalculateTransposeHalfTone performance improvements

[ammatwain]

The code I propose is approximately 99.97% faster than the original code.
The functions have been embedded in a separate HTML page and tested with Chromium Browser,
comparing a loop of 2,000,000 consecutive instructions, and the return values have been compared
with each other without errors, matching the previous code.
// for (halftone = -1000000; halftone <1000000; halftone++) {...}
WrapAroundCheck_Old() 72426 milliseconds for 2000000 operations
WrapAroundCheck_New() 15 milliseconds for 2000000 operations
Performance improvements: 99.97928920553393%

[sschmidTU]

Nice! We just need to check if the result is the same, I'll run the visual regression tests.
Also, this code is less intuitive/understandable, so some explanatory comments should probably be added. Also (a docstring) for the function, ideally.

[sschmidTU]

Also, I don't think this function is used very often, so the performance impact will probably be hardly noticable. If we usually spend 100 nanoseconds on all of the function calls, and now only 1 nanosecond, it will hardly be noticable. (we probably spend more than 100 nanoseconds, but just theoretically speaking)
That makes it all the more important to keep the same level of understandability.

I'll also quickly check how often this function is used and how much time it uses.

[sschmidTU]

By the way, most of the time is spent during Vexflow rendering and skybottomline processing, not in OSMD processing. We already profiled this, maybe you'll find info in #1242 or #1158 (which added shaders for skybottomline).
Still, it's nice to optimize a bit here of course.

[ammatwain]

By the way, most of the time is spent during Vexflow rendering and skybottomline processing, not in OSMD processing. We already profiled this, maybe you'll find info in #1242 or #1158 (which added shaders for skybottomline). Still, it's nice to optimize a bit here of course.

on my fork i put a console.log
in the function and it is actually used. Sometimes sheet music has thousands and thousands of notes! :)

[ammatwain]

In any case, you must do all the necessary checks!

[sschmidTU]

i put a console.log
in the function and it is actually used

I didn't say it's not used, I said it's not used a lot, and not much time is spent in it ;) but I'll check it myself as well.
And as I said, it's still a nice performance improvement, it should just remain understandable code (via comments).
I can also add the comments after the merge if you prefer.

[ammatwain]

This is right! The actual code of your function is very understandable, this facilitates optimizations!

In any case, think back to my test, if I start from halftone values ​​from -1000000 to +1000000 (values ​​that don't exist in music) and considering that the current routine is based on while loops to increase the octaves, it is obvious that the my routine is faster.
I have to rewrite the test thinking of values ​​intended in a narrow range (>=128 .. <128)

[ammatwain]

Indeed, I got carried away by enthusiasm; now the values are completely different. In the following message, I will write my test.

Test 1: consistency between results
SUCCESS!!! from -128 at 128 WrapAroundCheck_New() has the same results as WrapAroundCheck_Old()
Test 2: WrapAroundCheck_Old performance
Test 3: WrapAroundCheck_New performance
WrapAroundCheck_Old 32 milliseconds for 2560000 operations
WrapAroundCheck_New 18 milliseconds for 2560000 operations
Performance Improvements: 43.75%

[ammatwain]

<script>
function log(text){
  let div = document.createElement("div");
  div.innerHTML = text;
  document.body.appendChild(div);
}
function WrapAroundCheck_Old(value, limit) {
    let overflow = 0;
    while (value < 0) {
        value += limit;
        overflow--; // the octave change
    }
    while (value >= limit) {
        value -= limit;
        overflow++; // the octave change
    }
    return { overflow: overflow, halftone: value };
}

function WrapAroundCheck_New(value, limit) {
    return {overflow: Math.floor(value / limit) || 0 , halftone:(value % limit + limit) % limit || 0 };
}

function transpose_test(){
  let repetitions = 10000;
  let startNumber = -128;
  let endNumber = +128;
  let shots = (endNumber - startNumber) * repetitions;
  log("Test 1: consistency between results");
  let success = true;

  for (let e = 0; e < repetitions ; e++) {
    for (let i = startNumber; i < endNumber; i++) {
        let o = WrapAroundCheck_Old(i, 12);
        let n = WrapAroundCheck_New(i, 12);
        if (o.overflow !== n.overflow || o.halftone !== n.halftone) {
            success = false;
        }
    }
  }

  if (success) {
      log(`SUCCESS!!! for ${shots} times WrapAroundCheck_New() has the same results as WrapAroundCheck_Old()`);
      log("Test 2: WrapAroundCheck_Old performance");

      let aOld = Date.now();
      for (let e = 0; e < repetitions ; e++) {
          for (let i = startNumber; i < endNumber; i++) {
              WrapAroundCheck_Old(i, 12);
          }
      }
      let zOld = Date.now();
      let oldTime = zOld - aOld;

      log("Test 3: WrapAroundCheck_New performance");

      let aNew = Date.now();

      for (let e = 0; e < repetitions ; e++) {
        for (let i = startNumber; i < endNumber; i++) {
            WrapAroundCheck_New(i, 12);
        }
      }
      let zNew = Date.now();

      let newTime = zNew - aNew;

      let performanceImprovements = ((oldTime - newTime)/oldTime) * 100;
      log(`WrapAroundCheck_Old ${oldTime} milliseconds for ${shots} operations`);
      log(`WrapAroundCheck_New ${newTime} milliseconds for ${shots} operations`);
      log(`Performance Improvements: ${performanceImprovements}%`);
  }
  else {
      log("FAILURE!!! WrapAroundCheck_New() does not have the same results af WrapAroundCheck_Old()");
  }
}
window.onload = function(){
  transpose_test();
}
</script>

[ammatwain]

... and with >=24 .. <24 range

Test 1: consistency between results
SUCCESS!!! for 480000 times WrapAroundCheck_New() has the same results as WrapAroundCheck_Old()
Test 2: WrapAroundCheck_Old performance
Test 3: WrapAroundCheck_New performance
WrapAroundCheck_Old 9 milliseconds for 480000 operations
WrapAroundCheck_New 9 milliseconds for 480000 operations
Performance Improvements: 0%

...please and for the sake of clarity, leave your routine untouched... :D

[sschmidTU]

@ammatwain So in the realistic usecases of -128 to +128 halftones transposition, your method is 43.75% faster, and for more common cases of -24 to +24 halftones, it's the same speed, correct?
We can still use your version since it's faster in some cases (and shorter and correct), add some comments, and maybe add the old code in comments below so that we don't lose understandability.

[ammatwain]

The function comment could be something like this?

function WrapAroundCheck(value, limit) {
    return {
        // Calculate the number of times "value" exceeds the "limit" in the
        // positive direction and round it down to the nearest integer.
        // "overflow" represents the octave in which the transposed note resides.
        overflow: Math.floor(value / limit) || 0 ,
        // Calculate the "halftone" corresponding to the transposed note using
        // the modulo (%) operator. Ensure the result is always positive by
        // adding the "limit" to the remainder and then calculating the modulo "limit".
        halftone:(value % limit + limit) % limit || 0
    }
}

[sschmidTU]

Closing, as I've added the alternative method in a comment in commit e6cbba6, and the method is not faster for realistic use cases.

Still, thanks for the investigation, it could have been a performance improvement if the realistic input data range wasn't limited, and it's an interesting rewrite of the loops into a one-line mathematical statement / code line.

By the way, you've named the TransposeCalculator class file TransportCalculator and addded it as a new file, but it's not relevant anymore now.

[ammatwain]

Simon, thank you for your response. You did well to keep the original method since it performs its job excellently. Regarding the commit containing the "TransportCalculator" class, I apologize; it was due to my lack of experience with GIT.

As for the "TransportCalculator" class, I am currently investigating the possibility of transposing between musical keys instead of semitones.

[sschmidTU]

No problem!
Transposing to a target key (signature) would be a great comfort option / good API!