-
Notifications
You must be signed in to change notification settings - Fork 323
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
Register type definitions lazily #11938
base: develop
Are you sure you want to change the base?
Conversation
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.
- Few style-like comments inline. Treat as opinions.
- Overall this seems like the right change.
- what's the impact on benchmarks?
- explicitly: what's the impact on
test/Benchmarks/src/Startup/*.enso
startups?
* @param annotations the list of attached annotations | ||
* @param args the list of argument definitions | ||
*/ | ||
public record InitializationParts( |
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.
Does this new record have to be public? Because the record
defines not just public constructor, but also public getters. I do not think we want people around the code base to call the getters.
enso/engine/runtime/src/main/java/org/enso/interpreter/runtime/callable/function/FunctionSchema.java
Line 235 in 0c41e8d
public static final class Builder { |
Could we rather follow this example by @Akirathan where Pavel introduced FunctionSchema.Builder
?
* @return the supplier providing the value with the mapping function applied | ||
* @param <R> the result type | ||
*/ | ||
public <R> CachingSupplier<R> map(Function<T, R> f) { |
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 "tendency towards functional style" is clear...
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, on the other hand, really like it :)
this.constructorFunction = | ||
buildConstructorFunction( | ||
language, section, localScope, scopeBuilder, assignments, varReads, annotations, args); | ||
initializationResultSupplier.map( |
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.
Interesting use of the new map
function. The alternative would be to have a single field Supplier<InitializationResult>
stored in the AtomConstructor
. I seem to like it a bit more - shows the "fixed" vs. the lazily computed part of the structure.
new RuntimeAnnotation(annotation.name, closureRootNode) | ||
} | ||
|
||
new InitializationParts( |
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.
With the builder style, this would become:
AtomConstructor.newBuilder().
section(...).
scope(...).
assignments(...).
reads(...).
args(defs);
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.
LGTM. Agree with @JaroslavTulach on the commenteds about style. You don't have to schedule the benchmarks and compare them on this PR, but remember to please at least add a comment on this PR about the results.
Pull Request Description
close #10923
Changelog:
registerTypeDefinitions
registers constructors lazilyImportant Notes
The improvement of time spent in the
registerTypeDefinitions
method (highlighted):before
after
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.