Skip to content

Update FormSet to not recompute each scalling all the time #16886

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

Merged
merged 4 commits into from
Jul 16, 2024

Conversation

jecisc
Copy link
Member

@jecisc jecisc commented Jul 9, 2024

Having a scaling that is different of 1 can slow down a lot the image. For example, if we set a background in the image then the perf will be horrible because we will scale the Form of the background all the time and it takes time.

I'm proposing to save the forms we are scaling in the FormSet. Like this we only have to scale once.

Fixes #16884

I'd like a review from @Rinzwind

Having a scaling that is different of 1 can slow down a lot the image. For example, if we set a background in the image then the perf will be horrible because we will scale the Form of the background all the time and it takes time.

I'm proposing to save the forms we are scaling in the FormSet. Like this we only have to scale once.

Fixes pharo-project#16884

I'd like a review from @Rinzwind
@jecisc
Copy link
Member Author

jecisc commented Jul 9, 2024

I changed my way to deal with the problem. I introduced a CachedFormSet that keeps the scaled versions and I left the FormSet as before. Like this, my change only impacts AlphaImageMorph and not all the other forms.

@Rinzwind
Copy link
Contributor

Rinzwind commented Jul 9, 2024

I would suggest to keep the cached forms separate from the forms the CachedFormSet was constructed with, so that the #forms accessor only gives the latter ones.

It might be sufficient to have just one cached form, so #asFormWithExtent: on CachedFormSet could just answer the cached form if its extent is the same as the argument extent, or else would use the inherited implementation to get the form and set it as the new cached form.

@Rinzwind
Copy link
Contributor

Looks OK, though the sends of #asFormOfDepth: in the implementation of #asFormWithExtent: on CachedFormSet seem redundant, and I’m not sure about not having a limit on the size of the ‘cache’ collection.

@MarcusDenker MarcusDenker merged commit 9afe15a into pharo-project:Pharo13 Jul 16, 2024
1 of 2 checks passed
@jecisc jecisc deleted the perf-auto-scale-factor branch July 18, 2024 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AutoScaleFactor and background image are not working together
3 participants