Optimize TotalWrapper #64
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
isaacl
force-pushed
the
optimizeTotalWrapper
branch
from
24d41a0 to
ec0466a
Compare
|
ready for review, I guess |
isaacl
force-pushed
the
optimizeTotalWrapper
branch
from
ec0466a to
b5ed135
Compare
Because of the use of evidence, the types for TotalWrapper are not being elided, leading to 10-15 JVM instruction methods for OpaqueInt::atMost. This is especially problematic because these methods are tagged 'inline', so they are significantly increasing size of the methods which use OpaqueInts. I've verified these changes now correctly elide types entirely, but this implementation is extremely fragile. Because scala sees evidence for conversion, it will implicitly use the evidence given a chance, which we don't want. The best I could come up with is a scary warning. :-\
isaacl
force-pushed
the
optimizeTotalWrapper
branch
from
b5ed135 to
2ba4f3b
Compare
|
I also looked at whether using |
|
Scala 3 hasn't implemented |
They are used in lila.
isaacl
commented
Oct 14, 2024
Add back `OpaqueInt`::`unary_-` which got removed accidentally during experimentation.
- Add extension methods similar to OpaqueInt
isaacl
force-pushed
the
optimizeTotalWrapper
branch
from
92689a3 to
b2ee2f5
Compare
|
great explanation in the source 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.
Suggestion cannot be applied right now. Please check back later.
Because of the use of evidence, the types for TotalWrapper are not being elided, leading to 10-15 JVM instruction methods for OpaqueInt::atMost. This is especially problematic because these methods are tagged 'inline', so they are significantly increasing size of the methods which use OpaqueInts.
I've verified these changes now correctly elide types entirely, but this implementation is extremely fragile. Because scala sees evidence for conversion, it will implicitly use the evidence given a chance, which we don't want.
The best I could come up with is a scary warning. :-\