GeezerVison is showing github.com — Rework constructor handling by dcodeIO · Pull Request #446 · AssemblyScript/assemblyscript
Conversation
As a follow-up to #445 this PR aims at reworking constructor handling. Currently, whenever a new is encountered, it calls the constructor if it is present and if it isn't, inlines a bare allocation with field initializers at that place. While we could get away with this, generating actual constructors for classes that don't explicitly define one would simplify fixing some remaining issues, while also reducing code size (inlined allocation and initializers quickly accumulate), instead of just working around them.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
dcodeIO
commented
Feb 1, 2019
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
|
Alright, this is passing the tests now. Changes:
- If a class doesn't have a declared constructor, one is generated that contains both a conditional allocation (if
this=0 was provided) to allocate the necessary memorym plus field initializers in any case. This allows providing this!=0 if the memory has already been allocated earlier (just as before), i.e. in a derived constructor.
- When
super() is called, it calls the now guaranteed to exist base constructor, but first allocates the necessary memory for this which might be larger due to additional own fields, and then initializes own fields (TODO: this can be optimized if there are no own fields)
Other notes:
- In order to dynamically generate functions that have an IR representation, a
NATIVE_CODE element in builtins has been introduced to represent them. A bit hacky, but doing all that in pure code would be huge.
- Decided to keep tracking of allocations in flows, for example for when a constructor can both return something manually instantiated or use
this normally. Doing that has a cost in code size when this is first accessed in separate branches, though, because there's one conditional allocation and field initializers in each branch then (whenever this is fist accessed). This can probably be optimized somehow if all branches are known to allocate, but since code is compiled sequentially at this point, there's not yet a way to know that beforehand.
- This doesn't bother with inlining yet due to the existence of Proposing user-defined inlining WebAssembly/binaryen#1889.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
|
}
|
|
|
|
|
|
/** Compiles just the body of a function in whatever is the current context. */
|
|
|
private compileFunctionBody(instance: Function): ExpressionRef[] {
|
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.
This one isn't strictly necessary now that I've gone another route, but who knows what it might be good for in the future.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
LGTM.
Is it possible inline constructors for now?
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
dcodeIO
commented
Feb 1, 2019
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
|
Inlining constructors seems to be limited in usefulness, except for the Pointer experiment we had a while ago where the constructor essentially eliminates itself when inlined after optimizations. Going to take a look, though, as it shouldn't be too hard to get working again (mostly a matter of looking up actual this instead of assuming index 0).
One special case is if there's no declared constructor, which in turn isn't decorated @inline and inlining these per-se seems to be inefficient if the class has a lot of fields. Currently in favor of not doing this and relying on the user to actually declare a constructor with the decorator if they want it to be inlined.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Yeah, I mean declare constructor with forced decorator @inline which could be useful for Pointer scenario
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Turns out that inlining constructors that didn't return a custom value never worked due to this check preventing it, BUT I think I can at least make the Pointer constructor work again.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Merging, continuing from here :)
|
MaxGraey and jtenner reacted with thumbs up emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
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.
|