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 — A couple fixes for Ivy JIT by alxhub · Pull Request #24565 · angular/angular

Conversation

@alxhub

@alxhub alxhub commented Jun 18, 2018

Copy link
Copy Markdown
Member
  • Support inputs and outputs from metadata.
  • Remove option to use default value parameter for inject() in code generation.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@googlebot googlebot added the cla: yes label Jun 18, 2018
@alxhub
fix(ivy): inject() no longer uses default value parameters
9b94685 
inject() was changed in da31db7 to not take a default value parameter,
so injectable_compiler_2 should not request the use of one when
using inject().
@alxhub
alxhub force-pushed the ivy-jit-fixes branch from dd0f678 to 744db24 Compare June 18, 2018 17:21
@mary-poppins

mary-poppins commented Jun 18, 2018

Copy link
Copy Markdown

You can preview 744db24 at https://pr24565-744db24.ngbuilds.io/.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
alfaproject
alfaproject reviewed Jun 18, 2018
View reviewed changes
Comment thread packages/core/src/render3/jit/directive.ts Outdated

@alfaproject alfaproject Jun 18, 2018

Copy link
Copy Markdown
Contributor

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.

Should this be a standard for loop or doesn't matter?

Sorry, something went wrong.

Uh oh!

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

@alxhub alxhub Jun 18, 2018

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.

It could be. I've been taking an approach where I use ES6/fluent code unless something is provably a performance bottleneck.

Sorry, something went wrong.

Uh oh!

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

@benlesh benlesh Jun 18, 2018

Copy link
Copy Markdown
Contributor

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'm with @alfaproject on this, may as well write a for loop. One less function allocation per call to this function.

Sorry, something went wrong.

Uh oh!

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

@alxhub alxhub Jun 18, 2018

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.

I find the fluent form much more readable, and it's used throughout the compiler today. I would rather not optimize prematurely. If benchmarks show that these temporary allocations (which are short lived anyway) cause significant performance issues, then we should optimize it. Until then, my goal is to keep the code as readable as possible.

Sorry, something went wrong.

Uh oh!

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

@alfaproject alfaproject Jun 19, 2018

Copy link
Copy Markdown
Contributor

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 meant:

for (const value of values) {
  const [field, property] = value.split(',').map(piece => piece.trim());
  map[field] = property || field;
}

which is just as readable (I think) with the benefit of being more performant.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@alxhub
alxhub requested a review from benlesh June 18, 2018 21:05
benlesh
benlesh previously requested changes Jun 18, 2018
View reviewed changes

@benlesh benlesh left a comment

Copy link
Copy Markdown
Contributor

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.

A few small things, overall looks good.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread packages/core/src/render3/jit/directive.ts Outdated

@benlesh benlesh Jun 18, 2018

Copy link
Copy Markdown
Contributor

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.

You might considier making this into a pure function that reduces into the map. It can be used to create the map above.

Sorry, something went wrong.

Uh oh!

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

@alxhub alxhub Jun 19, 2018

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.

Done.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread packages/core/src/render3/jit/directive.ts Outdated

@benlesh benlesh Jun 18, 2018

Copy link
Copy Markdown
Contributor

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.

remove the map() here, and do the trim() on the next line where you need to. One less Array allocation.

Sorry, something went wrong.

Uh oh!

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

@alxhub alxhub Jun 19, 2018

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.

As we discussed, I'm not worried about optimizing the allocations at the moment.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread packages/core/src/render3/jit/directive.ts Outdated

@benlesh benlesh Jun 18, 2018

Copy link
Copy Markdown
Contributor

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'm with @alfaproject on this, may as well write a for loop. One less function allocation per call to this function.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread packages/core/src/render3/jit/directive.ts Outdated

@benlesh benlesh Jun 18, 2018

Copy link
Copy Markdown
Contributor

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 know you didn't touch this, but is the omission of a hasOwnProperty check here intentional?

Sorry, something went wrong.

Uh oh!

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

@benlesh benlesh Jun 18, 2018

Copy link
Copy Markdown
Contributor

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.

And as below, this could be a for..of loop, that will save a function allocation per turn through this loop.

Sorry, something went wrong.

Uh oh!

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

@benlesh benlesh Jun 18, 2018

Copy link
Copy Markdown
Contributor

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 same questions/problems exist in other code below (that you didn't touch) in extractHostBindings

Sorry, something went wrong.

Uh oh!

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

@alxhub alxhub Jun 18, 2018

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.

I know you didn't touch this, but is the omission of a hasOwnProperty check here intentional.

Can you elaborate on what the hasOwnProperty check would check? I don't follow.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@alxhub
fix(ivy): support inputs/outputs in decorator metadata in JIT
f96f81b
@alxhub
alxhub force-pushed the ivy-jit-fixes branch from 744db24 to f96f81b Compare June 19, 2018 18:04
@alxhub
alxhub dismissed benlesh’s stale review June 20, 2018 16:45

Most comments addressed, please re-review.

@mary-poppins

mary-poppins commented Jun 21, 2018

Copy link
Copy Markdown

You can preview f96f81b at https://pr24565-f96f81b.ngbuilds.io/.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
benlesh
benlesh approved these changes Jun 21, 2018
View reviewed changes
@alxhub alxhub added action: merge The PR is ready for merge by the caretaker target: major This PR is targeted for the next major release labels Jun 21, 2018
mhevery
mhevery approved these changes Jun 21, 2018
View reviewed changes
@mhevery mhevery closed this in 7d3fd4d Jun 21, 2018
mhevery pushed a commit that referenced this pull request Jun 21, 2018
@alxhub @mhevery
fix(ivy): support inputs/outputs in decorator metadata in JIT (#24565)
7b2b1af 
PR Close #24565
@angular-automatic-lock-bot

angular-automatic-lock-bot Bot commented Sep 13, 2019

Copy link
Copy Markdown

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@angular-automatic-lock-bot angular-automatic-lock-bot Bot locked and limited conversation to collaborators Sep 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers
@alfaproject alfaproject alfaproject left review comments
@mhevery mhevery mhevery approved these changes
@benlesh benlesh benlesh approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

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

Assignees

No one assigned

Labels

action: merge The PR is ready for merge by the caretaker cla: yes target: major This PR is targeted for the next major release

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.

6 participants

@alxhub @mary-poppins @alfaproject @mhevery @benlesh @googlebot
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.