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

ByteArrayOutput should have a customizable initial capacity #78

Open
pm47 opened this issue Nov 4, 2022 · 4 comments
Open

ByteArrayOutput should have a customizable initial capacity #78

pm47 opened this issue Nov 4, 2022 · 4 comments

Comments

@pm47
Copy link
Member

pm47 commented Nov 4, 2022

It should be possible to define a custom initial size for the backing array of ByteArrayOutput, like the jdk impl. The initial size of 32B is tiny and results in dozens of reallocation+memcopies in the lightning-kmp serialization use case (and probably is similar within bitcoin-kmp). That can't be good for performance, I wonder how setting a reasonable capacity would reduce the test time.

@sstone
Copy link
Member

sstone commented Nov 7, 2022

Yes we should allow it, but a quick test with a default value of 32 Kb (instead of 32 bytes) did not speed things up at all.

@pm47
Copy link
Member Author

pm47 commented Nov 7, 2022

Is this on the jvm ? Is there a chance the difference is more notable on native iOS ?

@sstone
Copy link
Member

sstone commented Nov 7, 2022

I ran linuxTest (on linux) and performance did not improve at all. I don't think it would be much different on iOS (but not completely sure).
The test I'm using is:

./gradlew :cleanLinuxTest :linuxTest --tests "fr.acinq.lightning.channel.states.NormalTestsCommon"

which should imho highlight serialization-related performance issues. I noticed too that lightning-kmp tests are slower now with the new serialization but I'm not sure it is I/O related. We had severe performance issues on bitcoin-kmp (#72) and the root cause was not I/O but the poor performance of kotlin collections, though I don't think it's the same problem with lightning-kmp because the performance difference between native and jvm tests is not as significant.

@pm47
Copy link
Member Author

pm47 commented Nov 7, 2022

I noticed too that lightning-kmp tests are slower now with the new serialization

We do serialize (bin+json) at all state transitions now (compared to only bin for some transitions), might explain the longer test time. I guess, if there is no measurable performance improvement, this can be left as-is.

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

No branches or pull requests

2 participants