Skip to content
This repository has been archived by the owner on Sep 4, 2024. It is now read-only.

Simplify implementation for ImageTagSet equality #1116

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

Therzok
Copy link
Contributor

@Therzok Therzok commented Feb 20, 2023

No description provided.

Therzok and others added 2 commits February 21, 2023 01:00
We're targeting netstandard2.0 and don't have ReadOnlySpan or any of the new APIs that avoid allocating
@Therzok Therzok force-pushed the dev/therzok/image-optimization branch from 4bd008c to 25cb1aa Compare February 20, 2023 23:03
We cache the ImageTagSet directly now. That should allow us to create fewer of these instances overall.
For the app I'm testing on, we cover 97% of the image tags combinations
In practice, cache hits will be better overall.

The other improvement is to not have to sort/create imagetagsets every time, instead opting to use more specialized APIs.
@Therzok Therzok force-pushed the dev/therzok/image-optimization branch from c63203a to 1557d48 Compare February 21, 2023 00:30
@Therzok Therzok requested a review from sevoku February 21, 2023 00:32
Copy link
Member

@sevoku sevoku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

These tag items amount for 97% of the image tags found in images.
*/
readonly string[] knownTags = new[] {
"dark",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know why I made the cache this way. Looking back, I think that we can store the original tags string and cache the ImageTagSet.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically, we can have some presets, but cache tags that are added later as well.


public ImageTagSet TryGetTagSet(string tags)
{
var index = Array.IndexOf(knownTags, tags);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change to a list, and cache any tagset that isn't in the list already.

@@ -1183,16 +1184,22 @@ public override object LoadImage (string fileName)

public override IEnumerable<string> GetAlternativeFiles (string fileName, string baseName, string ext)
{
if (!Context.RegisteredStyles.Any ()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check breaks my brain. I don't really understand why it is !Context.RegisteredStyles.Any()

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

Successfully merging this pull request may close these issues.

2 participants