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

[Enhancement] Lazy properties in code output #63

Closed
vladmircan opened this issue Jul 27, 2024 · 11 comments · Fixed by #70
Closed

[Enhancement] Lazy properties in code output #63

vladmircan opened this issue Jul 27, 2024 · 11 comments · Fixed by #70
Milestone

Comments

@vladmircan
Copy link

vladmircan commented Jul 27, 2024

Description
Instead of generating a custom singleton pattern for every icon, we could use the built-in lazy functionality.
For example:

val ArrowLeft: ImageVector
    get() {
        if (_ArrowLeft != null) {
            return _ArrowLeft!!
        }
        _ArrowLeft = ImageVector.Builder(....).build()

        return _ArrowLeft!!
    }

private var _ArrowLeft: ImageVector? = null

becomes

val ArrowLeft: ImageVector by lazy {
     ImageVector.Builder(....).build()
}

A minor thing to be sure but it does:

  1. Reduce indentation by one level
  2. Make the singleton pattern clearer
  3. Reduce the lines of code by 8 / per file which across something like 300 icons becomes significant

P.S. Love the plugin!

@egorikftp
Copy link
Member

Hello, thanks for idea 🙂
I thought about this option and want to add in next releases.

One point, I think better to add LazyThreadSafetyMode.NONE) to avoid unnecessary synchronization in this place.
Smth like this:

val ArrowLeft: ImageVector by lazy(LazyThreadSafetyMode.NONE) {
     ImageVector.Builder(....).build()
}

@vladmircan
Copy link
Author

Even better, looking forward to it!
Feel free to close this issue unless you want to keep it around as a reminder.

@egorikftp
Copy link
Member

Will keep open to track progress 🙂

@Nek-12
Copy link

Nek-12 commented Jul 30, 2024

I believe this is not ideal because lazy init will increase memory consumption by keeping a reference to the initializer and the lazy property delegate wrapper that in this case will serve no purpose other than looking pretty. If Google wanted to use lazy for their icons, they would have already done that, but they didn't.

@egorikftp
Copy link
Member

Yes, it is one of the reason why backing property is default output in plugin 🙂
Lazy block is just an alternative, but devs should be careful with this.

@egorikftp
Copy link
Member

@vladmircan feature implemented in latest nightly build: https://github.com/ComposeGears/Valkyrie/releases/tag/Nightly
could you please check on your project?

@vladmircan
Copy link
Author

@egorikftp It works, great job! Love that you added a toggle to choose between code outputs.
For my information and others that may stumble upon this thread, what is the expected additional memory footprint per image?

@egorikftp
Copy link
Member

GPT has other opinion about lazy block:

Memory Differences Analysis

Memory Overhead:

  1. Custom Lazy Initialization: The first approach has a slight overhead due to the nullable _Icon property. Each access involves a check and an additional property holder.

  2. lazy Delegate Initialization: It allocates additional memory through its internal mechanisms, but generally more optimized and compact compared to manually handling the lazy initialization.

The actual memory difference between these approaches is likely minimal and may not significantly impact performance, especially if the initialization code is lightweight and the application doesn't heavily rely on these icons.
However, lazily initializing via lazy is usually more idiomatic, concise, and potentially more performant because it avoids the manual null check and reduces boilerplate code.

Conclusions:

  • Maintainability and Readability: Using lazy is generally more readable and maintainable.

  • Memory Efficiency: lazy is optimized for memory and skips explicit null checks.

  • Thread Safety: Both approaches, as provided, are not thread-safe. For thread safety, use LazyThreadSafetyMode.SYNCHRONIZED as needed.

@vladmircan
Copy link
Author

@egorikftp Something to be aware of for sure, thanks for posting the analysis!

@Nek-12
Copy link

Nek-12 commented Jul 31, 2024

GPT is not entirely correct.
If we use non-thread-safe lazy, the additional footprint will be an additional class header and instance allocation + a dynamic mutable reference. The lazy internally also uses an if check to determine if the variable is initialized. The null value is only using a few cpu cycles for dereferencing and memory pointer comparison.

At this point this discussion is mostly nitpicking and overengineering, but I think the best solution is to just let the user choose the approach they want to use.

@egorikftp
Copy link
Member

Will close issue as completed in 0.5.0 release

https://github.com/ComposeGears/Valkyrie/releases/tag/Valkyrie-0.5.0

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 a pull request may close this issue.

3 participants