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

[ASTextKitRenderer] Adding locking when accessing the text renderer cache #2075

Merged
merged 1 commit into from
Jan 25, 2023

Conversation

rcancro
Copy link
Contributor

@rcancro rcancro commented Jan 24, 2023

While NSCache is thread safe, that alone does not make rendererForAttributes thread safe. This experiment will add a lock to rendererForAttributes to see how that affects performance/stability. As second experiment will forego the cache altogether to validate that we are getting some value out of having a renderer cache.

…ache

While NSCache is thread safe, that alone does not make `rendererForAttributes` thread safe. This experiment will add a lock to `rendererForAttributes` to see how that affects performance/stability. As second experiment will forego the cache altogether to validate that we are getting some value out of having a renderer cache.
@rcancro rcancro requested a review from garrettmoon January 24, 2023 21:47
@@ -130,6 +130,23 @@ - (BOOL)isEqual:(ASTextNodeRendererKey *)object
return renderer;
}

static AS::RecursiveMutex __sharedRendererCacheInstanceLock__;
Copy link
Member

Choose a reason for hiding this comment

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

nit: should this go at the top with the other locks? Or is the thinking to clean this up if it works?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which other locks? Since these are static C functions I put the lock right next to them. Is there a better place to put them?

Copy link
Member

Choose a reason for hiding this comment

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

Ignore me, I was looking at a different file 😅

@rcancro rcancro merged commit 2c7ba22 into TextureGroup:master Jan 25, 2023
@rcancro rcancro deleted the textRenderer branch January 25, 2023 00:07
andrey-golovin-ios added a commit to andrey-golovin-ios/Texture that referenced this pull request Oct 5, 2023
* 'master' of https://github.com/TextureGroup/Texture:
  Fix build errors and a crash in xcode 15 (TextureGroup#2093)
  [ASCellNodeVisibilityEvent] Add a new event when scrolling stops (TextureGroup#2084)
  Trying to get CI to work (TextureGroup#2085)
  fix typo: ASStackLayoutElement.h (TextureGroup#2067)
  Docs: Fix references of ASViewController/ASNavigationController (non-existent) to ASDKViewController/ASDKNavigationController (TextureGroup#2072)
  [ASTextKitRenderer] Adding locking when accessing the text renderer cache (TextureGroup#2075)
  Switch UITextWritingDirection to NSWritingDirection (TextureGroup#2071)
muukii pushed a commit to FluidGroup/Texture that referenced this pull request Dec 13, 2024
…ache (TextureGroup#2075)

While NSCache is thread safe, that alone does not make `rendererForAttributes` thread safe. This experiment will add a lock to `rendererForAttributes` to see how that affects performance/stability. As second experiment will forego the cache altogether to validate that we are getting some value out of having a renderer cache.
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