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

Add API for main thread deallocation of classes #599

Closed
wants to merge 2 commits into from

Conversation

maicki
Copy link
Contributor

@maicki maicki commented Oct 1, 2017

This PR add's a new API to let classes define if objects of that class need to be deallocated on main. Therefore we create the ASRequiringMainThreadDeallocating protocol that classes can conform to.

As we add some overhead for checking conformsToProtocol as well as ASClassRequiresMainThreadDeallocation is a hot path, I added a generic LRUCache to cache main thread deallocation of classes. The LRUCache in general could be handy in other places too as in comparison to NSURLCache it can also have support for primitive types so it could save us boxing / unboxing cost.

It is not ready for merge yet as it needs to be cleanup, but I would like to start a conversation if we would like to go forward with this approach.

TODOs:

  • Move ASRequiringMainThreadDeallocating related code out of ASInternalHelpers
  • Decide if we would like to add the LRUCache at all

Follow up PR to #598
Addresses #586

@ghost
Copy link

ghost commented Oct 1, 2017

🚫 CI failed with log

@maicki maicki force-pushed the MSDeallocTextKitComponentsMainMore branch from e971a69 to e46bf7b Compare October 1, 2017 14:54
@ghost
Copy link

ghost commented Oct 1, 2017

🚫 CI failed with log

@ghost
Copy link

ghost commented Oct 1, 2017

3 Warnings
⚠️ Any source code changes should have an entry in CHANGELOG.md or have #trivial in their title.
⚠️ Please ensure license is correct for ASRequiringMainThreadDeallocating.h:

//
//  ASRequiringMainThreadDeallocating.h
//  Texture
//
//  Copyright (c) 2017-present, Pinterest, Inc.  All rights reserved.
//  Licensed under the Apache License, Version 2.0 (the "License");
//  you may not use this file except in compliance with the License.
//  You may obtain a copy of the License at
//
//      http://www.apache.org/licenses/LICENSE-2.0
//

    
⚠️ Please ensure license is correct for ASTextKitComponents.mm:

//
//  ASTextKitComponents.mm
//  Texture
//
//  Copyright (c) 2014-present, Facebook, Inc.  All rights reserved.
//  This source code is licensed under the BSD-style license found in the
//  LICENSE file in the /ASDK-Licenses directory of this source tree. An additional
//  grant of patent rights can be found in the PATENTS file in the same directory.
//
//  Modifications to this file made after 4/13/2017 are: Copyright (c) 2017-present,
//  Pinterest, Inc.  Licensed under the Apache License, Version 2.0 (the "License");
//  you may not use this file except in compliance with the License.
//  You may obtain a copy of the License at
//
//      http://www.apache.org/licenses/LICENSE-2.0
//

    

Generated by 🚫 Danger

@ghost
Copy link

ghost commented Oct 1, 2017

🚫 CI failed with log

@appleguy
Copy link
Member

appleguy commented Oct 4, 2017

Thanks for exploring this and also #598!! This is an important line of investigation for a tricky issue...

Caching the results of conformsToProtocol: is really important, and I know you're very knowledgable about caches, so it's exciting to see the LRUCache idea :).

Should we just cache this kind of info (like "class X needs main thread deallocation: YES or NO") in an infinitely-sized cache instead of LRU? Can we use a C++ unordered_map, from void * (the singleton pointer of the class itself) to BOOL?

@maicki
Copy link
Contributor Author

maicki commented Oct 4, 2017

@appleguy We could just use and unordered_map for an infinitely-sized cache correct. It would work from Class to BOOL.

Closing in favor of #598

@maicki maicki closed this Oct 4, 2017
@maicki maicki deleted the MSDeallocTextKitComponentsMainMore branch September 8, 2018 16:24
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.

None yet

2 participants