Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimization attempt of findPeriod using binary search #163

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rveilleux
Copy link
Contributor

Optimized findPeriod to use a binary search into the 36 elements period table
O(log N) instead of O(N) lookups

@rveilleux
Copy link
Contributor Author

Note: Even while it performs less lookup in average, the loop is more complex, so perhaps real-world gain will be small.

@tehKaiN
Copy link
Member

tehKaiN commented Oct 15, 2022

It's a funny thing - I've done something similar in the past, emailed Frank W. about it and he told me he's unsure that the optimization is worth it. I've checked my code again by running it in release mode (-O3 compiler option) and it was horribly slower than his trivial array traversal.

Have you done some measurements with Bartman's profiler and/or setting custom.color[0] to fixed color during the function run? Lemme know how's the performance before/after.

My code looked like this:

static inline UBYTE findPeriod(UWORD *pPeriods, UWORD uwNote) {
    // Find nearest period for a note value
    // https://stackoverflow.com/questions/6553970/
    UBYTE ubLow = 0, ubHigh = MOD_PERIOD_TABLE_LENGTH;
    while (ubLow != ubHigh) {
        UBYTE ubMid = (ubLow + ubHigh) / 2;
        if (uwNote < pPeriods[ubMid]) {
            // This index, and everything below it, is not the first element
            // greater/equal than what we're looking for because
            // this element is no greater/equal than the element.
            ubLow = ubMid + 1;
        }
        else {
            // This element is at least as large as the element, so anything after it can't
            // be the first element that's at least as large.
            ubHigh = ubMid;
        }
    }
} 

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants