Skip to content

SP5: Cache typefaces#2848

Merged
swharden merged 5 commits intoScottPlot:mainfrom
KroMignon:SP5_2833
Aug 8, 2023
Merged

SP5: Cache typefaces#2848
swharden merged 5 commits intoScottPlot:mainfrom
KroMignon:SP5_2833

Conversation

@KroMignon
Copy link
Contributor

Update FontStyle to add a typeface cache to avoid to call too often the font face lookup table.

This should solve issue #2833

Fabrice Mousset added 2 commits August 7, 2023 08:27
Update FontStyle to add a typeface cache to avoid to call too often the font face lookup table.

This should solve issue ScottPlot#2833
fix string comparison issue: comparing string content not string holder!
@swharden swharden changed the title Add font typeface cache SP5: Cache typefaces Aug 8, 2023
@swharden swharden enabled auto-merge August 8, 2023 00:30
@swharden
Copy link
Member

swharden commented Aug 8, 2023

Thank you for this fantastic solution @KroMignon! I really appreciate your work improving ScottPlot 5 performance with this pull request 🚀

A few thoughts about this work:

  • I hope to to benchmark performance before/after this PR using docker: SP5: How to get started using ScottPlot 5 in Docker #2851

  • Your original SetField<T> function was very elegant! I'm copying the code below so it can be appreciated. However, to simplify the code I chose to put all the logic in the getters for font Name, Bold, and Italic. Your solution worked well and would have been preferred if there were more properties. However, moving this logic to the getters allowed explicit typing (preventing the need for casting) and also avoided multiple levels of if statements. In the end I favored the simplicity at the cost of a small amount of code duplication.

// strategy in the original PR
protected bool SetField<T>(ref T fieldValue, T value)
{
    if (value is string)
    {
        if (string.Compare(fieldValue as string, value as string) == 0)
            return false;
    }
    else if (Equals(fieldValue, value))
        return false;
    fieldValue = value;
    _typeface = null;
    return true;
}

Thank you again for this excellent work! 🙏

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.

SP5: frequent typeface lookups result in poor performance on Linux

2 participants