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

The number of exceptions thrown should be reduced. This may affect performance. #146

Open
ShiinaSekiu opened this issue Sep 30, 2024 · 7 comments
Labels
enhancement New feature or request

Comments

@ShiinaSekiu
Copy link
Contributor

/**
* Returns the next child held by this structure. Useful for iterating over its children when parsing complex structures.
* @throws [Asn1StructuralException] if no more children are available
*/
@Throws(Asn1StructuralException::class)
fun nextChild() =
catching { children[index++] }.getOrElse { throw Asn1StructuralException("No more content left") }
/**
* Exception-free version of [nextChild]
*/
fun nextChildOrNull() = catching { nextChild() }.getOrNull()

        /**
         * Returns the next child held by this structure. Useful for iterating over its children when parsing complex structures.
         * @throws [Asn1StructuralException] if no more children are available
         */
        @Throws(Asn1StructuralException::class)
        fun nextChild() = children.getOrElse(index++) { throw Asn1StructuralException("No more content left") }

        /**
         * Exception-free version of [nextChild]
         */
        fun nextChildOrNull() = children.getOrNull(index++)
@JesusMcCloud
Copy link
Collaborator

Thank you very much! There are probably a handful of locations, where it is possible to get around throwing and catching.
The more pressing issue wrt. performance is not rooted in this project, though, but in KmmResult, which cannot be a value class, as it would make it impossible to export it to an XC framework. Thus, catching incurs a performance hit. At the same time runCatching catches more than it should, so we require catching.

@ShiinaSekiu
Copy link
Contributor Author

By the way, why doesn't the peek() method of this class have a corresponding peekOrNull() method?

@JesusMcCloud
Copy link
Collaborator

JesusMcCloud commented Sep 30, 2024

I'll note that down for improving API docs. peek already returns null, if no more children are left. All the feedback you have provided so far is really appreciated!

@JesusMcCloud
Copy link
Collaborator

The improved docs are part of #151

@JesusMcCloud
Copy link
Collaborator

KmmResult 1.9.0 will be part of the next release, significantly reducing overhead, even though the same number of exceptions are thrown.

@iaik-jheher
Copy link
Collaborator

KmmResult 1.9.0 will be part of the next release, significantly reducing overhead, even though the same number of exceptions are thrown.

This would require us to go through and review catching uses/convert them to catchingUnwrapped, to my understanding?

@JesusMcCloud
Copy link
Collaborator

I think we can close this when #229 is through. After all, the critical parts throwing exceptions are mostly IO (encoding/decoding) where you do want to throw, when things go south. I know this does not cover everything perfectly, because we also internally use public API methods that instantiate a KmmResult and don't care for it, but I'd say were 95% there.

If this issue remains stale, I'll close it

@JesusMcCloud JesusMcCloud added the enhancement New feature or request label Jan 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants