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

refactor: LOImageCacher 구현 #351

Merged
merged 10 commits into from
Jan 25, 2024
Merged

refactor: LOImageCacher 구현 #351

merged 10 commits into from
Jan 25, 2024

Conversation

anyukyung
Copy link
Member

🧑‍🚀 PR 요약

해당 pr에서 작업한 내역을 적어주세요.

  • 패키지를 이용하여 LOImageCacher를 구현했습니다.
  • 캐싱을 위해 기존 코드에서 worker에서 일괄적으로 모든 Thumbnail을 다운로드하던 과정을 없앴습니다!
스크린샷 2024-01-21 오후 7 46 47
  • 아래와 같은 형태로 사용이 가능합니다. 갓피셔 🙏
    func setThumnailImage(with url: URL?) {
        if let url {
            thumbnailImageView.lo.setImage(with: url)
        } else {
            thumbnailImageView.image = .profile
        }
    }

📌 변경 사항

변경사항 및 주의 사항 (모듈 설치 등)을 적어주세요.

스크린샷 2024-01-21 오후 7 49 14
📸 ScreenShot

작동, 구현화면

  • 브레이크 포인트로 앱 종료 후 들어가는 경우, 삭제후 들어가는 경우를 테스트했을 때 모두 다 잘 동작했습니다만..
  • 지도 뷰에만 적용한 상태라 메모리 성능이 개선되었는지 확연하게 확인하기가 어려워서, PR approve되면 썸네일 쓰이는 부분에 모두 적용해볼 예정입니다!
스크린샷 2024-01-21 오후 8 01 50

Linked Issue

close #337


extension LOImageCacherWrapper where Base: UIImageView {

@MainActor
Copy link
Collaborator

@loinsir loinsir Jan 22, 2024

Choose a reason for hiding this comment

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

이미 UIImageView 자체가 @MainActor로 정의되어 있어서 떼어도 될 것 같습니다.
+) 정의된 해당 메서드 자체가 async가 아니기에 어차피 동작하지 않을것 같아요!
image

Copy link
Member Author

Choose a reason for hiding this comment

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

UIImageView의 내부 프로퍼티인 image를 직접적으로 변경하고있어서 메소드에 MainActor 어노테이션을 선언하지 않으면 외부 객체(다른 컨텍스트)에서 프로퍼티를 변경할 수 없더라구요.

그런데 인환님 리뷰를 보다보니 아래 코드가 더 명확한 것 같아 이렇게 변경하려고 합니다!! 혹시 제가 잘못 이해한 것 있으면 알려주세욤
아래 코드에서 MainActor를 제거하면 실행은 되지만 백그라운드 스레드에서 updateImage가 호출되고 런타임 에러가 발생합니다!

extension LOImageCacherWrapper where Base: UIImageView {

    public func setImage(with url: URL) {
        Task {
            if let data = await LOImageFetcher.shared.fetchImage(from: url) {
                await updateImage(data)
            }
        }
    }

    @MainActor
    private func updateImage(_ data: Data) {
        self.base.image = UIImage(data: data)
    }

}

Copy link
Member Author

Choose a reason for hiding this comment

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

MainActor 내부에 있는 프로퍼티가 고립된 상태라, 내부의 업데이트 메소드 등을 이용해 await 키워드를 붙여 호출하는 것은 가능하지만
직접적으로 변경하는 것은 외부에서 어떤 컨텍스트에서 접근하는지 모르니 이런 에러가 발생하는 것이 아닐까.. 생각해봅니다 🤔

Copy link
Collaborator

@loinsir loinsir Jan 23, 2024

Choose a reason for hiding this comment

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

오 유경님 말씀대로 제가 지금껏 Task에 대해 잘못 이해하고 있었네요
Task는 생성하는 시점의 actor-context를 이어받기 때문에, mainactor로 마킹을 시켜준 곳의 Task는 Main 스레드에서 동작되는 것이라 하네요

그래서 제가 말씀드린 것처럼 mainactor를 떼게 되면 해당 컨텍스트에서는 main 스레드인지 보장이 안되니 오류가 발생한다고 하네요

Task는 비동기 컨텍스트를 제공하는 것이지 실행 스레드를 제공하는 게 아니라는 걸 알았습니다.
Swift Concurrency... 제대로 다시 공부해야겠네요 ㅠㅠ 죄송함니다.

Copy link
Collaborator

Choose a reason for hiding this comment

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

이해하고 나니까 원본이 좀더 괜찮아보이긴 하지만...유경님 원하시는 대로 하셔도 좋을 것 같습니다!

Comment on lines 57 to 59
func configure(thumbnailImgaeURL: URL?) {
if let thumbnailImgaeURL {
thumbnailImageView.lo.setImage(with: thumbnailImgaeURL)
Copy link
Collaborator

Choose a reason for hiding this comment

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

image 오타 있습니다~

Copy link
Member Author

Choose a reason for hiding this comment

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

허거덩 감사합니다 🙀

Copy link
Collaborator

@loinsir loinsir left a comment

Choose a reason for hiding this comment

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

wrapper까지 구현하셨네요?!?
고생하셨습니다~~

Copy link
Collaborator

@chopmozzi chopmozzi left a comment

Choose a reason for hiding this comment

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

image cache를 취소하는 경우는 생기지 않나요? prepareForReuse때라던가
스토리지에 저장한 데이터를 지우는 경우도 필요 없는지 궁금합니다.
코드 상 큰 문제는 없어 보여 approve합니다. 고생하셨습니다.

Comment on lines +30 to +34
init(memory: Cache = Cache(),
disk: FileManager = .default) {
self.memory = memory
self.disk = disk
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

https://developer.apple.com/documentation/foundation/nscache/1407672-totalcostlimit
totalCostLimit을 지정하지 않으면 무한정으로 캐시에 데이터를 올려놓는다고 합니다. 해당 부분 제한을 두는건 어떨까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

마스터 클래스나 코드리뷰 때 이 부분에 대한 의견이 좀 나뉘었던게 기억나서, 지금은 iOS 내부 시스템에게 맡기도록 하였는데요..
킹피셔를 참고해서 공부하면서 한번 수정해보도록 하겠습니다 !!

@anyukyung anyukyung merged commit ba46b3a into iOS/dev Jan 25, 2024
1 check passed
@anyukyung anyukyung deleted the iOS/feat#337 branch February 4, 2024 07:18
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.

3 participants