Create JavaScript array without using `new` keyword. #1987

tpmccallum · 2020-02-03T07:52:25Z

At present this line of code creates the heap using JavaScript's built-in array constructor which uses. JavaScripts new keyword (to create the array).

//Line 747
self.global(&format!("const heap = new Array({});", INITIAL_HEAP_OFFSET));
self.global("heap.fill(undefined);");

Assuming that the INITIAL_HEAP_OFFSET is always 32 (because it is set as a constant in the Rust code), below is the equivalent of what this code will produce; an Array Object with 32 items which are all undefined.

const heap = new Array(32);
//(32) [empty × 32]
//Where
var zero_element = heap[0];
//undefined
var one_element = heap[1];
//undefined

I originally read this in the wasm-bindgen documentation section js-objects-in-rust
I believe that this is the desired outcome for the program. All good.

Suggestion to consider

I am always reminded not to use the new keyword. Mainly by reading or listening to JavaScript "The Good Parts".

Here is an example, in a very broad context, of why it would be best not to use the new keyword. If the INITIAL_HEAP_OFFSET was ever anything but one number, the heap would be created in a different way. For example if two numbers are passed in, then an array of size 2 would be created; where both items in the array are individual numbers.

const heap = new Array(32, 32);
var zero_element = heap[0];
var one_element = heap[1];
//32
//32

I know that this is highly unlikely, due to the fact that the INITIAL_HEAP_OFFSET is set as a const in the Rust. But thought that I would put out the following suggestion for consideration anyway. This comes from a place of just wanting to contribute in a way that could make this already awesome program a little better. :)

Suggested update

The heap array could be created using the following code

const heap = [];
heap.length = INITIAL_HEAP_OFFSET;
heap[0]
heap[1]
//undefined
//undefined

This would create a JavaScript Array of length INITIAL_HEAP_OFFSET, where are items are undefined

The new code generates (in raw JavaScript)

const heap = [];
heap.length = 32;

Which produces

(32) [empty × 32]

In the same way that the original code does.

