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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

TypedArray#set #516

Closed
wants to merge 14 commits into from
Closed

TypedArray#set #516

wants to merge 14 commits into from

Conversation

jtenner
Copy link
Contributor

@jtenner jtenner commented Feb 25, 2019

This implementation tries to take heavy advantage of conditional compilation to enable array parameters.

Accomplishes

  • Setting from array of values
  • Setting from TypedArray
  • Setting from ArrayLike classes
  • Conversion of data types
  • Bypass tslint problems

The test suite passes, and I'm open to optimization. I know that speed is important, but because Array.isView() does not compile to a constant, it's hard to take advantage of conditional compilation in this case.

Issues:

Currently the only way to determine if the underlying access type of SourceT when it is an array, we must actually perform a (un)checked access to grab a concrete variable with the correct type SourceU. Without ever referencing it, we must rely on stub instanceof [numbertype] to validate what the source is, then use the SET_COPY() inline method to actually move the data.

Please feel free to help me fix this right away, because there is a very large demand to get this function working. 馃憤

@jtenner
Copy link
Contributor Author

jtenner commented Feb 25, 2019

There are a bunch of instances where tslint does not like the use of any. There are currently no workarounds for this.

Instead, we need to use // tslint:disable-next-line

@dcodeIO
Copy link
Member

dcodeIO commented Feb 26, 2019

To recap, looks like we'd need

  1. typeof EXPR evaluating to the type of any expression, which in turn requires that the resolver is able to resolve any expression without first compiling it as mentioned here and
  2. type aliases within function bodies (that's currently limited to top-level) to make this easier to work with

@MaxGraey MaxGraey changed the title Typedarray#set<SourceT, SourceU> TypedArray#set Feb 26, 2019
@jtenner
Copy link
Contributor Author

jtenner commented Feb 26, 2019

Yeah. I also need to add a slowpath check to see if the ArrayBuffer is writing to itself.

@jtenner
Copy link
Contributor Author

jtenner commented Feb 26, 2019

This is now ready for review again.

Current problems

  1. No static way to determine if class is ArrayLike. Wishlist:
    • Need a way to get typeof without instanceof stub value
    • Could use static compiler constant isArrayLike() (and some way to check and see if it uses unchecked() gets?)
  2. Code looks difficult to maintain because of many instanceof checks (related to 1)
  3. Too many useless comments, and probably not good enough comments on parts that need it

@jtenner
Copy link
Contributor Author

jtenner commented Feb 28, 2019

I vote that we close this pull request and wait for better type support.

@vgrichina
Copy link

@dcodeIO @MaxGraey is it hard to add necessary features to compiler?

@dcodeIO
Copy link
Member

dcodeIO commented Mar 1, 2019

Adding typeof is somewhat blocked by the resolver not yet being able to resolve any kind of expression without first compiling it (second part of #473), which is not necessarily hard but quite some work. Certainly will get to it eventually.

@dcodeIO
Copy link
Member

dcodeIO commented Jun 15, 2019

Have been thinking about this a bit lately, and I figured that maybe RTTI can be used to implement it. Target properties are known statically

const targetAlignLog2 = alignof<T>();
const targetIsFloat = isFloat<T>();

while source properties can be obtained from RTTI:

var sourceInfo = __typeinfo(changetype<BLOCK>(source).rtId);
var sourceAlignLog2: usize = 31 - Math.clz32((sourceInfo / TypeinfoFlags.VALUE_ALIGN_0) & 31);
var sourceIsFloat = (sourceInfo & TypeinfoFlags.VALUE_FLOAT) != 0;
var sourceIsManaged = (sourceInfo & TypeinfoFlags.VALUE_MANAGED) != 0; // disallow this

That'd then give us all the possible combinations of integers and floating point values, even for normal arrays. Challenge is to statically eliminate as much as possible by checking target first.

@jtenner
Copy link
Contributor Author

jtenner commented Jun 15, 2019

Does that solution account for Uint8ClampedArray? If the target is Uint8ClampedArray we can easily apply a clamp.

@dcodeIO
Copy link
Member

dcodeIO commented Jun 15, 2019

That's a special case, yeah, but it's only relevant when writing values, which is possible to check statically.

@jtenner
Copy link
Contributor Author

jtenner commented Jun 15, 2019

Alright. The way I see it, I can try to implement this again, or someone can take up the responsibility. It's father's day this weekend and I'll be out camping.

Since I don't know enough about rtti, it may be best for @MaxGraey or yourself to implement this feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants