GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Rework constructor handling by dcodeIO · Pull Request #446 · AssemblyScript/assemblyscript

Conversation

@dcodeIO

@dcodeIO dcodeIO commented Jan 31, 2019

Copy link
Copy Markdown
Member

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.

All reactions
dcodeIO added 6 commits January 31, 2019 10:54
@dcodeIO
move some stuff around
538ba88
@dcodeIO
refactor function compile step
0060a3c
@dcodeIO
fix indentation
6efa2a3
@dcodeIO
break everything, fix everything
9039dc2
@dcodeIO
document tests
9574185
@dcodeIO
fix std definition
451706b
@dcodeIO

dcodeIO commented Feb 1, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

dcodeIO
dcodeIO commented Feb 1, 2019
View reviewed changes
Comment thread src/compiler.ts
}

/** Compiles just the body of a function in whatever is the current context. */
private compileFunctionBody(instance: Function): ExpressionRef[] {

@dcodeIO dcodeIO Feb 1, 2019

Copy link
Copy Markdown
Member Author

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.

All reactions
@MaxGraey

MaxGraey commented Feb 1, 2019

Copy link
Copy Markdown
Member

LGTM.

Is it possible inline constructors for now?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@dcodeIO
cleanup
62edd61
@dcodeIO

dcodeIO commented Feb 1, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@MaxGraey

MaxGraey commented Feb 1, 2019

Copy link
Copy Markdown
Member

Yeah, I mean declare constructor with forced decorator @inline which could be useful for Pointer scenario

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@dcodeIO

dcodeIO commented Feb 1, 2019

Copy link
Copy Markdown
Member Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

dcodeIO added 2 commits February 1, 2019 16:50
@dcodeIO
inlining as before, though broken
53e0787
@dcodeIO
skip final alloc in any terminating scenario
0d476f8
@dcodeIO
dcodeIO force-pushed the rework-ctors branch from 7579063 to 0d476f8 Compare February 2, 2019 10:46
@dcodeIO
dcodeIO merged commit 2131c51 into master Feb 2, 2019
@dcodeIO

dcodeIO commented Feb 2, 2019

Copy link
Copy Markdown
Member Author

Merging, continuing from here :)

MaxGraey and jtenner reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@dcodeIO
dcodeIO deleted the rework-ctors branch March 7, 2019 23:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

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.

2 participants

@dcodeIO @MaxGraey
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.