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

Remove fastutil dependency #155

Merged
merged 1 commit into from
May 24, 2024
Merged

Conversation

ajalt
Copy link
Contributor

@ajalt ajalt commented Apr 8, 2024

As shown in ajalt/clikt#507, the fastutil dependency is over 6MB in size. This dependency is only used in a single line.

This PR removes that dependency and uses the same Stack<Int>() on JVM as on the other platforms.

To make sure this doesn't affect performance, I used the following JMH benchmark that converts the gitBook.md file from the test data to html:

val parser = MarkdownParser(CommonMarkFlavourDescriptor())
val src = Path.of("../src/fileBasedTest/resources/data/html/gitBook.md").readText()

@Warmup(iterations = 5, time = 1)
@Measurement(iterations = 5, time = 1)
@OutputTimeUnit(TimeUnit.MILLISECONDS)
@BenchmarkMode(Mode.AverageTime)
@Fork(1)
@State(Scope.Benchmark)
open class GitBookBenchmark {
    @Benchmark
    open fun htmlGenerator(): String {
        val parsedTree = parser.buildMarkdownTreeFromString(src)
        return HtmlGenerator(src, parsedTree, CommonMarkFlavourDescriptor()).generateHtml()
    }
}

Performance on master, with fastutil IntStack:

Benchmark                       Mode  Cnt   Score   Error  Units
GitBookBenchmark.htmlGenerator  avgt    5  19.293 ± 3.356  ms/op

Performance after this PR, with Stack<Int>():

Benchmark                       Mode  Cnt   Score   Error  Units
GitBookBenchmark.htmlGenerator  avgt    5  19.288 ± 7.111  ms/op

According to this benchmark, the change has no performance impact.

@KvanTTT
Copy link
Contributor

KvanTTT commented Apr 9, 2024

Interesting.

As shown in ajalt/clikt#507, the fastutil dependency is over 6MB in size. This dependency is only used in a single line.

Moreover, it isn't compatible with K2 if use KMP, see https://youtrack.jetbrains.com/issue/KT-66723 (but I've written a workaround).


actual class BitSet actual constructor(size: Int): java.util.BitSet(size){
actual val size = size()
}

actual typealias IntStack = IntArrayList
actual class IntStack: Stack<Int>()
Copy link
Contributor

Choose a reason for hiding this comment

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

I think if you replace IntArrayList with Stack<Int>, actual/expect classes become redundant and they can be replaced just with Stack<Int>. It's declared in a common source set and it's currently used for js and native.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, i chose to leave it since IntStack is used in generated code, I wanted to keep the PR smaller. But I can make that change if one of the maintainers wants me to.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense.

@Jerbell
Copy link

Jerbell commented May 22, 2024

Looking forward to this 🤟 (trying to use Clikt in Lambda function)

@Jerbell
Copy link

Jerbell commented May 24, 2024

All hail the good folk in the Markdown team. Please consider merging this lovely request & doing a new release 😅

@berezhkoE berezhkoE merged commit 22b162e into JetBrains:master May 24, 2024
3 checks passed
@ajalt ajalt deleted the remove-fastutil branch May 24, 2024 16:57
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

4 participants