At present [this line of code](https://github.com/rustwasm/wasm-bindgen/blob/master/crates/cli-support/src/js/mod.rs#L747) creates the heap using JavaScript's new keyword. ``` //Line 747 self.global(&format!("const heap = new Array({});", INITIAL_HEAP_OFFSET)); self.global("heap.fill(undefined);"); ``` Assuming that the `INITIAL_HEAP_OFFSET` is always 32 (because it is set as a constant in the Rust code), below is the equivalent of what this code will produce; an Array Object with 32 items which are all undefined. ``` const heap = new Array(32); //(32) [empty × 32] //Where var zero_element = heap[0]; //undefined var one_element = heap[1]; //undefined ``` I believe that this is the desired outcome for the program. All good. ### Suggestion to consider I am always reminded **not** to use the `new` keyword. Mainly by reading or listening to JavaScript ["The Good Parts"](https://youtu.be/XFTOG895C7c?t=1654). For example if the `INITIAL_HEAP_OFFSET` was ever anything but one number, the heap would be created in a different way. For example if two numbers are passed in, then an array of size 2 would be created; where both items in the array are individual numbers. ``` const heap = new Array(32, 32); var zero_element = heap[0]; var one_element = heap[1]; //32 //32 ``` I know that this is highly unlikely, due to the fact that the `INITIAL_HEAP_OFFSET` is set as a `const` in the Rust. But thought that I would put out the following suggestion for consideration anyway. This comes from a place of just wanting to contribute in a way that could make this already awesome program a little better. :) ### Suggested update The heap array could be created using the following code ``` const heap = []; heap.length = INITIAL_HEAP_OFFSET; heap[0] heap[1] //undefined //undefined ``` This would create a JavaScript Array of length `INITIAL_HEAP_OFFSET`, where are items are `undefined` The new code generates (in raw JavaScript) ``` const heap = []; heap.length = 32; ``` Which produces ``` (32) [empty × 32] ``` In the same way that the original code does.

alexcrichton · 2020-02-04T20:27:54Z

Thanks for the PR! I'm always a fan of following better idioms, and the change looks great to me!

I think the test failures here are legitimate, though, mind rerunning them locally to auto-regenerate the expected output? I think you'll need to set BLESS=1 to update the tests when running.

tpmccallum · 2020-02-05T04:13:49Z

Hi @alexcrichton,
Awesome, thanks!

I ran the following test command with the new code from this PR (using BLESS=1 as you suggested).

export BLESS=1
cargo test -p wasm-bindgen-cli --test reference

The above, produced the following result

    Finished test [unoptimized + debuginfo] target(s) in 0.11s
     Running target/debug/deps/reference-c3db2c64df48e6fd
13 tests passed

I have created a gist of the whole procedure i.e. running tests on:

untouched code
updated code (no BLESS)
updated code (using BLESS)

The reason that I used the cargo test -p wasm-bindgen-cli --test reference command is that it is the test command which was running when the checks were not successful.

Please let me know if you need anything else.
I am happy to perform more work to ensure that this is perfect. I will be guided by you.

Kind regards
Tim

alexcrichton · 2020-02-05T15:48:31Z

Hm yeah that should be the right command, but did files change locally for you after running that? You should just need to add a new commit with the changes that BLESS=1 makes.

tpmccallum · 2020-02-06T01:40:41Z

Thanks @alexcrichton,
I have run the tests using BLESS=1, then have listed the files (in my branch which I am commiting for this PR) that have changed (compared to the original code in the repo). Please see the list below

mod.rs

tpmccallum$ diff ~/orig_bindgen/wasm-bindgen/crates/cli-support/src/js/mod.rs ~/wasm-bindgen/crates/cli-support/src/js/mod.rs
747,748c747,748
<         self.global(&format!("const heap = new Array({});", INITIAL_HEAP_OFFSET));
<         self.global("heap.fill(undefined);");
---
>         self.global(&format!("const heap = [];"));
>         self.global(&format!("heap.length = {};", INITIAL_HEAP_OFFSET));

import-catch.js

tpmccallum$ diff ~/orig_bindgen/wasm-bindgen/crates/cli/tests/reference/import-catch.js ~/wasm-bindgen/crates/cli/tests/reference/import-catch.js
3c3
< const heap = new Array(32);
---
> const heap = [];
5c5
< heap.fill(undefined);
---
> heap.length = 32;

empty.wat

tpmccallum$ diff ~/orig_bindgen/wasm-bindgen/crates/cli/tests/reference/empty.wat ~/wasm-bindgen/crates/cli/tests/reference/empty.wat
2c2
<   (memory (;0;) 17)
---
>   (memory (;0;) 16)

anyref-import-catch.wat

tpmccallum$ diff ~/orig_bindgen/wasm-bindgen/crates/cli/tests/reference/anyref-import-catch.wat ~/wasm-bindgen/crates/cli/tests/reference/anyref-import-catch.wat
7d6
<   (func $exported (type 0))
8a8
>   (func $exported (type 0))

anyref-empty.wat

tpmccallum$ diff ~/orig_bindgen/wasm-bindgen/crates/cli/tests/reference/anyref-empty.wat ~/wasm-bindgen/crates/cli/tests/reference/anyref-empty.wat
5c5
<   (memory (;0;) 17)
---
>   (memory (;0;) 16)

Please note: I believe that the following lines are not related to my code changes. Reason being, they are there before I make the changes to mode.rs (and of course are also still there afterwards).

<   (memory (;0;) 17)
---
>   (memory (;0;) 16)

I would really appreciate it if you or someone else could explain (or point to some documentation) facts about the 17 and 16. Not sure what those numbers mean in the context of memory.

I will go ahead and add a new commit with the changes that BLESS=1 made, as you suggested.

Chat soon
Tim

…tially updating generated files that are used for testing.

… set

tpmccallum · 2020-02-06T07:39:25Z

Hi @alexcrichton
Managed to get all tests to pass, just using BLESS=1
Please let me know if there is anything else you want me to do before this gets merged.
Thanks
Tim

Pauan · 2020-02-06T09:00:34Z

I appreciate the attempt to help, but I don't really like this change.

When using new Array(32) the JS engine knows that it's exactly 32 elements, so it can pre-allocate the array.

Also, using new Array(32) is idiomatic code if you want to create an array of a specific length. Creating an empty array and then immediately resizing seems quite unidiomatic, and I personally have never seen it done.

Crockford has never said that new should never be used (he was talking about JS classes, not Array), there are always exceptions to his principles. Even JSLint (which was made by Crockford) accepts new Array(32) without errors or warnings (but it correctly rejects new Array(32, 32)).

And all of this is auto-generated internal code, so there's no real chance of making a mistake.

Another minor consideration is that new Array(32) minifies better, though it is a fairly small difference.

tpmccallum · 2020-02-06T11:36:51Z

Hi @Pauan,
Thanks for the feedback.
Douglas Crockford's JavaScript The Good Parts book [1] mentions (on page 114) to "avoid new Object and new Array. Use {} and [] instead."

The original code and new code both achieve the task in two separate steps

const heap = new Array(32);
heap.fill(undefined);

const heap = [];
heap.length = 32;

Happy to leave the code as is, I agree that this is in an area which is auto-generated internal code.

As mentioned in the initial PR comment thought that I would put out the following suggestion for consideration anyway. This comes from a place of just wanting to contribute in a way that could make this already awesome program a little better.

Kind regards
Tim

[1] https://books.google.com.au/books?id=PXa2bby0oQ0C&printsec=frontcover&source=gbs_ge_summary_r&cad=0#v=onepage&q=Also%20avoid%20new%20Object%20and%20new%20Array.%20Use%20%7B%7D%20and%20%5B%5D%20instead&f=false

alexcrichton

Code looks good to me, but let's hold off on merging until @Pauan's concerns are addressed.

tpmccallum · 2020-02-06T21:24:15Z

Awesome!
Thank you @alexcrichton and @Pauan I really appreciate you giving your valuable time to review this. Happy to help where I can.
Chat soon
Tim

Pauan · 2020-02-06T21:24:34Z

@tpmccallum Yes, I have read Crockford's books, and also spoken to him many years ago. I have 14 years of experience with JavaScript, so I'm well aware of the JS idioms and best practices.

In his book, he is recommending to use [] rather than new Array(), which is good advice. That is very different from new Array(32).

As I said, Crockford created JSLint in order to enforce his advice. JSLint rejects new Array() and new Array(32, 32) but accepts new Array(32), because Crockford realizes that new Array(32) is a useful and valid pattern of code. There are always exceptions to every rule.

The code would still need fill, because setting length creates a sparse array. So it would have to be like this:

const heap = [];
heap.length = 32;
heap.fill(undefined);

This doesn't seem clearer to me than using new Array(32).

However, one thing that could be done to make the code clearer would be to change it to this:

const heap = new Array(32).fill(undefined);

So I would gladly accept a change like that.

tpmccallum · 2020-02-06T21:40:48Z

Thanks @Pauan
I will:

update the mod.rs to generate the equivalent of const heap = new Array(32).fill(undefined);
run the tests
push the new code
Chat soon

tpmccallum · 2020-02-06T22:47:49Z

All the tests have passed, looks like we are good to go!
Thanks again for your time.
I am really enjoying using wasm-bindgen.
Kind regards
Tim

alexcrichton · 2020-02-07T01:00:20Z

Thanks!

tpmccallum requested a review from alexcrichton February 3, 2020 21:45

Add closing parenthesis to close out self.global

29d80a7

tpmccallum added 3 commits February 6, 2020 14:11

Adding files which were altered by the BLESS=1 system variable. Essen…

c496b24

…tially updating generated files that are used for testing.

Adding code generated wat file, by way of running tests using BLESS=1

c4b907d

Adding table.wat that was generated by running the tests with BLESS=1…

bdb32bf

… set

alexcrichton approved these changes Feb 6, 2020

View reviewed changes

tpmccallum added 2 commits February 7, 2020 07:53

Update code that creates heap array line 747 mod.rs

1598da5

Updating files that are automatically generated when using BLESS=1

28572a6

Pauan approved these changes Feb 7, 2020

View reviewed changes

alexcrichton merged commit 0f3c53b into rustwasm:master Feb 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create JavaScript array without using `new` keyword. #1987

Create JavaScript array without using `new` keyword. #1987

tpmccallum commented Feb 3, 2020 •

edited

Loading

alexcrichton commented Feb 4, 2020

tpmccallum commented Feb 5, 2020

alexcrichton commented Feb 5, 2020

tpmccallum commented Feb 6, 2020

tpmccallum commented Feb 6, 2020

Pauan commented Feb 6, 2020 •

edited

Loading

tpmccallum commented Feb 6, 2020

alexcrichton left a comment

tpmccallum commented Feb 6, 2020

Pauan commented Feb 6, 2020

tpmccallum commented Feb 6, 2020

tpmccallum commented Feb 6, 2020

alexcrichton commented Feb 7, 2020

Create JavaScript array without using new keyword. #1987

Create JavaScript array without using new keyword. #1987

Conversation

tpmccallum commented Feb 3, 2020 • edited Loading

Suggestion to consider

Suggested update

alexcrichton commented Feb 4, 2020

tpmccallum commented Feb 5, 2020

alexcrichton commented Feb 5, 2020

tpmccallum commented Feb 6, 2020

tpmccallum commented Feb 6, 2020

Pauan commented Feb 6, 2020 • edited Loading

tpmccallum commented Feb 6, 2020

alexcrichton left a comment

Choose a reason for hiding this comment

tpmccallum commented Feb 6, 2020

Pauan commented Feb 6, 2020

tpmccallum commented Feb 6, 2020

tpmccallum commented Feb 6, 2020

alexcrichton commented Feb 7, 2020

Create JavaScript array without using `new` keyword. #1987

Create JavaScript array without using `new` keyword. #1987

tpmccallum commented Feb 3, 2020 •

edited

Loading

Pauan commented Feb 6, 2020 •

edited

Loading