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

Add newSeqUninitialized, closes #6401 #6402

Merged
merged 1 commit into from Jan 3, 2018

Conversation

edubart
Copy link
Contributor

@edubart edubart commented Sep 19, 2017

This is useful as mentioned in the issue #6401

Note that this is only implemented for seq of numbers. I'm not sure if this is the correct implementation, however testing using this method I do see random numbers when initializing and no zeros anymore.

Please review.

@edubart edubart force-pushed the seq-uninitialized branch 2 times, most recently from 33de4ec to 1d4f8f8 Compare September 19, 2017 14:21
@edubart
Copy link
Contributor Author

edubart commented Sep 19, 2017

Doing some test, this implementation seems to work fine with small seqs, however huge seqs is not working:

Working example:

var a = newSeqUninitialized[int](10)
echo a[^1]

Failing example:

var a = newSeqUninitialized[int](1000000)
echo a[^1]

SIGSEGV: Illegal storage access. (Attempt to read from nil?)

I don't know why, its like the newSeqOfCap doesn't actually create a seq with the desired cap.

@Araq Can you help me on this?

@edubart
Copy link
Contributor Author

edubart commented Sep 19, 2017

I've created another proc in system to see if this was true.

proc getCap*[T](s: seq[T]): int =
  var s = cast[PGenericSeq](s)
  return s.reserved

My test case:

var a = newSeqOfCap[int](10)
echo a.getCap() 

The test case outputs 0, either my method is wrong, or newSeqOfCap isn't working as desired.
I've opened #6403 regarding this.

@cooldome
Copy link
Member

Have you actually seen noInit pragma in the nim language manual? https://nim-lang.org/docs/manual.html

@edubart
Copy link
Contributor Author

edubart commented Sep 20, 2017

I don't think noInit works with seq

@bluenote10
Copy link
Contributor

Why did you close this?

@edubart
Copy link
Contributor Author

edubart commented Sep 28, 2017

No interest by the upstream having this as a new core function, as this also achievable by using newSeqOfCap followed by setLen

@Yardanico
Copy link
Collaborator

@edubart "no interest by the upstream" are you sure?
well PRs are not accepted fast in nim repo, but they're accepted when Araq has time

@edubart
Copy link
Contributor Author

edubart commented Sep 28, 2017

quote

Eduardo Bart @edubart Sep 22 15:55
@Araq would be a plus if you fix it and also add newSeqUninitialized(len) and maybe a getCap(seq) function too

From IRC (bridge bot) @FromIRC Sep 22 15:55
<Araq> you haven't convinced me of the merits

From IRC (bridge bot) @FromIRC Sep 22 15:56
<Araq> your FFI use cases were all covered by setLen()

I'm sure

@yglukhov
Copy link
Member

what's the use case for this? IMO, newSeqOfCap should be enough, as it should not do initialization, and setLen should always nullify the extent no matter what to not introduce non-deterministic behavior. So to get max performance one has to do newSeqOfCap + subsequent adds.

@edubart
Copy link
Contributor Author

edubart commented Sep 28, 2017

Filling a huge seq (GBs of bytes length) non contiguously, fastly without double initialization, and also in parallel with OpenMP (we can't use add in parallel) , such as in machine learning tasks in https://github.com/mratsim/Arraymancer.

@Araq
Copy link
Member

Araq commented Sep 28, 2017

FYI I'm fine with this being closed.

@bluenote10
Copy link
Contributor

Pity, I find newSeqUninitialized communicates the behavior much clearer than newSeqOfCap + setLen.

@edubart
Copy link
Contributor Author

edubart commented Sep 28, 2017

Indeed, Araq had to tell me a few times to become clear that this could be achieved by using newSeqOfCap + setLen. From reading the manual, setLen is not clear if it does initializes or not when growing a seq, because nim default behavior is to initialize to zeros I've thought it would. But to my surprise setLen does not initialize to zeros when growing a seq and this could be done with the combination of both funcs.

@bluenote10
Copy link
Contributor

Same reasoning for me, I always thought this just wasn't possible.

@Yardanico
Copy link
Collaborator

@edubart maybe make a PR for docs then ? :)

@Araq Araq reopened this Sep 28, 2017
@Araq
Copy link
Member

Araq commented Sep 28, 2017

Thinking about it, this is arguably a setLen bug that is exploited here. :D

@mratsim
Copy link
Collaborator

mratsim commented Sep 28, 2017

I guess we need a PR for setLenUninitialized then ;)

@Araq Araq merged commit bbfe6e8 into nim-lang:devel Jan 3, 2018
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

7 participants