Skip to content
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

perf: don't create holey arrays #32155

Closed
wants to merge 7 commits into from

Conversation

@mhevery
Copy link
Member

commented Aug 15, 2019

Don't use Array constructor with the size value (ex. new Array(5)) - this will create a HOLEY_ELEMENTS array (even if this array is filled in later on!);

https://v8.dev/blog/elements-kinds
https://stackoverflow.com/questions/32054170/how-to-resize-an-array

@googlebot googlebot added the cla: yes label Aug 15, 2019

@mhevery mhevery force-pushed the mhevery:array_cleanup branch 6 times, most recently from 82acada to 4b66067 Aug 15, 2019

@mhevery mhevery marked this pull request as ready for review Aug 15, 2019

@mhevery mhevery requested review from IgorMinar and angular/fw-compiler as code owners Aug 15, 2019

@AndrewKushnir
Copy link
Contributor

left a comment

Looks good, just one question related to push with no arguments. Thank you.

modules/benchmarks/src/tree/util.ts Outdated Show resolved Hide resolved
@mhevery

This comment has been minimized.

Copy link
Member Author

commented Aug 16, 2019

@mhevery

This comment has been minimized.

Copy link
Member Author

commented Aug 16, 2019

MERGE ASSISTANCE: unrelated failure in presubmit can't seem to overwrite the status.

for (let i = 0; i < tQueryMatches.length; i += 2) {
const matchedNodeIdx = tQueryMatches[i];
if (matchedNodeIdx < 0) {
// we at the <ng-template> marker which might have results in views created based on this
// <ng-template> - those results will be in separate views though, so here we just leave
// null as a placeholder
result[i / 2] = null;
result[i >> 1] = null;

This comment has been minimized.

Copy link
@pkozlowski-opensource

pkozlowski-opensource Aug 19, 2019

Member

Could we please not use bitwise instead of 2?

This comment has been minimized.

Copy link
@alfaproject

alfaproject Aug 19, 2019

Contributor

Does it even help anything? I'd expect this kind of optimisation to be done by the compiler

This comment has been minimized.

Copy link
@mhevery

mhevery Aug 20, 2019

Author Member

Well the compiler can't know if i is even or odd and in one case it needs to use integer arithmetic an in another it needs to use floating, so I think the instructions generated needs to check this and then coerce it into an integer, so I don't think there is an efficient way for JS to do this other than use float and than try to coerce it back to integer.

I know that @pkozlowski-opensource is not a fan of bit shifting, but I am not a fan of division because I know what actually has to happen to perform devision.

This comment has been minimized.

Copy link
@mhevery

mhevery Aug 20, 2019

Author Member

test case

  • On Chrome same speed
  • On Safari bit-shift is 25% faster
  • On FireFox bit-shift is 20% faster.

Can we keep it?

This comment has been minimized.

Copy link
@alfaproject

alfaproject Aug 20, 2019

Contributor

I'd say keep it (but it looks like Chrome knows how to optimise this)

@@ -313,18 +314,18 @@ function materializeViewResults<T>(lView: LView, tQuery: TQuery, queryIndex: num
if (lQuery.matches === null) {
const tViewData = lView[TVIEW].data;
const tQueryMatches = tQuery.matches !;
const result: T|null[] = new Array(tQueryMatches.length / 2);
const result: T|null[] = newArray(tQueryMatches.length >> 1, null);

This comment has been minimized.

Copy link
@pkozlowski-opensource

pkozlowski-opensource Aug 19, 2019

Member

In this particular case pre-filling an array with null doesn't buy us anything - we could use a simple push directly. Could we please re-write as const result: T|null[] = [] and then result.push(...). Feel free to revert query changes and I can do this change in a follow-up PR.

But yeh, as written, I think that this might be making the queries code actually slower as we are writing to the same array twice.

This comment has been minimized.

Copy link
@mhevery

mhevery Aug 20, 2019

Author Member

perf case

Turns out push is 60% faster than a[i] way of growing an array. Will refactor.

@pkozlowski-opensource
Copy link
Member

left a comment

Could we please revert changes in queries, see: https://github.com/angular/angular/pull/32155/files#r315174783
(or happy to do it myself in a follow-up PR if this helps...)

@mhevery

This comment has been minimized.

Copy link
Member Author

commented Aug 20, 2019

@pkozlowski-opensource comments have been addressed.

Changed as requested

mhevery added 5 commits Aug 15, 2019
perf: don't create holey arrays
Don't use `Array` constructor with the size value (ex. `new Array(5)`) - this will create a `HOLEY_ELEMENTS` array (even if this array is filled in later on!);

https://v8.dev/blog/elements-kinds
https://stackoverflow.com/questions/32054170/how-to-resize-an-array

@mhevery mhevery force-pushed the mhevery:array_cleanup branch from 8bd9e0b to a8ccd49 Aug 20, 2019

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2019

ngdevelop-tech added a commit to ngdevelop-tech/angular that referenced this pull request Aug 27, 2019
perf: don't create holey arrays (angular#32155)
Don't use `Array` constructor with the size value (ex. `new Array(5)`) - this will create a `HOLEY_ELEMENTS` array (even if this array is filled in later on!);

https://v8.dev/blog/elements-kinds
https://stackoverflow.com/questions/32054170/how-to-resize-an-array

PR Close angular#32155
sabeersulaiman added a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
perf: don't create holey arrays (angular#32155)
Don't use `Array` constructor with the size value (ex. `new Array(5)`) - this will create a `HOLEY_ELEMENTS` array (even if this array is filled in later on!);

https://v8.dev/blog/elements-kinds
https://stackoverflow.com/questions/32054170/how-to-resize-an-array

PR Close angular#32155
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.