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 smol caching & a utility method #1239

Merged
merged 3 commits into from
Dec 6, 2024
Merged

Conversation

pyricau
Copy link
Member

@pyricau pyricau commented Dec 4, 2024

No description provided.

@pyricau pyricau requested review from zach-klippenstein and a team as code owners December 4, 2024 19:25
@@ -58,6 +58,21 @@ public class WorkflowIdentifier internal constructor(

private val proxiedIdentifiers = generateSequence(this) { it.proxiedIdentifier }

private val cachedToString by lazy {
Copy link
Collaborator

@zach-klippenstein zach-klippenstein Dec 4, 2024

Choose a reason for hiding this comment

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

FYI: Each by lazy allocates an additional private object to store the property delegate. If we don't need multi-thread synchronization, it's cheaper to just use a nullable backing property and initialize it yourself. Usually matters more for framework code, so we might want to avoid by lazy here.

E.g.

private var _cachedToString: String?
private val cachedToString: String
  get() = _cachedToString ?: calculateToString().also { _cachedToString = it }

Copy link
Member Author

@pyricau pyricau Dec 5, 2024

Choose a reason for hiding this comment

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

Did that but made cachedToString volatile because I'm not 100% this is actually single threaded. volatile is still better than using synchronization primitives as there's no locking, and the downside is that we might sometimes race init and compute it twice then set it twice to the same value, which is ok.

@steve-the-edwards
Copy link
Contributor

See build failure - we run in explicit API mode, so you need to add return types explicitly.

* Either a [KClass] or [KType] representing the "real" type that this identifier
* identifies – i.e. which is not an [ImpostorWorkflow].
*/
public val realIdentifierType by lazy {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is what needs an explicit type declared.

@pyricau pyricau force-pushed the py/nit_cleanups branch 3 times, most recently from 37d902a to a14b08d Compare December 5, 2024 21:18
@pyricau
Copy link
Member Author

pyricau commented Dec 6, 2024

e: file:///home/runner/work/workflow-kotlin/workflow-kotlin/workflow-core/src/commonMain/kotlin/com/squareup/workflow1/WorkflowIdentifier.kt:69:57 Platform declaration clash: The following declarations have the same JVM signature (getRealIdentifierType()Lcom/squareup/workflow1/WorkflowIdentifierType;):
    fun `<get-realIdentifierType>`(): WorkflowIdentifierType defined in com.squareup.workflow1.WorkflowIdentifier
    fun getRealIdentifierType(): WorkflowIdentifierType defined in com.squareup.workflow1.WorkflowIdentifier

I was surprised that the IDE was fine with it.

@pyricau
Copy link
Member Author

pyricau commented Dec 6, 2024

To avoid the conflict, renamed the field to realType which is better anyway

pyricau and others added 2 commits December 6, 2024 02:24
Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@steve-the-edwards steve-the-edwards merged commit 32cb7bb into main Dec 6, 2024
30 checks passed
@steve-the-edwards steve-the-edwards deleted the py/nit_cleanups branch December 6, 2024 15:09
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.

4 participants