%TypedArray%.prototype.map #349

jtenner · 2018-11-27T21:25:02Z

So far this is the best I have for implementing map.

This is my first pull request and I hope to contribute much more if this turns out well.

std/assembly/internal/typedarray.ts

MaxGraey · 2018-11-28T15:41:29Z

std/assembly/internal/typedarray.ts

+    var length = this.length;
+    var byteLength = this.byteLength;
+    var byteOffset = this.byteOffset;
+    var newbuff = allocateUnsafe(byteLength);


Why just not create var result = new TypedArray<T,V>(this.length) and result.byteOffset = this.byteOffset?

It's in the definition of the class

export abstract class TypedArray<T,V>

I can't create an abstract class with new, right?

yeah, now I see

TypedArray is abstract because you'd lose the subarray implementation on the concrete class. Though I agree that this should be easier, somehow, because as it stands it is missing GC registration.

Oh wait! I have an idea.

Maybe a built-in helper instantiate<T>(...constructorArgs) (here: T is this) that is able to instantiate anything, given the class as a type argument?

@dcodeIO I can implement the method on each subclass using the new keyword. Let me submit changes.

jtenner · 2018-11-28T16:14:46Z

Okay so this totally works. It feels a bit hacky, but the code is very clear and does the job. Is there a way to make this method more generic?

std/assembly/typedarray.ts

jtenner · 2018-11-28T16:20:06Z

How can I copy byteOffset if the field is readonly?

MaxGraey · 2018-11-28T16:25:03Z

store<i32>(changetype<usize>(result), this.byteOffset, offsetof<this>("byteOffset"));

MaxGraey · 2018-11-28T16:30:40Z

@dcodeIO for better design I think make sense use getter for byteOffset instead readonly field. wdyt?

jtenner · 2018-11-28T16:33:02Z

And for testing, how should I verify the map method works? Is there a place to modify the test suite?

MaxGraey · 2018-11-28T16:35:57Z

https://github.com/AssemblyScript/assemblyscript/blob/master/tests/compiler/std/typedarray.ts

jtenner · 2018-11-28T16:47:00Z

One last question, I'm looking through the spec and can't find the reason why we must copy the byteOffset to a new TypedArray. Is there a reason I'm missing?

MaxGraey · 2018-11-28T16:49:35Z

see how implemented __get and __set. Btw recommend use __unchecked_get/__unchecked_set interanally

dcodeIO · 2018-11-28T16:55:02Z

