-
Notifications
You must be signed in to change notification settings - Fork 321
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
Generate UUIDs on demand #8728
Generate UUIDs on demand #8728
Conversation
Trying to avoid expensive `UUID.randomUUID()` unless we reallly need it.
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 am glad the with LazyId
solution seems to be working. Everything is nicely green. And the startup benchmarks seem to be a bit improved too.
Can you manually check the amount of UUID
in memory before and now at the end of execution? The memory consumption should dramatically decrease, right?
Measuring how much UUID
has been allocated on the heap (and GCed) would be interesting as well, but I am not sure how to perform such measurement reliably.
@@ -412,7 +411,7 @@ case object LambdaShorthandToLambda extends IRPass { | |||
) | |||
|
|||
val lambdaArg = DefinitionArgument.Specified( | |||
scrutineeName.copy(id = IR.randomId), | |||
scrutineeName.copy(id = null), |
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.
null
is "cheaper" value than any other value. Occupies nothing in the heap.
with IRKind.Primitive { | ||
var id: UUID @Identifier = randomId | ||
with IRKind.Primitive | ||
with LazyId { |
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.
LazyId
trait! That sounds promising.
engine/runtime-parser/src/main/scala/org/enso/compiler/core/ir/LazyId.scala
Show resolved
Hide resolved
private[this] var _id: UUID @Identifier = null | ||
|
||
override def id: UUID @Identifier = { | ||
_id |
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 thought that the
if (_id == null) {
_id = randomId()
}
_id
would be here, in this id
method. That this id
should never return null
.
Wouldn't it be getter to:
override def id: UUID @Identifier = getId()
?
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.
The problem is that in IR definitions we use .id
all over the place and we do not want to initialize it at that point. Only at the point when it is actually used, .getId()
, outside of IR.
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.
Can you make id
private then? Package private or subtree private? Or there could be:
def isIdInitialized(): Boolean
and we could write
id = if (keepIdentifiers && isIdInitialized()) getId() else null
everywhere.
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.
If it is only used within IRs, we can mark it protected
engine/runtime-parser/src/main/scala/org/enso/compiler/core/ir/LazyId.scala
Outdated
Show resolved
Hide resolved
...e/runtime-parser/src/main/scala/org/enso/compiler/core/ir/expression/errors/Resolution.scala
Outdated
Show resolved
Hide resolved
engine/runtime/src/test/scala/org/enso/compiler/test/pass/analyse/DataflowAnalysisTest.scala
Show resolved
Hide resolved
Added screenshots that illustrate improvement on a simple Hello World program. |
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.
Very nice and elegant improvement. Congratulations.
Pull Request Description
Trying to avoid expensive
UUID.randomUUID()
unless we reallly need it.Closes #8716.
Important Notes
Some improvement:
FWIW Total Time for a Hello World example
Before
After
Memory usage
Before
After
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
./run ide build
.