Skip to content

Functions inline #1111

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

Merged
merged 6 commits into from
Apr 11, 2025
Merged

Functions inline #1111

merged 6 commits into from
Apr 11, 2025

Conversation

AndreiKingsley
Copy link
Collaborator

Closes #1026

@@ -64,22 +64,23 @@ private interface CommonSelectDocs
*/
@Refine
@Interpretable("Select0")
public fun <T> DataFrame<T>.select(columns: ColumnsSelector<T, *>): DataFrame<T> = get(columns).toDataFrame().cast()
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's seems to be no benefit in inline for select, so let's maybe keep it a regular function

@@ -135,6 +135,8 @@ public operator fun <T> DataFrame<T>.get(columnRange: ClosedRange<String>): Data

internal val ColumnsContainer<*>.ncol get() = columnsCount()
internal val AnyFrame.nrow get() = rowsCount()

@PublishedApi
internal val AnyFrame.indices get() = indices()
Copy link
Collaborator

Choose a reason for hiding this comment

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

inline functions that use this internal properties can use already public functions instead

@Jolanrensen Jolanrensen self-requested a review March 27, 2025 17:17
Copy link
Collaborator

@Jolanrensen Jolanrensen left a comment

Choose a reason for hiding this comment

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

Could you do another run over the changes to see if crossinline is needed? I encountered a lot of cases already and crossinline blocks non-local returns, while allowing those is the main goal of the PR. crossinline is only needed for lambdas that need to be executed in a non-inlined scope

@@ -25,7 +25,7 @@ internal class JoinedDataRowImpl<A, B>(
override val right: DataRow<B> = DataRowImpl(index1, rightOwner)
}

internal fun <A, B> DataFrame<A>.joinWithImpl(
internal inline fun <A, B> DataFrame<A>.joinWithImpl(
Copy link
Collaborator

@Jolanrensen Jolanrensen Mar 31, 2025

Choose a reason for hiding this comment

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

hmm I'm doubting the benefits outweigh the downsides for inlining a function as large as this. Remember inlining copies over everthing in this function to the execution site increasing code size and compile time for the user. @koperagen, what do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree, let's keep it a regular fun

isNumber() -> cast<Number?>().map {
if (it?.isPrimitiveNumber() == false) {
// Cannot calculate statistics of a non-primitive number type
isNumber() -> with(cast<Number?>()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd revert this back to map with non-local break. Then all values are visited just once, now they are visited twice.

converter: (List<ColumnWithPath<A>>) -> List<ColumnWithPath<B>>,
@PublishedApi
internal inline fun <A, B> ColumnsResolver<A>.transform(
crossinline converter: (List<ColumnWithPath<A>>) -> List<ColumnWithPath<B>>,
): TransformableColumnSet<B> =
object : TransformableColumnSet<B> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

remember me someday to remove this TransformableColumnSet stuff. It was the result of .recursively(). I have a hunch we can inline more if transformResolve doesn't exist anymore :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Then maybe add an issue 😄?

@@ -60,8 +60,8 @@ public fun <T> DataFrame<T>.takeLast(n: Int = 1): DataFrame<T> {
/**
* Returns a DataFrame containing first rows that satisfy the given [predicate].
*/
public fun <T> DataFrame<T>.takeWhile(predicate: RowFilter<T>): DataFrame<T> =
firstOrNull { !predicate(it, it) }?.let { take(it.index) } ?: this
public inline fun <T> DataFrame<T>.takeWhile(crossinline predicate: RowFilter<T>): DataFrame<T> =
Copy link
Collaborator

Choose a reason for hiding this comment

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

crossinline unneeded

@@ -11,7 +11,7 @@ public fun <T> DataFrame<T>.duplicate(n: Int): FrameColumn<T> = List(n) { this }

public fun <T> DataFrame<T>.duplicateRows(n: Int): DataFrame<T> = duplicateRowsImpl(n)

public fun <T> DataFrame<T>.duplicateRows(n: Int, filter: RowFilter<T>): DataFrame<T> =
public inline fun <T> DataFrame<T>.duplicateRows(n: Int, crossinline filter: RowFilter<T>): DataFrame<T> =
Copy link
Collaborator

Choose a reason for hiding this comment

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

crossinline unneeded


/**
* Returns a DataFrame containing all rows except first rows that satisfy the given [predicate].
*/
public fun <T> DataFrame<T>.dropWhile(predicate: RowFilter<T>): DataFrame<T> =
firstOrNull { !predicate(it, it) }?.let { drop(it.index) } ?: this
public inline fun <T> DataFrame<T>.dropWhile(crossinline predicate: RowFilter<T>): DataFrame<T> =
Copy link
Collaborator

Choose a reason for hiding this comment

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

crossinline unneeded

@@ -205,7 +205,7 @@ public fun <T> DataRow<T>.relative(relativeIndices: IntRange): DataFrame<T> =
(relativeIndices.first + index).coerceIn(df().indices)..(relativeIndices.last + index).coerceIn(df().indices),
)

public fun <T> DataRow<T>.movingAverage(k: Int, expression: RowExpression<T, Number>): Double {
public inline fun <T> DataRow<T>.movingAverage(k: Int, crossinline expression: RowExpression<T, Number>): Double {
Copy link
Collaborator

Choose a reason for hiding this comment

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

crossinline unneeded

@@ -199,9 +199,9 @@ public interface ColsOfKindColumnsSelectionDsl {
* @param filter The filter function to apply on each column. Must accept a ColumnWithPath object and return a Boolean.
* @return A [TransformableColumnSet] containing the columns of given kinds that satisfy the filter.
*/
internal fun ColumnsResolver<*>.columnsOfKindInternal(
internal inline fun ColumnsResolver<*>.columnsOfKindInternal(
Copy link
Collaborator

Choose a reason for hiding this comment

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

currently there's no benefit for having these CS DSL internal functions inlined, because they are only called from non-inlined functions. However, I think they can stay inlined, because it reminds us we can move the CS DSL to a fully inlined context-parameter based API in the future :)

@@ -262,7 +262,10 @@ public inline fun <reified C> SingleColumn<DataRow<*>>.colsOf(
* match the given [filter] and are the given [type].
*/
@Suppress("UNCHECKED_CAST")
internal fun <C> ColumnsResolver<*>.colsOfInternal(type: KType, filter: ColumnFilter<C>): TransformableColumnSet<C> =
internal inline fun <C> ColumnsResolver<*>.colsOfInternal(
Copy link
Collaborator

Choose a reason for hiding this comment

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

there are a couple more functions that be inline in this file

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Which ones?

@koperagen
Copy link
Collaborator

Looking at changes that make some internal functions inline, can you please verify size of our core jar in core/build/libs before and after this change? Let's make sure we don't accidentally affect it too much

# Conflicts:
#	core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/api/max.kt
#	core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/api/min.kt
#	core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/Utils.kt
@AndrewKis AndrewKis merged commit 5bc9783 into master Apr 11, 2025
7 checks passed
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.

Functions that cán be inlined should be inlined
4 participants