Fix #1435: Cache instances of j.l.Class #1894
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
|
Looks like this change introduced significant regression in performance. CI jobs takes in debug mode around 15-25% longer and in release-fast it take ~50% more time. Due to this 3 last CI jobs has timed out (there's a limit of 50 minutes). I'll take a look at this |
|
Do you think it might be helpful to have some diagrams showing the runtime layout? Isn't one of the big advantages over the JVM is that the JVM carries a bunch of overhead in each object? I don't want to slow down the release maybe a follow on issue if you agree. |
sjrd
requested changes
Oct 7, 2020
nativelib/src/main/scala/java/lang/ClassInstancesRegistry.scala
Outdated
Show resolved
Hide resolved
nativelib/src/main/scala/java/lang/ClassInstancesRegistry.scala
Outdated
Show resolved
Hide resolved
nativelib/src/main/scala/java/lang/ClassInstancesRegistry.scala
Outdated
Show resolved
Hide resolved
nativelib/src/main/scala/java/lang/ClassInstancesRegistry.scala
Outdated
Show resolved
Hide resolved
nativelib/src/main/scala/java/lang/ClassInstancesRegistry.scala
Outdated
Show resolved
Hide resolved
WojciechMazur
force-pushed
the
feature/1435-cache_instances_of_j.l.Class
branch
from
fb06602
to
54398eb
Compare
sjrd
requested changes
Oct 8, 2020
WojciechMazur
force-pushed
the
feature/1435-cache_instances_of_j.l.Class
branch
from
54398eb
to
2bcc818
Compare
WojciechMazur
force-pushed
the
feature/1435-cache_instances_of_j.l.Class
branch
from
2bcc818
to
a6d1eea
Compare
|
This probably needs a rebase on top of master, now that #1946 has been merged. |
WojciechMazur
force-pushed
the
feature/1435-cache_instances_of_j.l.Class
branch
2 times, most recently
from
cdfb0ab
to
640b5d1
Compare
WojciechMazur
force-pushed
the
feature/1435-cache_instances_of_j.l.Class
branch
from
640b5d1
to
1e9c825
Compare
sjrd
requested changes
Nov 12, 2020
nativelib/src/main/scala/scala/scalanative/runtime/ClassInstancesRegistry.scala
Outdated
Show resolved
Hide resolved
WojciechMazur
force-pushed
the
feature/1435-cache_instances_of_j.l.Class
branch
from
1e9c825
to
4937ee3
Compare
sjrd
approved these changes
Nov 30, 2020
There was a problem hiding this comment.
@WojciechMazur could you rebase this on top of latest master, just to make sure it still works with the latest CI setup?
sjrd
changed the title
…l.ClassOf -> Object.getClass conversion
…e conversion in genVal for Val.String)
…tore class instances" This reverts commit 58cc6c6.
…opy instead of Array.copy
WojciechMazur
force-pushed
the
feature/1435-cache_instances_of_j.l.Class
branch
from
4937ee3
to
6a49537
Compare
ekrich
pushed a commit
to ekrich/scala-native
that referenced
this pull request
May 21, 2021
* 1435 - Move j.l.Class allocations to linker/runtime. Introduce caching of j.l.Classes * 1435 - Compare _Class instances using their rawPtr instead of Rt.Type ptr * 1435 - Updated immix/commix RTTI structures to contain additional class pointer * 1435 - Transform Vals in Lower with nir.Buffer in order to support Val.ClassOf -> Object.getClass conversion * Added classOf / Object.getClass tests * Use loadObject instead of loadrawPtr in Object.getClass * Workaround Object.getClass when clsPtr does not contain j.l.Class instance * Create registry for instances of j.l.Class to prevent GC from collecting them * Remove commented code * Lower.genMethodLookup uses original value to test targets (due to type conversion in genVal for Val.String) * Replace ClassInstancesRegistry Array with UnrolledBuffer to store class instances * Add Val.ClassOf support to NIR text parser * Add Object.getClass to Lower dependencies * Revert "Replace ClassInstancesRegistry Array with UnrolledBuffer to store class instances" This reverts commit 58cc6c6. * Move ClassInstancesRegistry to seperate file * Styling and access fixes to ClassInstancesRegistry. Use System.arraycopy instead of Array.copy * Move class caching and Rtti -> Class lookup to scalanative.runtime * After rebase fixes * Use same calling and styling convention for runtime.toClass as other Rt methods * After rebase fix * After rebase fix: Port new ClassSuite tests to JUnit * Comments styling fix * Simplify Class.is method * Fix comment
WojciechMazur
added a commit
to WojciechMazur/scala-native
that referenced
this pull request
Aug 25, 2021
* 1435 - Move j.l.Class allocations to linker/runtime. Introduce caching of j.l.Classes * 1435 - Compare _Class instances using their rawPtr instead of Rt.Type ptr * 1435 - Updated immix/commix RTTI structures to contain additional class pointer * 1435 - Transform Vals in Lower with nir.Buffer in order to support Val.ClassOf -> Object.getClass conversion * Added classOf / Object.getClass tests * Use loadObject instead of loadrawPtr in Object.getClass * Workaround Object.getClass when clsPtr does not contain j.l.Class instance * Create registry for instances of j.l.Class to prevent GC from collecting them * Remove commented code * Lower.genMethodLookup uses original value to test targets (due to type conversion in genVal for Val.String) * Replace ClassInstancesRegistry Array with UnrolledBuffer to store class instances * Add Val.ClassOf support to NIR text parser * Add Object.getClass to Lower dependencies * Revert "Replace ClassInstancesRegistry Array with UnrolledBuffer to store class instances" This reverts commit 58cc6c6. * Move ClassInstancesRegistry to seperate file * Styling and access fixes to ClassInstancesRegistry. Use System.arraycopy instead of Array.copy * Move class caching and Rtti -> Class lookup to scalanative.runtime * After rebase fixes * Use same calling and styling convention for runtime.toClass as other Rt methods * After rebase fix * After rebase fix: Port new ClassSuite tests to JUnit * Comments styling fix * Simplify Class.is method * Fix comment
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.
Resolves #1435
This PR moves allocation of java.lang.Class instances to linker phase and introduces caching of them.
In previous implementation Class instances were created by compiler backend, which lead to multiple instances of this type in runtime and made impossible to cache them.
Current implementation introduces new Val literal type
Val.ClassOf(n: Global)which replaces allocations from previous implementation. At lowering phase this literals are transformed intoObject.getClasscalls with argument of RTTI object (as previous). RTTI type was also extended to store reference of associated with it java.lang.Class. InsideObject.getClassit's checked whether RTTI class pointer is not empty and instantiated in otherwise. Newly created instance of j.l.Class is added to registry in order to prevent GC from collecting it.Immix/Commix implementation was fixed to handle new RTTI layout.