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

Korge memory split #2170

Merged
merged 12 commits into from Mar 1, 2024
Merged

Korge memory split #2170

merged 12 commits into from Mar 1, 2024

Conversation

holloszaboakos
Copy link
Contributor

No description provided.

Comment on lines 13 to 12
private var _size: Int = size
var _size: Int = size
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be exposed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the problem with the half structure. If I do not expose code like this, I can't make those functions extention functions. This was my problem in the original PR. I need some help with it, because I don't know how the Half stuff works and why.

Comment on lines 38 to 37
private fun ensureCount(count: Int) {
fun ensureCount(count: Int) {
Copy link
Member

Choose a reason for hiding this comment

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

Ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ditto

Comment on lines 9 to 14
fun ByteArrayBuilder.f16(v: Half, little: Boolean): ByteArrayBuilder = this.apply { prepare(2) { data.setF16(_size, v, little) } }
fun ByteArrayBuilder.f16LE(v: Half): ByteArrayBuilder = this.apply { prepare(2) { data.setF16LE(_size, v) } }
fun ByteArrayBuilder.f16BE(v: Half): ByteArrayBuilder = this.apply { prepare(2) { data.setF16BE(_size, v) } }

fun ByteArrayBuilderLE.f16(v: Half): ByteArrayBuilder = bab.f16LE(v)
fun ByteArrayBuilderBE.f16(v: Half): ByteArrayBuilder = bab.f16BE(v)
Copy link
Member

Choose a reason for hiding this comment

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

public inline class Half(public val rawBits: UShort) {

We can use s16 and Half.rawBits to expose these methods without exposing the private functionality.

For example:

fun ByteArrayBuilder.f16(v: Half, little: Boolean): ByteArrayBuilder = s16(v.rawBits, little)

Analogously the rest of the methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see! This solves it, thanks!

Comment on lines 6 to 10
fun ByteArrayReader.f16(little: Boolean): Half = move(2) { getF16(it, little) }
fun ByteArrayReader.f16LE(): Half = move(2) { getF16LE(it) }
fun ByteArrayReader.f16BE(): Half = move(2) { getF16BE(it) }
fun ByteArrayReaderLE.f16(): Half = bar.f16LE()
fun ByteArrayReaderBE.f16(): Half = bar.f16BE()
Copy link
Member

Choose a reason for hiding this comment

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

To not expose move:

fun ByteArrayReader.f16(little: Boolean): Half = Half.fromBits(s16(little))
// ...

So in general, we use s16 (Short type) methods, and convert back and forth with Half.fromBits and half.rawBits.toShort()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice!

Copy link
Member

@soywiz soywiz left a comment

Choose a reason for hiding this comment

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

I'm fine with extracting the Half functionality from memory, to not add that extra dependency. But private functionaluty shouldn't be exposed in the process.

As commented Half.rawBits.toShort and Half.fromBits using with s16 variants should do the trick.

Comment on lines +14 to +21
commonMainApi(libs.kotlinx.coroutines.core)
commonMainApi(libs.kotlinx.atomicfu)
commonTestApi(libs.kotlinx.coroutines.test)
commonMainApi(project(":korlibs-math-core"))
commonMainApi(project(":korlibs-platform"))
commonMainApi(project(":korlibs-util"))
commonMainApi(project(":korlibs-crypto"))
}
Copy link
Member

@soywiz soywiz Mar 1, 2024

Choose a reason for hiding this comment

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

We should evaluate those.

  • kotlinx-coroutines are not used at all
  • math-core might be used, but maybe we can internally copy stuff to remove that dependency?
  • util and crypto probably not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

util and math-core are used. We should avoid copiing stuff to private. I check the other two

Copy link
Member

Choose a reason for hiding this comment

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

Okay, let's keep it as it is for now. We can optimize dependencies later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could not remove any of them

Comment on lines +5 to +7
private fun ByteArray.u8(offset: Int): Int = this[offset].toInt() and 0xFF
private inline fun ByteArray.get16LE(offset: Int): Int = (u8(offset + 0) shl 0) or (u8(offset + 1) shl 8)
private inline fun ByteArray.get16BE(offset: Int): Int = (u8(offset + 1) shl 0) or (u8(offset + 0) shl 8)
Copy link
Member

@soywiz soywiz Mar 1, 2024

Choose a reason for hiding this comment

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

Aren't these exposed publicly already in other file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I dont know about that. They were private in the original class

@soywiz soywiz merged commit 6fdb9c2 into korlibs:main Mar 1, 2024
9 checks passed
@soywiz
Copy link
Member

soywiz commented Mar 1, 2024

Thanks!

@holloszaboakos holloszaboakos deleted the korge-memory-split branch March 6, 2024 19:55
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.

None yet

2 participants