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

Vector defaultValue argument #9478

Closed
wants to merge 1 commit into from

Conversation

RblSb
Copy link
Member

@RblSb RblSb commented May 25, 2020

Should fix #8040 and unblock #8495

@Simn
Copy link
Member

Simn commented May 25, 2020

I'm worried about code generation on static targets because the argument is optional. In particular, we have to make sure that this[i] = defaultValue does not unwrap Null<T> on each iteration as that would cause a big performance hit.

@RblSb
Copy link
Member Author

RblSb commented May 25, 2020

Some way to unwrap it before iteration, like final value:T = cast value?
In other case Vector probably can be generated as {array: [], length: 123} on js (only 10% slower for array access, but requires more work for other methods). Or filled with nulls (50% slower, still fine).

@back2dos
Copy link
Member

Adding optional arguments is technically a breaking change. Also I'm having trouble imagining that new Vector(1000000) has the same performance characteristics on JS with this change. I wonder if having a fill method that returns the vector itself won't do the trick while avoiding all sorts of issues.

@RblSb
Copy link
Member Author

RblSb commented May 27, 2020

In what cases this is breaking change?
Not against Vector.fill, but i think access performance should be fixed for new Vector too, and initilization performance mean less for storage of fixed size (static targets fill the vector anyway). Some opinions about my ideas above?

@RealyUniqueName
Copy link
Member

It's a breaking change because of a signature change. But that's not important because Vector.new is probably never used as a closure.
But having a loop for 100000 iterations inside of new Vector(100000) is important.

@back2dos
Copy link
Member

back2dos commented Jun 1, 2020

Sorry, forgot to respond.

It's a breaking change because of a signature change. But that's not important because Vector.new is probably never used as a closure.

I was thinking of allocation pools, which doesn't seem that far fetched. But yeah, it's probably not going to affect anyone.


I would propose this:

abstract Vector<T> {
  public function new(size:Int):Void;
  public function fill(value:T):Void;// exercise for the writer
  static public function prefilled<T>(size:Int, value:T):Vector<T> {
    #if xyz
      /* fancy xyz-specific magic here */
    #else // default implementation
      var ret = new Vector(size);
      ret.fill(value);
      return ret;
    #end
  }
}

@RealyUniqueName RealyUniqueName added this to the Release 4.2 milestone Jun 5, 2020
@Simn
Copy link
Member

Simn commented Jul 28, 2020

I'm closing this because I don't think having the Vector constructor loop over its length is acceptable. I'd be fine with adding a separate fill method as suggested, that might be useful in various situations anyway.

@Simn Simn closed this Jul 28, 2020
@RblSb RblSb deleted the vector_arg branch July 28, 2020 14:29
@tobil4sk tobil4sk mentioned this pull request Mar 1, 2023
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.

[js][java] Performance lost
4 participants