Native (Kotlin): improve performance #2
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ilya-g
reviewed
Apr 22, 2020
ilya-g
reviewed
Apr 22, 2020
dkhalanskyjb
force-pushed
the
native
branch
2 times, most recently
from
ef0a902 to
54a8d39
Compare
ilya-g
reviewed
Apr 27, 2020
ilya-g
reviewed
Apr 27, 2020
| require(secondOfDay in 0..SECONDS_PER_DAY) | ||
| require(nanoOfSecond in 0..1_000_000_000) | ||
| require(secondOfDay >= 0 && secondOfDay <= SECONDS_PER_DAY) | ||
| require(nanoOfSecond >= 0 && nanoOfSecond <= 1_000_000_000) |
There was a problem hiding this comment.
Needs an issue reference about the missing optimization in Native.
There was a problem hiding this comment.
Created an issue.
There was a problem hiding this comment.
Please put a reference to it here, that would explain why the non-idiomatic code is used.
Fixes include: * The system timezone is only queried once, after that, it's cached for the whole life of the thread; Java does something similar, as, according to https://docs.oracle.com/javase/7/docs/api/java/util/TimeZone.html#setDefault(java.util.TimeZone), the system timezone is only queried at the start of the VM, if ever. * Most common `ZoneOffset` instances are now cached, which saves a lot of time due to not having to recompute their string representation, which is costly in aggregate due to how common getting anonymous offsets is for operations on datetimes. * Strings are not converted between UTF-16 and UTF-8 when querying offsets for a particular timezone (the `noStringConversion` cinterop parameter handles this). * Comparators have been rewritten not to use `compareBy`, which is comparatively slow. * `LocalDate` no longer uses `Month` as its main representation of months, as it is an enum, which are surprisingly slow. * `LocalDate` used to calculate many additional fields which may never be required but were nonetheless computed for each instance of the class. Now, instantiating `LocalDate` is much cheaper at the cost of several additional fields being computed on each call. After these fixes, on my machine tests run in 25 seconds instead of 56.
The tactics employed in this commit are questionable. The main change is that now, instead of passing strings between Kotlin and native parts, numeric ids are used. To have a sensible interpretation of such ids that prohibited their invalidation, Apple and Windows use additional memory to store the mapping. Moreover, it is assumed for Apple that the set of timezones and their rules never change, and for *nix, it is assumed that the timezone objects are always at the same memory location, which is true for now but will change if the implementation switches to use C++20 capabilities.
dkhalanskyjb
force-pushed
the
native
branch
2 times, most recently
from
4d8159d to
869bba9
Compare
Testing revealed that accessing even immutable fields concurrently is not supported out-of-the-box and instead crashes the Kotlin Native runtime.
ilya-g
approved these changes
Apr 30, 2020
dkhalanskyjb
added a commit
that referenced
this pull request
Apr 30, 2020
Testing revealed that accessing even immutable global variables concurrently is not supported out-of-the-box and instead crashes the Kotlin Native runtime. Thus, `SharedImmutable` annotation was added to every global variable. Performance fixes include: * The system timezone is only queried once, after that, it's cached for the whole life of the thread; Java does something similar, as, according to https://docs.oracle.com/javase/7/docs/api/java/util/TimeZone.html#setDefault(java.util.TimeZone), the system timezone is only queried at the start of the VM, if ever. * Instead of passing strings between Kotlin and native parts, numeric ids are used. To have a sensible interpretation of such ids that prohibited their invalidation, Apple and Windows use additional memory to store the mapping. Moreover, it is assumed for Apple that the set of timezones and their rules never change, and for *nix, it is assumed that the timezone objects are always at the same memory location, which is true for now but will change if the implementation switches to use C++20 capabilities. * Most common `ZoneOffset` instances are now cached, which saves a lot of time due to not having to recompute their string representation, which is costly in aggregate due to how common getting anonymous offsets is for operations on datetimes. * Comparators have been rewritten not to use `compareBy`, which is comparatively slow. * `LocalDate` no longer uses `Month` as its main representation of months, as it is an enum, which are surprisingly slow. * `LocalDate` used to calculate many additional fields which may never be required but were nonetheless computed for each instance of the class. Now, instantiating `LocalDate` is much cheaper at the cost of several additional fields being computed on each call. * Some Kotlin-style range checks were replaced with the corresponding simple comparisons due to a missing optimization in Kotlin/Native. See #5 After these fixes, on my machine tests run in 22 seconds instead of 56.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes include:
for the whole life of the thread; Java does something similar,
as, according to
https://docs.oracle.com/javase/7/docs/api/java/util/TimeZone.html#setDefault(java.util.TimeZone),
the system timezone is only queried at the start of the VM, if
ever.
ZoneOffsetinstances are now cached, which savesa lot of time due to not having to recompute their string
representation, which is costly in aggregate due to how common
getting anonymous offsets is for operations on datetimes.
offsets for a particular timezone (the
noStringConversioncinterop parameter handles this).
compareBy, which iscomparatively slow.
LocalDateno longer usesMonthas its main representation ofmonths, as it is an enum, which are surprisingly slow.
LocalDateused to calculate many additional fields which maynever be required but were nonetheless computed for each instance
of the class. Now, instantiating
LocalDateis much cheaper atthe cost of several additional fields being computed on each call.
After these fixes, on my machine tests run in 25 seconds instead of
56.