Not sure about setting `byteOffset´, as you are dealing with an entirely new array with a new backing buffer. Modifying byteOffset there might actually lead to out of bounds loads and stores.

MaxGraey · 2018-11-28T16:59:24Z

@dcodeIO yeah, you are right

jtenner · 2018-11-28T17:19:10Z

Yeah this pull request isn't gonna be easy.

jtenner · 2018-11-28T17:36:23Z

Looks like something is broken. Can someone help me identify the problem?

std/assembly/typedarray.ts

jtenner · 2018-11-28T19:07:24Z

Now it looks like I have a problem with the function callbacks.

 Error: Compile error
  ERROR AS200: Conversion from type '(val: i8, index: i32) => i8' to '(value: i8, index: i32, array: Int8Array) => i8' requires an explicit cast.

   assert(isInt8ArrayEqual(testMapArray1.map(squarei8), [1, 4, 9]));
                                             ~~~~~~~~
   in std/typedarray.ts(370,42)

I went to check out how Array.prototype.map performs the map operation and I see the callback parameter looks like this.

  map<U>(callbackfn: (value: T, index: i32, array: Array<T>) => U): Array<U> {

Am I missing something?

MaxGraey · 2018-11-28T19:32:00Z

Yes, currently signature of callback should fully declare. Change:

function squarei8(val: i8, index: i32): i8 { ... }

to

function squarei8(val: i8, index: i32, array: i8[]): i8 { ... }

jtenner · 2018-11-28T19:34:57Z

Okay. It looks like I've gotten it to the point where the tests look like they are supposed to look. There's still some issues though.

dcodeIO · 2018-11-28T20:42:35Z

std/assembly/typedarray.ts

+    var length = this.length;
+    var result = new Int8Array(this.length);
+    for (let i = 0; i < length; ++i) {
+      result.__unchecked_set(i, callbackfn(this.__unchecked_get(i), i, this));


This can also be written as unchecked(result[i] = callbackfn(this.__unchecked_get(i), i, this)), which will ensure that it still works whenever indexed access becomes more optimized or changes otherwise.

Thanks dcode, but when I try to use it, it errors about the type because it doesn't have an index signature.

Might be a TS error that results from the .d.ts not having it, but should compile with AS.

I tried compiling with AS and it fails when trying to run the tests.

Hmm, that's strange. Really shouldn't use __unchecked_set here.

For reference, the low-level way to write this is:

import { HEADER_SIZE } from "./internal/arraybuffer";

map(callbackfn: (value: i8, index: i32, array: Int8Array) => i8): Int8Array { var length = this.length; var result = new Int8Array(length); var src = changetype<usize>(this.buffer) + HEADER_SIZE + this.byteOffset; var dst = changetype<usize>(result.buffer) + HEADER_SIZE; // byteOffset is 0 for a new array for (let i = 0; i < length; ++i) { let off = <usize>i << alignof<i8>(); store<i8>(dst + off, callbackfn(load<i8>(src + off), i, this)); // can actually skip alignof<T> for i8/u8 because it's 0 anyway, but added it for clarity } return result; }

@MaxGraey please correct me if I made a mistake :)

MaxGraey · 2018-11-28T20:44:39Z

Also you should run npm run test:compiler -- --create for update test snapshots

jtenner · 2018-11-28T20:54:45Z

I somehow broke the bool test.

MaxGraey · 2018-11-28T20:56:58Z

It seems you need use npm run build first because build dist files outdated currently. So run:

npm run build

and

npm run test:compiler -- --create

later.
And don't forget exclude updated dist folder from commit.
@dcodeIO could you update dists?

jtenner · 2018-11-28T21:04:07Z

@MaxGraey Okay. What do you mean by disclude dist folder? Should I delete it and commit with empty dist folder?

MaxGraey · 2018-11-28T21:07:10Z

https://github.com/AssemblyScript/assemblyscript/blob/master/CONTRIBUTING.md#submitting-pull-requests

jtenner · 2018-11-28T21:10:13Z

@MaxGraey I'm confused by what this means. Should I leave the dist files as they are, from when I cloned the repo?

When I ran the build to do the testing it updated the files and I can't disclude them.

Should I use this?

git update-index --assume-unchanged dist/*

dcodeIO · 2018-11-28T21:12:30Z

When working on the compiler, make sure to run npm run clean to remove the dist files before running the tests. That makes sure that the sources are being run. Then, when committing, regardless of whether you decided to run npm run build for yourself, make sure to not commit any changes to dist/*. CI checks that, hence this PR fails.

jtenner · 2018-11-28T21:14:42Z

Okay. So all the tests pass. Pushing now. Please check my work, because it modified the dist folder.

dcodeIO · 2018-11-28T21:20:16Z

Modifying the dist folder will make CI fail automatically, unfortunately. Let me suggest that you git reset --soft HASH to the last commit from the master repo right before your first commit, and make a new condensed one from your changes with changes to the dist files excluded followed by an git push -f.

jtenner · 2018-11-28T21:27:02Z

@dcodeIO sorry about this. When I was asked to run npm build, it caused the dist folder to be overwritten. Is it possible to restore the files back to their original contents by just copy/pasting them instead? I'm afraid of git magic right now and I don't want to lose all my work :)

Edit: It does look like the files are diffed using git, so I restored their contents manually.

dcodeIO · 2018-11-28T21:38:42Z

What you'd usually do in such a scenario is, for example

$> git reset --soft 7596d732849c26d9ee80de2d295647c235fd5d4b

that keeps your local changes but resets the tree to the state before you made your first commit, followed by a

$> git add .
$> git reset dist/*

to add your changes back, except those to the dist files. Then creating a new first commit on your branch through

$> git commit -m "New commit message"

and force-pushing it to your branch, effectively replacing the chain of commits that was there before with your new single one

$> git push -f

Hope that helps :)

jtenner · 2018-11-28T21:40:14Z

Yeah thanks for the tip. It all seems to work now. Should I change the documentation, add some comments? It didn't seem like I needed any since it was all very obvious what the code was doing.

dcodeIO · 2018-11-28T22:25:16Z

std/assembly/typedarray.ts

 }

 export class Uint8ClampedArray extends TypedArray<u8,u32> {
  static readonly BYTES_PER_ELEMENT: usize = sizeof<u8>();

  @inline @operator("[]=")
  protected __set(index: i32, value: i32): void {
-    super.__set(index, max(min(value, 255), 0));
+    super.__unchecked_set(index, max(min(value, 255), 0));


Why did this change?

I must have accidentally replaced this. I am sorry. Let me change it back.

changing this function back to super.__set causes this error to occur when testing compiler/std/typedarray.ts

Fatal: Module::addFunction: $~lib/internal/typedarray/TypedArray<u8,u32>#__set already exists

Try renaming the function itself to __setClamped as a workaround.

may be you forget replace result.__set to result.__setClamped for map inside Uint8ClampedArray?

As it turns out, I don't think that matters because the values will already be clamped, and changing the function names simply does not resolve the issue.

It is trying to add the __set method to the table when it already exists apparently. Could this be a compiler error?

I can bring this chat to slack if you want, but I'll be around to work on this again tomorrow. Thank you @MaxGraey and @dcodeIO for your amazing help.

Yes, this is most likely a compiler bug that involves super.

Where can I look to help?

std/assembly/typedarray.ts

dcodeIO · 2018-11-29T12:14:09Z

The "Operation not supported" issue on unchecked(... this[i]) should be fixed by the above commit. Should now now be possible to use that syntax instead of __unchecked_get.

jtenner · 2018-11-29T14:14:47Z

We still get a bug when I tried to switch over to normal array access involving tslint. There is a signature problem.

Note that the cursor in the picture is where the error occurred. (I should have copied over the line numbers)

dcodeIO · 2018-11-29T14:23:54Z

Seems to originate from TypedArray<T,V> being an internal class only, with no declaration whatsoever that could include the index signature. Try to disable tslint on this exact line with a trailing // tslint:disable-line.

jtenner · 2018-11-29T14:35:18Z

Actually I think the typescript compiler throws the error from within tslint. It says specifically:

ERROR: Unsafe use of expression of type 'any'. [no-unsafe-any]

This is a TypeScript error. Not a tslint one.

jtenner · 2018-11-29T14:45:49Z

Of course adding this signature doesn't work, and causes the build to completely freak out with syntax errors. This is gonna be good.

dcodeIO · 2018-11-29T14:48:30Z

Well, while I hoped we could have avoided it, it appears that using __unchecked_get etc. is indeed the only viable option at this point :)

jtenner · 2018-11-29T14:52:26Z

We still need to figure out what the problem with is with the unclamped array problem anyway.

Testing compiler/std/typedarray.ts

Fatal: Module::addFunction: $~lib/internal/typedarray/TypedArray<u8,u32>#__set already exists
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! assemblyscript@0.5.0 test:compiler: `node tests/compiler`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the assemblyscript@0.5.0 test:compiler script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     C:\Users\jtenner\AppData\Roaming\npm-cache\_logs\2018-11-29T14_51_25_586Z-debug.log
npm ERR! Test failed.  See above for more details.
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! assemblyscript@0.5.0 make: `npm run clean && npm test && npm run build && npm test`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the assemblyscript@0.5.0 make script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

If I am not mistaken, perhaps there's a problem with operator overriding? I think it might have something to do with using the parent class's function name because I've renamed the functions in the Uint8UnclampedArray class.

 @inline @operator("[]=")
  protected __setClamped(index: i32, value: i32): void {
    super.__set(index, max(min(value, 255), 0));
  }

  @inline @operator("{}=")
  protected __unchecked_setClamped(index: i32, value: i32): void {
    super.__unchecked_set(index, max(min(value, 255), 0));
  }

The only place that uses the identifier __set is in the TypedArray<T, V> class.

dcodeIO · 2018-11-29T16:16:25Z

I think the problem is that through calling __set manually, that function becomes compiled already, while when a normal array access like something[i] is used, that only sees that the full accessor hasn't been compiled yet and compiles both the getter and the setter, which then results in the error due to the duplicate setter. Should be easy to fix, going to take a look :)

jtenner · 2018-11-29T20:09:52Z

@dcodeIO

Can't I just make a generic map function like this in TypedArray<T, V>?

export abstract class  TypedArray<T, V> {
  map(callbackfn: (value: T, index: i32, array: Int16Array) => T): this {
    var length = this.length;
    var result = instantiate<this>(length);
    for (let i = 0; i < length; ++i) {
      result.__unchecked_set(i, callbackfn(this.__unchecked_get(i), i, this));
    }
    return result;
  }
}

dcodeIO · 2018-11-29T20:19:57Z

Problem is that this (currently differs from how TS resolves this) is TypedArray<T,V> there, which is abstract. Nonetheless, it should work because abstract isn't checked currently, but would require an overload on each concrete class changetypeing from super.map's return type to the concrete type, similar to subarray.

dcodeIO · 2018-11-29T20:22:45Z

One solution could be to check whether a function makes use of the this type and compiling a suitable version of this function for each subclass, like one would expect. That'd make overloading and changetypeing unnecessary, but not sure how much work this would be.

jtenner · 2018-11-29T20:25:59Z

@dcodeIO I can implement this no problem! Also, we need to override the default functionality for Uint8ClampedArray and this will cause the other problem to still occur. In the meantime, I'll switch over the implementations to use instantiate<this>().

jtenner · 2018-11-29T20:31:38Z

I just realized something. __unchecked_set(index: i32, value: V) accepts type V. How do I convert T to V? Also, is this the reason why it needs to recompile the __set function?

dcodeIO · 2018-11-29T21:22:29Z

The idea behind the unconvenient T and V notation is that an Int8Array, for example, handles the i8 type T while that's actually a native i32 type V. Doing so allows us to omit masking/sign-extension at some occasions. For any signed small integer T, V is i32, for any unsigned small integer, V is u32. Hence, conversion works by <V>valueOfT.

Maybe renaming V to TNative would be clearer?

jtenner · 2018-11-29T21:24:42Z

Alright. The problem with this is that V and T are generic any type parameters. They do not overlap, and thus we get a tsc error like this:

Edit: Actually, I have an idea.

jtenner · 2018-11-29T21:33:24Z

// in internal/typedarray.ts
  map(callbackfn: (value: T, index: i32) => V): TypedArray<T, V> {
    var length = this.length;
    var result = instantiate<TypedArray<T, V>>(length);

    for (let i = 0; i < length; ++i) {
      result.__unchecked_set(
        i,
        callbackfn(this.__unchecked_get(i), i),
      );
    }
    return result;
  }

// for Int8Array
  map(callbackfn: (value: i8, index: i32, array: Int8Array) => i8): Int8Array {
    return <Int8Array>super.map((input: i8, idx: i32) => <i32>callbackfn(input, idx, this));
  }

…., see #349

…lyScript#349

…., see AssemblyScript#349

dcodeIO reviewed Nov 28, 2018

View reviewed changes

std/assembly/internal/typedarray.ts Outdated Show resolved Hide resolved

MaxGraey reviewed Nov 28, 2018

View reviewed changes

std/assembly/typedarray.ts Outdated Show resolved Hide resolved

MaxGraey reviewed Nov 28, 2018

View reviewed changes

std/assembly/typedarray.ts Outdated Show resolved Hide resolved

dcodeIO reviewed Nov 28, 2018

View reviewed changes

std/assembly/typedarray.ts Outdated Show resolved Hide resolved

MaxGraey reviewed Nov 28, 2018

View reviewed changes

std/assembly/typedarray.ts Outdated Show resolved Hide resolved

dcodeIO added a commit that referenced this pull request Nov 29, 2018

Fix unsupported element access on 'this', see #349

1d93877

dcodeIO added a commit that referenced this pull request Nov 29, 2018

Add 'instantiate<T>' builtin (like 'new' but from a type), see #349

1149abf

dcodeIO added a commit that referenced this pull request Nov 30, 2018

Document the use of two type parameters in loadUnsafe, TypedArray etc…

bf7dd1a

…., see #349

jtenner closed this Nov 30, 2018

willemneal pushed a commit to willemneal/assemblyscript that referenced this pull request Dec 26, 2018

Fix unsupported element access on 'this', see AssemblyScript#349

7a002d2

willemneal pushed a commit to willemneal/assemblyscript that referenced this pull request Dec 26, 2018

Add 'instantiate<T>' builtin (like 'new' but from a type), see Assemb…

0c40d2f

…lyScript#349

willemneal pushed a commit to willemneal/assemblyscript that referenced this pull request Dec 26, 2018

Document the use of two type parameters in loadUnsafe, TypedArray etc…

b7f9ba1

…., see AssemblyScript#349

%TypedArray%.prototype.map #349

%TypedArray%.prototype.map #349

Conversation

jtenner commented Nov 27, 2018

MaxGraey Nov 28, 2018 • edited Loading

Choose a reason for hiding this comment

jtenner Nov 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcodeIO Nov 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtenner commented Nov 28, 2018

jtenner commented Nov 28, 2018 • edited Loading

MaxGraey commented Nov 28, 2018

MaxGraey commented Nov 28, 2018 • edited Loading

jtenner commented Nov 28, 2018

MaxGraey commented Nov 28, 2018

jtenner commented Nov 28, 2018

MaxGraey commented Nov 28, 2018 • edited Loading

dcodeIO commented Nov 28, 2018

MaxGraey commented Nov 28, 2018

jtenner commented Nov 28, 2018

jtenner commented Nov 28, 2018

jtenner commented Nov 28, 2018

MaxGraey commented Nov 28, 2018

jtenner commented Nov 28, 2018

Choose a reason for hiding this comment

jtenner Nov 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcodeIO Nov 28, 2018 • edited Loading

Choose a reason for hiding this comment

MaxGraey commented Nov 28, 2018

jtenner commented Nov 28, 2018

MaxGraey commented Nov 28, 2018 • edited Loading

jtenner commented Nov 28, 2018 • edited Loading

MaxGraey commented Nov 28, 2018

jtenner commented Nov 28, 2018 • edited Loading

dcodeIO commented Nov 28, 2018

jtenner commented Nov 28, 2018

dcodeIO commented Nov 28, 2018

jtenner commented Nov 28, 2018 • edited Loading

dcodeIO commented Nov 28, 2018 • edited Loading

jtenner commented Nov 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcodeIO Nov 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcodeIO commented Nov 29, 2018

jtenner commented Nov 29, 2018 • edited Loading

dcodeIO commented Nov 29, 2018

jtenner commented Nov 29, 2018

jtenner commented Nov 29, 2018

dcodeIO commented Nov 29, 2018

jtenner commented Nov 29, 2018 • edited Loading

dcodeIO commented Nov 29, 2018

jtenner commented Nov 29, 2018

dcodeIO commented Nov 29, 2018 • edited Loading

dcodeIO commented Nov 29, 2018

jtenner commented Nov 29, 2018

jtenner commented Nov 29, 2018 • edited Loading

dcodeIO commented Nov 29, 2018 • edited Loading

jtenner commented Nov 29, 2018 • edited Loading

jtenner commented Nov 29, 2018 • edited Loading

MaxGraey Nov 28, 2018 •

edited

Loading

jtenner Nov 28, 2018 •

edited

Loading

dcodeIO Nov 28, 2018 •

edited

Loading

jtenner commented Nov 28, 2018 •

edited

Loading

MaxGraey commented Nov 28, 2018 •

edited

Loading

MaxGraey commented Nov 28, 2018 •

edited

Loading

jtenner Nov 28, 2018 •

edited

Loading

dcodeIO Nov 28, 2018 •

edited

Loading

MaxGraey commented Nov 28, 2018 •

edited

Loading

jtenner commented Nov 28, 2018 •

edited

Loading

jtenner commented Nov 28, 2018 •

edited

Loading

jtenner commented Nov 28, 2018 •

edited

Loading

dcodeIO commented Nov 28, 2018 •

edited

Loading

dcodeIO Nov 28, 2018 •

edited

Loading

jtenner commented Nov 29, 2018 •

edited

Loading

jtenner commented Nov 29, 2018 •

edited

Loading

dcodeIO commented Nov 29, 2018 •

edited

Loading

jtenner commented Nov 29, 2018 •

edited

Loading

dcodeIO commented Nov 29, 2018 •

edited

Loading

jtenner commented Nov 29, 2018 •

edited

Loading

jtenner commented Nov 29, 2018 •

edited

Loading