[K/N] Make the full build of a CustomBitSet lazy#5735
[K/N] Make the full build of a CustomBitSet lazy#5735troelsbjerre wants to merge 1 commit intomasterfrom
Conversation
Code Owners
|
...ative/compiler/ir/backend.native/src/org/jetbrains/kotlin/backend/konan/util/CustomBitSet.kt
Outdated
Show resolved
Hide resolved
...ative/compiler/ir/backend.native/src/org/jetbrains/kotlin/backend/konan/util/CustomBitSet.kt
Show resolved
Hide resolved
...ative/compiler/ir/backend.native/src/org/jetbrains/kotlin/backend/konan/util/CustomBitSet.kt
Outdated
Show resolved
Hide resolved
|
I observe a roughly 25% speedup on a reasonably large KMP project. Good job! 🎉 |
|
So far the biggest question I have is how clearing a bi should be handled. When the bits are only added everything is clear, and there are some nice invariants we could rely on. When a bit is cleared then suddenly we can get same bitsets with different internal representations. That complicated stuff quite a bit. What do you think? Did not convert a bitset back to the lazy for as shrinks intentionally? |
|
I don't think the efficiency loss of enforcing an invariant like "for cardinality < k, it's always an |
...ative/compiler/ir/backend.native/src/org/jetbrains/kotlin/backend/konan/util/CustomBitSet.kt
Outdated
Show resolved
Hide resolved
...ative/compiler/ir/backend.native/src/org/jetbrains/kotlin/backend/konan/util/CustomBitSet.kt
Outdated
Show resolved
Hide resolved
...ative/compiler/ir/backend.native/src/org/jetbrains/kotlin/backend/konan/util/CustomBitSet.kt
Show resolved
Hide resolved
...ative/compiler/ir/backend.native/src/org/jetbrains/kotlin/backend/konan/util/CustomBitSet.kt
Outdated
Show resolved
Hide resolved
...ative/compiler/ir/backend.native/src/org/jetbrains/kotlin/backend/konan/util/CustomBitSet.kt
Show resolved
Hide resolved
...ative/compiler/ir/backend.native/src/org/jetbrains/kotlin/backend/konan/util/CustomBitSet.kt
Outdated
Show resolved
Hide resolved
...ative/compiler/ir/backend.native/src/org/jetbrains/kotlin/backend/konan/util/CustomBitSet.kt
Show resolved
Hide resolved
...ative/compiler/ir/backend.native/src/org/jetbrains/kotlin/backend/konan/util/CustomBitSet.kt
Show resolved
Hide resolved
...ative/compiler/ir/backend.native/src/org/jetbrains/kotlin/backend/konan/util/CustomBitSet.kt
Show resolved
Hide resolved
| @@ -5,20 +5,33 @@ | |||
|
|
|||
| package org.jetbrains.kotlin.backend.konan.util | |||
|
|
|||
| import it.unimi.dsi.fastutil.ints.IntArraySet | |||
| import it.unimi.dsi.fastutil.ints.IntSet | |||
There was a problem hiding this comment.
Probably fine to use, will double check
There was a problem hiding this comment.
It's worth noting that the fastutils library is used elsewhere in the compiler. This is not a new dependecy.
...ative/compiler/ir/backend.native/src/org/jetbrains/kotlin/backend/konan/util/CustomBitSet.kt
Show resolved
Hide resolved
...ative/compiler/ir/backend.native/src/org/jetbrains/kotlin/backend/konan/util/CustomBitSet.kt
Show resolved
Hide resolved
...ative/compiler/ir/backend.native/src/org/jetbrains/kotlin/backend/konan/util/CustomBitSet.kt
Show resolved
Hide resolved
...ative/compiler/ir/backend.native/src/org/jetbrains/kotlin/backend/konan/util/CustomBitSet.kt
Show resolved
Hide resolved
...ative/compiler/ir/backend.native/src/org/jetbrains/kotlin/backend/konan/util/CustomBitSet.kt
Show resolved
Hide resolved
| /** | ||
| * Provides some bulk operations needed for devirtualization | ||
| */ | ||
| internal class CustomBitSet private constructor(size: Int, data: LongArray) { | ||
| var size = size | ||
| private set | ||
| private var data = data | ||
| private var lazy: IntSet? = IntArraySet() |
There was a problem hiding this comment.
[major] When using the private contructor the lazy property still gets initialized. This leads to effective "empty" lazy bitset despite the data array that was passed.
This affects the copy method and the valueOf companion method
|
You may also be interested in using a library such as https://github.com/RoaringBitmap/RoaringBitmap. As I get it, it implements an optimized bitmap with a dynamic underlying representation, similar to the one proposed here. |
I tried it back in August, didn't observe any benefit for some reason 🤷 . Maybe I used it wrong or something. |
I'll do a quick test to see if there is a noticeable difference. I suspect the general RoaringBitMap might be at a disadvantage for the common case of very sparse BitSets, with only a handful of set bits. |
|
Changing from CustomBitSet to RoaringBitmaps, the peak memory usage drops by 5%, but the link step takes 11% longer. |
This changes the representation of CustomBitSet to initially be an explicit list of set bits, until the cardinality grows beyond a fixed size. This saves a lot of memory, since many of the use cases of CustomBitSet are sparse.
174530a to
db4252a
Compare
This changes the representation of CustomBitSet to initially be an explicit list of set bits, until the cardinality grows beyond a fixed size. This saves a lot of memory, since many of the use cases of CustomBitSet are sparse.