Sqldelight: Runtime performance: trimMargin

[Dkhusainov]

In the current implementation(1.0.3) queries are ran through trimMargin function on each execution, which leads to completely unnecesarry allocations and resource usage.

I understands that it's not much, but why do it in the first place when it doesn't provide any benefit from doing so? If someone want's to read the query in debug, he can just copy it and run through some sqlite formatter. It's especially bad if you're doing many small and formatted queries.

It's even worse on native, since kotlin collections and some operations with them are slow (compared to jvm or other native languages).

[JakeWharton]

Have you actually measured this to be a problem or are you speculating? You should voice your support for https://youtrack.jetbrains.com/issue/KT-17755 which will make this a non-issue.

[Dkhusainov]

Obviously I can't measury something this insignificant(but not nothing). I'm talking about the need to have trimMargin at all, when it doesn't do anything except preformats sql for reading in debug.

About the issue: me writing a comment on youtrack will cause them to take on a two year old issue? Doubt it. Minor performance improvements like that are never looked on, since there are always issues with more priority.

[JakeWharton]

The use of trimMargin is added automatically by KotlinPoet, not explicitly by us. It's actually non-trivial to avoid this behavior. I'd much rather see the problem fixed in the Kotlin compiler than convincing everyone who is using trimMargin or trimIndent to stop using it.

[Dkhusainov]

I'm not talking about advocating for stopping the use of trimMargin, just
removing it from sqldelight generated code. It surely won't hurt anyone or
break anything.

If you say it has no use case and trivial to get rid of, I don't see a
reason why it should stay.

[JakeWharton]

Ah, unfortunately it's non-trivial to remove because we aren't adding it. It's generated automatically by the library we use to generate code, KotlinPoet.

I'd rather make a strong push for advocating for it to be made a compiler intrinsic first. This way we improve the performance and reduce the APK size of everyone who is using it rather than just helping the tiny subset of people who are using SQL Delight.

[Dkhusainov]

Yes, it would be great if they did that. But as I said, minor performance issues like that are rarely if ever fixed. JB obviously doesn't have enough resources for something like that, seeing as they have: 3 platforms, mpp, Android part of kotlin, kapt, coroutines, new IR, and other things that I forgot. Do so you see this issue being a priority over the things I've listed?

How is implementing it non-trival? Adding explicit option to Kotlin Poet whether to use trimMargin or not, and then use it in the generator. Or am i missing something?

[JakeWharton]

If we make it a priority, yes. A defeatist attitude towards it certainly will never see it implemented.

We don't want to add that option to KotlinPoet because you should never need to know or care what is being used. The language should just make it free.

[JakeWharton]

The wasted characters to support trimMargin account for 0.03919103523% of the dex size of the Square Cash production apk as pulled from my phone.

Queries are actually cached so the runtime performance impact of trimMargin is only incurred when a new connection needs to execute the query for the first time. After that, the trimmed string becomes a parsed AST inside of sqlite and the generated code looks it up with an integer key.

I don't see much justification for removing trimMargin as a result. I hoped the dex impact was larger so we could use it to convince JetBrains but in a real app it's effectively meaningless in terms of overhead. I will continue to advocate for the compiler intrinsic, though.

We would have far greater impact on APK size and runtime performance from removing extra lambdas we generate or making some of the data structures in use more efficient.

[Dkhusainov]

_Queries are actually cached so the runtime performance impact of trimMargin is only incurred when a new connection needs to execute the query for the first time_

You are wrong, Only sqlite bytecode is cached inside a statement object, sql string is evaluated on each query execution(before a call to SqlDriver.executeQuery and so trimMargin is executoed on the whole string on each query. You can see this easily if you set a breakpoint inside String.replaceIndentByMargin function.

[JakeWharton]

It was lazy at one point. I think we may have lost that with the changes to the architecture of the driver in the lead up to 1.0.

In any case, that's just more of an argument for making it a compiler intrinsic. So you should voice your support on YouTrack.

[Dkhusainov]

I'm a realist and I don't think this will do anything for the reasons stated above.

As for the overhead problem, I'll just make all of my heavy queries a one-liners.

[JakeWharton]

Well not doing anything definitely won't do anything! Yep.

On Wed, Mar 6, 2019 at 4:33 PM Dkhusainov [e-mail] wrote:

I'm a realist and I don't think this will do anything for the reasons
stated above.

As for the overhead problem, I'll just make all of my heavy queries a
one-liners.

—
You are receiving this because you modified the open/close state.

Reply to this email directly, view it on GitHub
https://github.com/square/sqldelight/issues/1265#issuecomment-470285573,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAEEEbVgF97z_U8UW1Y6IrHGr8FC1Asyks5vUDQQgaJpZM4bhhDJ
.

[JakeWharton]

Last night after putting the baby to bed I took a shot at implementing this
intrinsic in the Kotlin compiler. I got it working for margin and indent
for non-interpolated strings only. Sending a PR to the compiler later today
after beefing up the tests.

On Wed, Mar 6, 2019, 4:34 PM Jake Wharton <[email protected] wrote:

Well not doing anything definitely won't do anything! Yep.

On Wed, Mar 6, 2019 at 4:33 PM Dkhusainov [e-mail]
wrote:

I'm a realist and I don't think this will do anything for the reasons
stated above.

As for the overhead problem, I'll just make all of my heavy queries a
one-liners.

—
You are receiving this because you modified the open/close state.

Reply to this email directly, view it on GitHub
https://github.com/square/sqldelight/issues/1265#issuecomment-470285573,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAEEEbVgF97z_U8UW1Y6IrHGr8FC1Asyks5vUDQQgaJpZM4bhhDJ
.

[JakeWharton]

This was merged and will ship in Kotlin 1.3.40.

On Thu, Mar 7, 2019 at 9:41 AM Jake Wharton [e-mail] wrote:

Last night after putting the baby to bed I took a shot at implementing
this intrinsic in the Kotlin compiler. I got it working for margin and
indent for non-interpolated strings only. Sending a PR to the compiler
later today after beefing up the tests.

On Wed, Mar 6, 2019, 4:34 PM Jake Wharton <[email protected] wrote:

Well not doing anything definitely won't do anything! Yep.

On Wed, Mar 6, 2019 at 4:33 PM Dkhusainov [e-mail]
wrote:

I'm a realist and I don't think this will do anything for the reasons
stated above.

As for the overhead problem, I'll just make all of my heavy queries a
one-liners.

—
You are receiving this because you modified the open/close state.

Reply to this email directly, view it on GitHub
https://github.com/square/sqldelight/issues/1265#issuecomment-470285573,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAEEEbVgF97z_U8UW1Y6IrHGr8FC1Asyks5vUDQQgaJpZM4bhhDJ
.