-
Notifications
You must be signed in to change notification settings - Fork 9.2k
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
Embed the public suffix database list directly inside a class #8589
Conversation
Seems to kill the kotlin compiler Maybe https://youtrack.jetbrains.com/issue/KT-72429/StackOverflowError-when-compiling-large-files or |
Base64 will added 4/3 overhead, and some decoding, but avoid issues with java class files.
I shouldn't have uncompressed it. Will compare the compressed version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll review the rest on desktop. Too dense for mobile.
One alternative is to leave the existing implementation in src/main as the default, until we get a version that we are happy with and then switch. |
@@ -338,7 +239,7 @@ class PublicSuffixDatabase internal constructor( | |||
low = mid + end + 1 | |||
} else { | |||
// Found a match. | |||
match = String(this, mid, publicSuffixLength) | |||
match = this.substring(mid, mid + publicSuffixLength).string(Charsets.UTF_8) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like string
should have an overload which takes start/end indices to avoid this double copy. Or we should have added ByteString.slice
!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will raise a PR.
override fun ensureLoaded() { | ||
} | ||
|
||
val compressedBytes: ByteString = $publicSuffixListBytes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about a nullable var
and setting to null once read?
|
||
override val exceptionBytes: ByteString | ||
|
||
init { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not done in ensureLoaded
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I figured the JVM classloader would be a nice simple mutex. Ther s so much locking code in the other impl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I particularly don’t like this. We shouldn’t need to degrade good user’s runtime performance and artifact sizes because bad users are clumsy and trip on every rake.
Is there a way to load resources on Android that isn’t horrible for performance?! |
I can benchmark
Although my money is on this being better for performance than resources, just maybe at a small runtime memory cost. |
Can we do assets on Android and resources on the JVM? OkHttp 5 can be multiplatform with just Android and JVM platforms, right? |
This went a bit pear shaped... But exploring the space :) |
I'll check the multiplatform build option next. |
hopefully go with multiplatform following something like https://jeroenmols.com/blog/2021/03/17/share-code-kotlin-multiplatform/ And avoid the com.android.library plugin |
Made an attempt #8600 We would need that other PR above in order to get a Context to load from assets. So that other change is worth benchmarking first. |
Attempt a fix for #8580
Removes the need for proguard for the database. At the cost of more costly code generation at build time.
Point at which we parse the file
Generated source looks like, and both the source and class file are 57KB.